Hi Hannes,

On Wed, 30 Apr 2008 11:04:26 +0200, hare@suse.de (Hannes Reinecke) wrote:
> Calling 'multipath -ll' on devices with paths results in
> lots of error messages; they really should be suppressed
> for the normal output. The user can always have them printed
> out by increasing verbosity.

In general, serious error messages should be printed by default.
Otherwise, the users (even developers) may overlook serious errors.

Please see the detailed comments below.

> @@ -32,7 +33,7 @@ int execute_program(char *path, char *value, int len)
> int retval;
> int count;
> int status;
> - int fds[2];
> + int fds[2], null_fd;
> pid_t pid;
> char *pos;
> char arg[PROGRAM_SIZE];
> @@ -75,7 +76,16 @@ int execute_program(char *path, char *value, int len)
> close(STDOUT_FILENO);
>
> /* dup write side of pipe to STDOUT */
> - dup(fds[1]);
> + if (dup(fds[1]) < 0)
> + return -1;
> +
> + /* Ignore writes to stderr */
> + null_fd = open("/dev/null", O_WRONLY);
> + if (null_fd > 0) {
> + close(STDERR_FILENO);
> + dup(null_fd);
> + close(null_fd);
> + }
>
> retval = execv(argv[0], argv);
>

This looks discarding all error messages from callouts anyway,
even if we want to get them.
Can you use the verbosity setting here, too?


> @@ -642,7 +642,7 @@ get_prio (struct path * pp)
> }
> pp->priority = prio_getprio(pp->prio, pp);
> if (pp->priority < 0) {
> - condlog(0, "%s: %s prio error", pp->dev, prio_name(pp->prio));
> + condlog(3, "%s: %s prio error", pp->dev, prio_name(pp->prio));
> pp->priority = PRIO_UNDEF;
> return 1;
> }

When prio_getprio() fails, that path is not used for multipath.
I think that it is a kind of serious error and the verbosity should
be "2" at least.

Why do you need to suppress this message by default?


> @@ -663,7 +663,7 @@ get_uid (struct path * pp)
> condlog(0, "error formatting uid callout command");
> memset(pp->wwid, 0, WWID_SIZE);
> } else if (execute_program(buff, pp->wwid, WWID_SIZE)) {
> - condlog(0, "error calling out %s", buff);
> + condlog(3, "error calling out %s", buff);
> memset(pp->wwid, 0, WWID_SIZE);
> return 1;
> }

ditto.

Thanks,
Kiyoshi Ueda

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Kiyoshi Ueda wrote:

Hi Hannes,

On Wed, 30 Apr 2008 11:04:26 +0200, hare@suse.de (Hannes Reinecke) wrote:

Calling 'multipath -ll' on devices with paths results in
lots of error messages; they really should be suppressed
for the normal output. The user can always have them printed
out by increasing verbosity.


In general, serious error messages should be printed by default.
Otherwise, the users (even developers) may overlook serious errors.


Agreed in principle. However, most of these error messages are secondary
messages (ie the are generated after the principal cause was reported)
so they do not carry any additional information to the 'normal' user.

Case in point here is for a path failure: this is multipathing, after all,
the whole point of which is that we _can_ handle path failure seamlessly.
So throwing error messages for a perfectly normal flow of operation is
really confusing.


Please see the detailed comments below.


@@ -32,7 +33,7 @@ int execute_program(char *path, char *value, int len)
int retval;
int count;
int status;
- int fds[2];
+ int fds[2], null_fd;
pid_t pid;
char *pos;
char arg[PROGRAM_SIZE];
@@ -75,7 +76,16 @@ int execute_program(char *path, char *value, int len)
close(STDOUT_FILENO);

/* dup write side of pipe to STDOUT */

- dup(fds[1]);
+ if (dup(fds[1]) < 0)
+ return -1;
+
+ /* Ignore writes to stderr */
+ null_fd = open("/dev/null", O_WRONLY);
+ if (null_fd > 0) {
+ close(STDERR_FILENO);
+ dup(null_fd);
+ close(null_fd);
+ }

retval = execv(argv[0], argv);



This looks discarding all error messages from callouts anyway,
even if we want to get them.
Can you use the verbosity setting here, too?


Yes, we could; no problem.




@@ -642,7 +642,7 @@ get_prio (struct path * pp)
}
pp->priority = prio_getprio(pp->prio, pp);
if (pp->priority < 0) {
- condlog(0, "%s: %s prio error", pp->dev, prio_name(pp->prio));
+ condlog(3, "%s: %s prio error", pp->dev, prio_name(pp->prio));
pp->priority = PRIO_UNDEF;
return 1;
}


When prio_getprio() fails, that path is not used for multipath.
I think that it is a kind of serious error and the verbosity should
be "2" at least.

Why do you need to suppress this message by default?


Because the prioritizer also will fail if a path failed. In this case
you get quite some errors like 'prio_alua failed' on a failed path,
inducing you to check if the prio_alua program might be wrong.
In fact, everything is alright and working as expected, only
that a path has failed.
So this error message might a well be suppressed.




@@ -663,7 +663,7 @@ get_uid (struct path * pp)
condlog(0, "error formatting uid callout command");
memset(pp->wwid, 0, WWID_SIZE);
} else if (execute_program(buff, pp->wwid, WWID_SIZE)) {
- condlog(0, "error calling out %s", buff);
+ condlog(3, "error calling out %s", buff);
memset(pp->wwid, 0, WWID_SIZE);
return 1;
}


ditto.


Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Hi Hannes,

On Fri, 02 May 2008 08:55:11 +0200, Hannes Reinecke wrote:
> >> @@ -642,7 +642,7 @@ get_prio (struct path * pp)
> >> }
> >> pp->priority = prio_getprio(pp->prio, pp);
> >> if (pp->priority < 0) {
> >> - condlog(0, "%s: %s prio error", pp->dev, prio_name(pp->prio));
> >> + condlog(3, "%s: %s prio error", pp->dev, prio_name(pp->prio));
> >> pp->priority = PRIO_UNDEF;
> >> return 1;
> >> }
> >
> > When prio_getprio() fails, that path is not used for multipath.
> > I think that it is a kind of serious error and the verbosity should
> > be "2" at least.
> >
> > Why do you need to suppress this message by default?
>
> Because the prioritizer also will fail if a path failed. In this case
> you get quite some errors like 'prio_alua failed' on a failed path,
> inducing you to check if the prio_alua program might be wrong.
> In fact, everything is alright and working as expected, only
> that a path has failed.
> So this error message might a well be suppressed.

OK, agreed on this.


> > Hi Hannes,
> >
> > On Wed, 30 Apr 2008 11:04:26 +0200, hare@suse.de (Hannes Reinecke) wrote:
> >> Calling 'multipath -ll' on devices with paths results in
> >> lots of error messages; they really should be suppressed
> >> for the normal output. The user can always have them printed
> >> out by increasing verbosity.
> >
> > In general, serious error messages should be printed by default.
> > Otherwise, the users (even developers) may overlook serious errors.
> >
> Agreed in principle. However, most of these error messages are secondary
> messages (ie the are generated after the principal cause was reported)
> so they do not carry any additional information to the 'normal' user.
>
> Case in point here is for a path failure: this is multipathing, after all,
> the whole point of which is that we _can_ handle path failure seamlessly.
> So throwing error messages for a perfectly normal flow of operation is
> really confusing.
>
> > Please see the detailed comments below.
> >
> >> @@ -32,7 +33,7 @@ int execute_program(char *path, char *value, int len)
> >> int retval;
> >> int count;
> >> int status;
> >> - int fds[2];
> >> + int fds[2], null_fd;
> >> pid_t pid;
> >> char *pos;
> >> char arg[PROGRAM_SIZE];
> >> @@ -75,7 +76,16 @@ int execute_program(char *path, char *value, int len)
> >> close(STDOUT_FILENO);
> >>
> >> /* dup write side of pipe to STDOUT */
> >> - dup(fds[1]);
> >> + if (dup(fds[1]) < 0)
> >> + return -1;
> >> +
> >> + /* Ignore writes to stderr */
> >> + null_fd = open("/dev/null", O_WRONLY);
> >> + if (null_fd > 0) {
> >> + close(STDERR_FILENO);
> >> + dup(null_fd);
> >> + close(null_fd);
> >> + }
> >>
> >> retval = execv(argv[0], argv);
> >>
> >
> > This looks discarding all error messages from callouts anyway,
> > even if we want to get them.
> > Can you use the verbosity setting here, too?
> >
> Yes, we could; no problem.

Thanks.
This part should print primary error messages which include much
information to analyze the cause of the error.


> >> @@ -663,7 +663,7 @@ get_uid (struct path * pp)
> >> condlog(0, "error formatting uid callout command");
> >> memset(pp->wwid, 0, WWID_SIZE);
> >> } else if (execute_program(buff, pp->wwid, WWID_SIZE)) {
> >> - condlog(0, "error calling out %s", buff);
> >> + condlog(3, "error calling out %s", buff);
> >> memset(pp->wwid, 0, WWID_SIZE);
> >> return 1;
> >> }
> >
> > ditto.
> >
> Cheers,

OK. When the primary messages above are printed correctly,
this secondary message can be suppressed by default.

Thanks,
Kiyoshi Ueda

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel