Merge lp:~azzar1/update-notifier/fix-lp-1800862 into lp:update-notifier/ubuntu

Proposed by Andrea Azzarone
Status: Merged
Merged at revision: 943
Proposed branch: lp:~azzar1/update-notifier/fix-lp-1800862
Merge into: lp:update-notifier/ubuntu
Diff against target: 129 lines (+75/-13)
3 files modified
debian/changelog (+7/-0)
src/livepatch.c (+63/-13)
src/update-notifier.c (+5/-0)
To merge this branch: bzr merge lp:~azzar1/update-notifier/fix-lp-1800862
Reviewer Review Type Date Requested Status
Iain Lane Approve
Sebastien Bacher Approve
Review via email: mp+358309@code.launchpad.net

Commit message

Check if a Livepatch patch has been applied during boot or before update-notifier has started. (LP: #1800862)

To post a comment you must log in.
Revision history for this message
Sebastien Bacher (seb128) wrote :

Thanks, looks good, one small comment inline though

review: Needs Fixing
Revision history for this message
Andrea Azzarone (azzar1) wrote :

Fixed.

Revision history for this message
Iain Lane (laney) wrote :

Some small comments & nitpicks inline - the logic looks sound for what it's supposed to do, thanks.

Is the uptime check to work around livepatch not clearing the status file at reboot? If so, has/could that bug been reported there? If the status is specific to the current boot then it sounds like it should be under /run, or it could record the boot ID (/proc/sys/kernel/random/boot_id). Worth a discussion with livepatch people anyway imho.

review: Needs Fixing
Revision history for this message
Andrea Azzarone (azzar1) wrote :

Branch updated.

Revision history for this message
Sebastien Bacher (seb128) wrote :

Seems fine to me expected the one thing I'm pointing in the code comments

review: Needs Fixing
Revision history for this message
Sebastien Bacher (seb128) wrote :

Ignore that, I didn't know that "%m" was doing that, cool :)

(note that Laney suggested maybe movung the g_ascii_strtoull call into the get_event_from_file function, did you discard doing that? either fine is fine with me)

review: Approve
Revision history for this message
Andrea Azzarone (azzar1) wrote :

> Ignore that, I didn't know that "%m" was doing that, cool :)
>
> (note that Laney suggested maybe movung the g_ascii_strtoull call into the
> get_event_from_file function, did you discard doing that? either fine is fine
> with me)

I perfer to keep it like it is because in theory the description depends on the event type. Right now we only have "applied %d", but this is going to change in the next few weeks once we implement notification for error status, etc.

Revision history for this message
Sebastien Bacher (seb128) wrote :

In fact there is a small issue, those warnings are new (when building on i386 at least)

"livepatch.c:119:27: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘guint64 {aka long long unsigned int}’ [-Wformat=]
                           "%lu Livepatch updates have been successfully applied"

review: Needs Fixing
Revision history for this message
Iain Lane (laney) wrote :

Use G_GUINT64_FORMAT to avoid that.

Is there is a similar warning about the parameter passed to ngettext()?

Revision history for this message
Sebastien Bacher (seb128) :
Revision history for this message
Sebastien Bacher (seb128) wrote :

yes, sorry I should have been explicit, those are the 2 new warnings

livepatch.c:119:27: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘guint64 {aka long long unsigned int}’ [-Wformat=]
                           "%lu Livepatch updates have been successfully applied
                           ^
livepatch.c:118:27: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘guint64 {aka long long unsigned int}’ [-Wformat=]
                 ngettext ("%lu Livepatch update has been successfully applied."
                           ^

944. By Andrea Azzarone

Use G_GUINT64_FORMAT to be plataform independent.

Revision history for this message
Andrea Azzarone (azzar1) wrote :

> Use G_GUINT64_FORMAT to avoid that.

Done.

Revision history for this message
Sebastien Bacher (seb128) wrote :

gcc is happy now, thx :)

review: Approve
Revision history for this message
Iain Lane (laney) wrote :

interesting, I thought that ngettext() *itself* might warn due to this (maybe it does at higher warning levels?)

small comment, otherwise +1 from me now, thanks!

945. By Andrea Azzarone

Handle overflow when using g_ascii_strtoull.

Revision history for this message
Iain Lane (laney) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2018-07-02 09:18:36 +0000
3+++ debian/changelog 2018-11-12 09:52:23 +0000
4@@ -1,3 +1,10 @@
5+update-notifier (3.192.8) UNRELEASED; urgency=medium
6+
7+ * Check if a Livepatch patch has been applied during boot or before
8+ update-notifier has started. (LP: #1800862)
9+
10+ -- Andrea Azzarone <andrea.azzarone@canonical.com> Fri, 02 Nov 2018 11:34:03 +0000
11+
12 update-notifier (3.192.7) cosmic; urgency=medium
13
14 * Use absolute patch when testing if whoopsie service is enabled.
15
16=== modified file 'src/livepatch.c'
17--- src/livepatch.c 2017-09-04 13:44:59 +0000
18+++ src/livepatch.c 2018-11-12 09:52:23 +0000
19@@ -2,9 +2,12 @@
20 #include "config.h"
21 #endif
22
23+#include <errno.h>
24 #include <glib.h>
25+#include <glib/gstdio.h>
26 #include <libnotify/notify.h>
27 #include <stdlib.h>
28+#include <sys/sysinfo.h>
29
30 #include "update-notifier.h"
31
32@@ -59,22 +62,69 @@
33 }
34 }
35
36+static long
37+get_uptime ()
38+{
39+ struct sysinfo info;
40+
41+ if (sysinfo (&info) == -1) {
42+ g_critical ("Failed to get uptime: %m");
43+ return -1;
44+ }
45+
46+ return info.uptime;
47+}
48+
49+static gboolean
50+file_modified_after_boot (const char* filename)
51+{
52+ GStatBuf status_stat;
53+ long uptime;
54+ time_t boot_timestamp;
55+
56+ /* In case of error it's safer to assume that the status file has been
57+ modified after boot in order to not miss the notification. */
58+ if (g_stat (STATUS_PATH, &status_stat) == -1)
59+ return TRUE;
60+
61+ if ((uptime = get_uptime ()) == -1)
62+ return TRUE;
63+
64+ boot_timestamp = time (NULL) - (time_t) uptime;
65+ return difftime (status_stat.st_mtim.tv_sec, boot_timestamp) >= 0;
66+}
67+
68 static void
69 show_status_notification ()
70 {
71- if (g_file_test (STATUS_PATH, G_FILE_TEST_EXISTS))
72- {
73- g_autofree gchar *event, *description;
74- get_event_from_file (STATUS_PATH, &event, &description);
75-
76- if (!g_strcmp0 (event, "applied")) {
77- g_autofree gchar *body = NULL;
78-
79- int num_updates = atoi(description);
80- body = g_strdup_printf(
81- ngettext("%d Livepatch update has been successfully applied.",
82- "%d Livepatch updates have been successfully applied.",
83- num_updates),
84+ g_autofree gchar *event = NULL;
85+ g_autofree gchar *description = NULL;
86+
87+ if (!g_file_test (STATUS_PATH, G_FILE_TEST_EXISTS))
88+ return;
89+
90+ if (!file_modified_after_boot (STATUS_PATH))
91+ return;
92+
93+ get_event_from_file (STATUS_PATH, &event, &description);
94+
95+ if (g_strcmp0 (event, "applied") == 0) {
96+ g_autofree gchar *body = NULL;
97+ gchar *endptr;
98+ gboolean is_overflow, conversion_failed;
99+
100+ errno = 0;
101+ guint64 num_updates = g_ascii_strtoull (description, &endptr, 10);
102+ is_overflow = (num_updates == G_MAXUINT64 && errno == ERANGE);
103+ conversion_failed = (num_updates == 0 && description == endptr);
104+
105+ if (is_overflow || conversion_failed) {
106+ g_warning ("Failed to parse the status file");
107+ } else if (num_updates != 0) {
108+ body = g_strdup_printf (
109+ ngettext ("%" G_GUINT64_FORMAT " Livepatch update has been successfully applied.",
110+ "%" G_GUINT64_FORMAT " Livepatch updates have been successfully applied.",
111+ num_updates),
112 num_updates);
113
114 show_notification (_("Canonical Livepatch"), body, NULL);
115
116=== modified file 'src/update-notifier.c'
117--- src/update-notifier.c 2018-04-20 18:02:03 +0000
118+++ src/update-notifier.c 2018-11-12 09:52:23 +0000
119@@ -654,6 +654,11 @@
120 #ifdef ENABLE_SCP
121 uevent_init();
122 #endif
123+
124+ /* check if livepatch patches has been applied during boot or before
125+ update-notifier has started (see LP: #1800862) */
126+ livepatch_check ();
127+
128 // init gio file monitoring
129 monitor_init (un);
130

Subscribers

People subscribed via source and target branches

to all changes: