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@xxxxxxx (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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
