Re: [systemd-devel] [PATCH 1/5] fsckd: Don't use strjoina on gettext() call
Looks good. Pushed. Thanks. Tom On Wed, Mar 11, 2015 at 2:25 PM, Didier Roche wrote: > Le 11/03/2015 09:34, Didier Roche a écrit : >> >> Le 11/03/2015 09:29, Martin Pitt a écrit : >>> >>> Hello all, >>> >>> Didier Roche [2015-03-10 17:56 +0100]: --- a/src/fsckd/fsckd.c +++ b/src/fsckd/fsckd.c @@ -272,7 +272,7 @@ static int plymouth_send_message(int plymouth_fd, const char *message, bool upda } static int manager_send_plymouth_message(Manager *m, const char *message) { -const char *plymouth_cancel_message = NULL; +_cleanup_free_ const char *plymouth_cancel_message = NULL, *l10n_cancel_message = NULL; int r; >>> >>> As far as I can see, this would free(l10n_cancel_message) on exit, but >>> you must never free the result of gettext(). So split this into >>> >>>_cleanup_free_ const char *plymouth_cancel_message = NULL; >>>const char *l10n_cancel_message; >> >> >> Indeed (weird I didn't get a double free crash), but the man page says >> it's statically allocated. Will fix it and report while bringing up the >> other patch with the architecture modification. >> Thanks! > > > Actually, none of then needed a cleanup_free as the second is a strjoina(). > > Here is the updated patch, thanks! > Didier > > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel > ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/5] fsckd: Don't use strjoina on gettext() call
On Tue, 10.03.15 11:32, Didier Roche (didro...@ubuntu.com) wrote: > static int manager_send_plymouth_message(Manager *m, const char *message) { > -const char *plymouth_cancel_message = NULL; > +_cleanup_free_ const char *plymouth_cancel_message = NULL; > int r; > > r = manager_connect_plymouth(m); > @@ -258,7 +258,7 @@ static int manager_send_plymouth_message(Manager *m, > const char *message) { > > m->plymouth_cancel_sent = true; > > -plymouth_cancel_message = strjoina("fsckd-cancel-msg:", > _("Press Ctrl+C to cancel all filesystem checks in progress")); > +plymouth_cancel_message = strjoin("fsckd-cancel-msg:", > _("Press Ctrl+C to cancel all filesystem checks in progress"), NULL); > Well, there's an OOM check missing now, as strjoin() can fail... That said, I think using strjoina is actually a good idea here, the string is not unbounded. Hence I think it would be a good idea to first assign the result of _() to a const char* variable in one step, and then concat it in a second... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/5] fsckd: Don't use strjoina on gettext() call
Le 11/03/2015 09:34, Didier Roche a écrit : Le 11/03/2015 09:29, Martin Pitt a écrit : Hello all, Didier Roche [2015-03-10 17:56 +0100]: --- a/src/fsckd/fsckd.c +++ b/src/fsckd/fsckd.c @@ -272,7 +272,7 @@ static int plymouth_send_message(int plymouth_fd, const char *message, bool upda } static int manager_send_plymouth_message(Manager *m, const char *message) { -const char *plymouth_cancel_message = NULL; +_cleanup_free_ const char *plymouth_cancel_message = NULL, *l10n_cancel_message = NULL; int r; As far as I can see, this would free(l10n_cancel_message) on exit, but you must never free the result of gettext(). So split this into _cleanup_free_ const char *plymouth_cancel_message = NULL; const char *l10n_cancel_message; Indeed (weird I didn't get a double free crash), but the man page says it's statically allocated. Will fix it and report while bringing up the other patch with the architecture modification. Thanks! Actually, none of then needed a cleanup_free as the second is a strjoina(). Here is the updated patch, thanks! Didier >From 0bcaa5707de7e71abcf507454ec98ac4dec8 Mon Sep 17 00:00:00 2001 From: Didier Roche Date: Tue, 10 Mar 2015 08:58:23 +0100 Subject: [PATCH 1/3] fsckd: Don't use strjoina on gettext() call --- src/fsckd/fsckd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c index 3c587a3..f24715c 100644 --- a/src/fsckd/fsckd.c +++ b/src/fsckd/fsckd.c @@ -272,7 +272,7 @@ static int plymouth_send_message(int plymouth_fd, const char *message, bool upda } static int manager_send_plymouth_message(Manager *m, const char *message) { -const char *plymouth_cancel_message = NULL; +const char *plymouth_cancel_message = NULL, *l10n_cancel_message = NULL; int r; r = manager_connect_plymouth(m); @@ -288,7 +288,8 @@ static int manager_send_plymouth_message(Manager *m, const char *message) { m->plymouth_cancel_sent = true; -plymouth_cancel_message = strjoina("fsckd-cancel-msg:", _("Press Ctrl+C to cancel all filesystem checks in progress")); +l10n_cancel_message = _("Press Ctrl+C to cancel all filesystem checks in progress"); +plymouth_cancel_message = strjoina("fsckd-cancel-msg:", l10n_cancel_message); r = plymouth_send_message(m->plymouth_fd, plymouth_cancel_message, false); if (r < 0) -- 2.1.4 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/5] fsckd: Don't use strjoina on gettext() call
Le 11/03/2015 09:29, Martin Pitt a écrit : Hello all, Didier Roche [2015-03-10 17:56 +0100]: --- a/src/fsckd/fsckd.c +++ b/src/fsckd/fsckd.c @@ -272,7 +272,7 @@ static int plymouth_send_message(int plymouth_fd, const char *message, bool upda } static int manager_send_plymouth_message(Manager *m, const char *message) { -const char *plymouth_cancel_message = NULL; +_cleanup_free_ const char *plymouth_cancel_message = NULL, *l10n_cancel_message = NULL; int r; As far as I can see, this would free(l10n_cancel_message) on exit, but you must never free the result of gettext(). So split this into _cleanup_free_ const char *plymouth_cancel_message = NULL; const char *l10n_cancel_message; Indeed (weird I didn't get a double free crash), but the man page says it's statically allocated. Will fix it and report while bringing up the other patch with the architecture modification. Thanks! ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/5] fsckd: Don't use strjoina on gettext() call
Hello all, Didier Roche [2015-03-10 17:56 +0100]: > --- a/src/fsckd/fsckd.c > +++ b/src/fsckd/fsckd.c > @@ -272,7 +272,7 @@ static int plymouth_send_message(int plymouth_fd, const > char *message, bool upda > } > > static int manager_send_plymouth_message(Manager *m, const char *message) { > -const char *plymouth_cancel_message = NULL; > +_cleanup_free_ const char *plymouth_cancel_message = NULL, > *l10n_cancel_message = NULL; > int r; As far as I can see, this would free(l10n_cancel_message) on exit, but you must never free the result of gettext(). So split this into _cleanup_free_ const char *plymouth_cancel_message = NULL; const char *l10n_cancel_message; ? > > r = manager_connect_plymouth(m); > @@ -288,7 +288,8 @@ static int manager_send_plymouth_message(Manager *m, > const char *message) { > > m->plymouth_cancel_sent = true; > > -plymouth_cancel_message = strjoina("fsckd-cancel-msg:", > _("Press Ctrl+C to cancel all filesystem checks in progress")); > +l10n_cancel_message = _("Press Ctrl+C to cancel all > filesystem checks in progress"); > +plymouth_cancel_message = strjoina("fsckd-cancel-msg:", > l10n_cancel_message); Martin -- Martin Pitt| http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org) ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/5] fsckd: Don't use strjoina on gettext() call
Le 10/03/2015 11:36, Lennart Poettering a écrit : On Tue, 10.03.15 11:32, Didier Roche (didro...@ubuntu.com) wrote: static int manager_send_plymouth_message(Manager *m, const char *message) { -const char *plymouth_cancel_message = NULL; +_cleanup_free_ const char *plymouth_cancel_message = NULL; int r; r = manager_connect_plymouth(m); @@ -258,7 +258,7 @@ static int manager_send_plymouth_message(Manager *m, const char *message) { m->plymouth_cancel_sent = true; -plymouth_cancel_message = strjoina("fsckd-cancel-msg:", _("Press Ctrl+C to cancel all filesystem checks in progress")); +plymouth_cancel_message = strjoin("fsckd-cancel-msg:", _("Press Ctrl+C to cancel all filesystem checks in progress"), NULL); Well, there's an OOM check missing now, as strjoin() can fail... That said, I think using strjoina is actually a good idea here, the string is not unbounded. Hence I think it would be a good idea to first assign the result of _() to a const char* variable in one step, and then concat it in a second... Makes sense. Here is the suggested change. Didier PS: just posting a couple of patches from your suggestion, I'll work on the bigger discussion of passing the fsck pipe to systemd-fsckd later in the week. >From ab84557e5840ff0a606b4b6f694b655b85bdcd45 Mon Sep 17 00:00:00 2001 From: Didier Roche Date: Tue, 10 Mar 2015 08:58:23 +0100 Subject: [PATCH 1/2] fsckd: Don't use strjoina on gettext() call --- src/fsckd/fsckd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c index 3c587a3..f23f272 100644 --- a/src/fsckd/fsckd.c +++ b/src/fsckd/fsckd.c @@ -272,7 +272,7 @@ static int plymouth_send_message(int plymouth_fd, const char *message, bool upda } static int manager_send_plymouth_message(Manager *m, const char *message) { -const char *plymouth_cancel_message = NULL; +_cleanup_free_ const char *plymouth_cancel_message = NULL, *l10n_cancel_message = NULL; int r; r = manager_connect_plymouth(m); @@ -288,7 +288,8 @@ static int manager_send_plymouth_message(Manager *m, const char *message) { m->plymouth_cancel_sent = true; -plymouth_cancel_message = strjoina("fsckd-cancel-msg:", _("Press Ctrl+C to cancel all filesystem checks in progress")); +l10n_cancel_message = _("Press Ctrl+C to cancel all filesystem checks in progress"); +plymouth_cancel_message = strjoina("fsckd-cancel-msg:", l10n_cancel_message); r = plymouth_send_message(m->plymouth_fd, plymouth_cancel_message, false); if (r < 0) -- 2.1.4 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 1/5] fsckd: Don't use strjoina on gettext() call
Le 09/03/2015 19:11, Lennart Poettering a écrit : So I fixed a number of issues, but there are some bits I am not really keen to fix, mostly because it's hard for me to test as I don't use a file system that still requires fsck... - /dev/console needs to be opened only on demand but not continously, to keep the SAK window as small as possible. - A clear regime needs to be enforced: either functions log errors on their own, or they don't. In the latter case the calling function needs to log errors instead. Currently some of the functions (like send_message_plymouth_socket()) log some errors but don't log others. And sometimes if they currently do log the functions invoking them log each error a second time! (As is the case for send_message_plymouth() which calls send_message_plymouth_socket()). - We should avoid mixing stack allocation calls and function calls. See man page of alloca() about this. Hence: invoking strjoina() around gettext calls is *not* OK. And there's probably more to fix... Anyway, please look into fixing this, I am kinda relying on patches for this, as I don't use this myself. Fedora isn't set up for it, nor do I use a file system that still requires fsck at boot... Those 3 points should be fixed or at least enhanced with the set of patches I'm attaching now. Please look in particular on the 02 one as I changed back some errors values introduced by your recent commits (I'm unsure if that was just an oversight). I tried to rationalize the log erroring to be more coherent (and especially avoiding double erroring), hope this is what you wanted. I didn't touch to the communication (and so, global flow) from systemd-fsck to systemd-fsckd point yet as changing it raises quite some issues as discussed in the other part of the thread (how to ensure the unit instance is in error if fsck fails or is cancel? How to even send the cancel message request itself if we don't have any communication back?) Feel free to point any mistake. Didier >From 4d279e1818dd05dccdf4595a5bf1f3dc32c3e8c2 Mon Sep 17 00:00:00 2001 From: Didier Roche Date: Tue, 10 Mar 2015 08:58:23 +0100 Subject: [PATCH 1/5] fsckd: Don't use strjoina on gettext() call --- src/fsckd/fsckd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fsckd/fsckd.c b/src/fsckd/fsckd.c index dedc828..3fc923b 100644 --- a/src/fsckd/fsckd.c +++ b/src/fsckd/fsckd.c @@ -242,7 +242,7 @@ static int plymouth_send_message(int plymouth_fd, const char *message, bool upda } static int manager_send_plymouth_message(Manager *m, const char *message) { -const char *plymouth_cancel_message = NULL; +_cleanup_free_ const char *plymouth_cancel_message = NULL; int r; r = manager_connect_plymouth(m); @@ -258,7 +258,7 @@ static int manager_send_plymouth_message(Manager *m, const char *message) { m->plymouth_cancel_sent = true; -plymouth_cancel_message = strjoina("fsckd-cancel-msg:", _("Press Ctrl+C to cancel all filesystem checks in progress")); +plymouth_cancel_message = strjoin("fsckd-cancel-msg:", _("Press Ctrl+C to cancel all filesystem checks in progress"), NULL); r = plymouth_send_message(m->plymouth_fd, plymouth_cancel_message, false); if (r < 0) -- 2.1.4 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel