* [PATCH] kpartx: print "loop deleted" to stdout, not stderr @ 2018-02-05 10:36 Julian Andres Klode 2018-02-05 13:58 ` Martin Wilck 0 siblings, 1 reply; 5+ messages in thread From: Julian Andres Klode @ 2018-02-05 10:36 UTC (permalink / raw) To: Christophe Varoqui, Device-mapper development mailing list Commit fa643f5d2590028a59c671b81ab41383806fd258 moved some code around and changed the print for loop deleted from stdout to stderr - but this is not an error message, and also printed to stdout in another place, so let's just use printf() again here. Signed-off-by: Julian Andres Klode <julian.klode@canonical.com> --- kpartx/kpartx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c index c1af1c5e..2e882721 100644 --- a/kpartx/kpartx.c +++ b/kpartx/kpartx.c @@ -399,7 +399,7 @@ main(int argc, char **argv){ loopdev); r = 1; } else - fprintf(stderr, "loop deleted : %s\n", loopdev); + printf("loop deleted : %s\n", loopdev); } goto end; } -- 2.15.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] kpartx: print "loop deleted" to stdout, not stderr 2018-02-05 10:36 [PATCH] kpartx: print "loop deleted" to stdout, not stderr Julian Andres Klode @ 2018-02-05 13:58 ` Martin Wilck 2018-02-05 14:12 ` Julian Andres Klode 0 siblings, 1 reply; 5+ messages in thread From: Martin Wilck @ 2018-02-05 13:58 UTC (permalink / raw) To: Julian Andres Klode, Christophe Varoqui, Device-mapper development mailing list On Mon, 2018-02-05 at 11:36 +0100, Julian Andres Klode wrote: > Commit fa643f5d2590028a59c671b81ab41383806fd258 moved some > code around and changed the print for loop deleted from stdout > to stderr - but this is not an error message, and also printed > to stdout in another place, so let's just use printf() again > here. > > Signed-off-by: Julian Andres Klode <julian.klode@canonical.com> > --- > kpartx/kpartx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c > index c1af1c5e..2e882721 100644 > --- a/kpartx/kpartx.c > +++ b/kpartx/kpartx.c > @@ -399,7 +399,7 @@ main(int argc, char **argv){ > loopdev); > r = 1; > } else > - fprintf(stderr, "loop deleted : > %s\n", loopdev); > + printf("loop deleted : %s\n", > loopdev); > } > goto end; > } Hm. This is a log message, and as such, should go to stderr. kpartx, as a program primarily intended to run in udev rules, shouldn't print log messages to stdout unless we have compelling reasons for it to do so. I'm aware that this isn't done consistently in kpartx at the moment, but IMO your patch goes in the wrong direction. Regards Martin -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kpartx: print "loop deleted" to stdout, not stderr 2018-02-05 13:58 ` Martin Wilck @ 2018-02-05 14:12 ` Julian Andres Klode 2018-02-05 14:17 ` Julian Andres Klode 0 siblings, 1 reply; 5+ messages in thread From: Julian Andres Klode @ 2018-02-05 14:12 UTC (permalink / raw) To: Martin Wilck; +Cc: Device-mapper development mailing list On Mon, Feb 05, 2018 at 02:58:46PM +0100, Martin Wilck wrote: > On Mon, 2018-02-05 at 11:36 +0100, Julian Andres Klode wrote: > > Commit fa643f5d2590028a59c671b81ab41383806fd258 moved some > > code around and changed the print for loop deleted from stdout > > to stderr - but this is not an error message, and also printed > > to stdout in another place, so let's just use printf() again > > here. > > > > Signed-off-by: Julian Andres Klode <julian.klode@canonical.com> > > --- > > kpartx/kpartx.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c > > index c1af1c5e..2e882721 100644 > > --- a/kpartx/kpartx.c > > +++ b/kpartx/kpartx.c > > @@ -399,7 +399,7 @@ main(int argc, char **argv){ > > loopdev); > > r = 1; > > } else > > - fprintf(stderr, "loop deleted : > > %s\n", loopdev); > > + printf("loop deleted : %s\n", > > loopdev); > > } > > goto end; > > } > > > Hm. This is a log message, and as such, should go to stderr. kpartx, as > a program primarily intended to run in udev rules, shouldn't print log > messages to stdout unless we have compelling reasons for it to do so. > > I'm aware that this isn't done consistently in kpartx at the moment, > but IMO your patch goes in the wrong direction. It seems very consistent for me: stderr is used for errors and warnings, and printf() for the rest. There are two exceptions: This one, and kpartx/devmapper.c: fprintf(stderr, "found map %s for uuid %s, renaming to %s\n", But that one really is what I'd consider a log message. Stuff like "del devmap" and "loop deleted" seem somehow different - you tell it to delete a mapping, and to be verbose, and then it should tell you what it deleted, on stdout. Maybe people are parsing this, who knows? It definitely broke our tests because unexpected stderr output appeared. -- debian developer - deb.li/jak | jak-linux.org - free software dev ubuntu core developer i speak de, en ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kpartx: print "loop deleted" to stdout, not stderr 2018-02-05 14:12 ` Julian Andres Klode @ 2018-02-05 14:17 ` Julian Andres Klode 2018-02-05 15:02 ` Martin Wilck 0 siblings, 1 reply; 5+ messages in thread From: Julian Andres Klode @ 2018-02-05 14:17 UTC (permalink / raw) To: Martin Wilck; +Cc: Device-mapper development mailing list On Mon, Feb 05, 2018 at 03:12:33PM +0100, Julian Andres Klode wrote: > On Mon, Feb 05, 2018 at 02:58:46PM +0100, Martin Wilck wrote: > > On Mon, 2018-02-05 at 11:36 +0100, Julian Andres Klode wrote: > > > Commit fa643f5d2590028a59c671b81ab41383806fd258 moved some > > > code around and changed the print for loop deleted from stdout > > > to stderr - but this is not an error message, and also printed > > > to stdout in another place, so let's just use printf() again > > > here. > > > > > > Signed-off-by: Julian Andres Klode <julian.klode@canonical.com> > > > --- > > > kpartx/kpartx.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c > > > index c1af1c5e..2e882721 100644 > > > --- a/kpartx/kpartx.c > > > +++ b/kpartx/kpartx.c > > > @@ -399,7 +399,7 @@ main(int argc, char **argv){ > > > loopdev); > > > r = 1; > > > } else > > > - fprintf(stderr, "loop deleted : > > > %s\n", loopdev); > > > + printf("loop deleted : %s\n", > > > loopdev); > > > } > > > goto end; > > > } > > > > > > Hm. This is a log message, and as such, should go to stderr. kpartx, as > > a program primarily intended to run in udev rules, shouldn't print log > > messages to stdout unless we have compelling reasons for it to do so. > > > > I'm aware that this isn't done consistently in kpartx at the moment, > > but IMO your patch goes in the wrong direction. > > It seems very consistent for me: stderr is used for errors and warnings, > and printf() for the rest. There are two exceptions: This one, and > > kpartx/devmapper.c: fprintf(stderr, "found map %s for uuid %s, renaming to %s\n", > > But that one really is what I'd consider a log message. Stuff like > "del devmap" and "loop deleted" seem somehow different - you tell > it to delete a mapping, and to be verbose, and then it should tell > you what it deleted, on stdout. Maybe people are parsing this, who > knows? It definitely broke our tests because unexpected stderr output > appeared. > In fact, if you look at the bug mentioned in the previous patch, https://bugs.launchpad.net/ubuntu/+source/multipath-tools/+bug/1747044 you'll see that we explicitly check the output of kpartx -v -d for "loop deleted" apparently because kpartx was (or is?) flaky and returned an error even when it successfully deleted the loop device, as seen in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=860894 It seems reasonable that Debian and Ubuntu are not the only ones relying on that output. -- debian developer - deb.li/jak | jak-linux.org - free software dev ubuntu core developer i speak de, en ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kpartx: print "loop deleted" to stdout, not stderr 2018-02-05 14:17 ` Julian Andres Klode @ 2018-02-05 15:02 ` Martin Wilck 0 siblings, 0 replies; 5+ messages in thread From: Martin Wilck @ 2018-02-05 15:02 UTC (permalink / raw) To: Julian Andres Klode; +Cc: development mailing list, Device-mapper On Mon, 2018-02-05 at 15:17 +0100, Julian Andres Klode wrote: > On Mon, Feb 05, 2018 at 03:12:33PM +0100, Julian Andres Klode wrote: > > On Mon, Feb 05, 2018 at 02:58:46PM +0100, Martin Wilck wrote: > > > On Mon, 2018-02-05 at 11:36 +0100, Julian Andres Klode wrote: > > > > Commit fa643f5d2590028a59c671b81ab41383806fd258 moved some > > > > code around and changed the print for loop deleted from stdout > > > > to stderr - but this is not an error message, and also printed > > > > to stdout in another place, so let's just use printf() again > > > > here. > > > > > > > > Signed-off-by: Julian Andres Klode <julian.klode@canonical.com> > > > > --- > > > > kpartx/kpartx.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c > > > > index c1af1c5e..2e882721 100644 > > > > --- a/kpartx/kpartx.c > > > > +++ b/kpartx/kpartx.c > > > > @@ -399,7 +399,7 @@ main(int argc, char **argv){ > > > > loopdev); > > > > r = 1; > > > > } else > > > > - fprintf(stderr, "loop deleted > > > > : > > > > %s\n", loopdev); > > > > + printf("loop deleted : %s\n", > > > > loopdev); > > > > } > > > > goto end; > > > > } > > > > > > > > > Hm. This is a log message, and as such, should go to stderr. > > > kpartx, as > > > a program primarily intended to run in udev rules, shouldn't > > > print log > > > messages to stdout unless we have compelling reasons for it to do > > > so. > > > > > > I'm aware that this isn't done consistently in kpartx at the > > > moment, > > > but IMO your patch goes in the wrong direction. > > > > It seems very consistent for me: stderr is used for errors and > > warnings, > > and printf() for the rest. There are two exceptions: This one, and > > > > kpartx/devmapper.c: fprintf(stderr, "found map %s for > > uuid %s, renaming to %s\n", > > > > But that one really is what I'd consider a log message. Stuff like > > "del devmap" and "loop deleted" seem somehow different - you tell > > it to delete a mapping, and to be verbose, and then it should tell > > you what it deleted, on stdout. Maybe people are parsing this, who > > knows? It definitely broke our tests because unexpected stderr > > output > > appeared. > > > > In fact, if you look at the bug mentioned in the previous patch, > > https://bugs.launchpad.net/ubuntu/+source/multipath-tools/+bug/174704 > 4 > > you'll see that we explicitly check the output of kpartx -v -d for > "loop deleted" apparently because kpartx was (or is?) flaky and > returned > an error even when it successfully deleted the loop device, as seen > in > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=860894 That's a very different issue. Without digging much deeper into it, I'd like to repeat that kpartx' job is to delete _partition mappings_, not loop devices. The ability to delete the loop device as well ist just a convenience, saving users another call to "losetup -d". So it's not necessarily "flaky" for kpartx to return a non-zero error code if errors were encountered during it's core task (removing the partitions), even if removing the loop device was successful later. (*) > > It seems reasonable that Debian and Ubuntu are not the only ones > relying on that output. The fact that they rely on parsing this this message on stdout is sufficient justification for me to keep it there, even though I'm not buying your "errors to stderr, the rest to stdout" argument in general. Please submit again, adding a comment that Ubuntu/Debian rely on the presence of this output, lest it be changed again in the future. Regards Martin (*) One might argue whether the loop device shouldn't have been deleted in that error case, but that's yet another issue. -- Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-05 15:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-05 10:36 [PATCH] kpartx: print "loop deleted" to stdout, not stderr Julian Andres Klode 2018-02-05 13:58 ` Martin Wilck 2018-02-05 14:12 ` Julian Andres Klode 2018-02-05 14:17 ` Julian Andres Klode 2018-02-05 15:02 ` Martin Wilck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.