[dm-devel] [RFC PATCH v2 3/3] libmultipath: change log level for null uid_attribute

[Martin Wilck]

On Thu, 2020-09-24 at 11:20 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 24, 2020 at 08:06:41AM +0000, Martin Wilck wrote:
> > On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote:
> > > 
> > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > > index f650534f..435f2639 100644
> > > --- a/libmultipath/discovery.c
> > > +++ b/libmultipath/discovery.c
> > > @@ -2091,7 +2091,8 @@ get_uid (struct path * pp, int path_state,
> > > struct udev_device *udev,
> > >  		}
> > >  		if ((!udev_available || (len <= 0 && allow_fallback))
> > >  		    && has_uid_fallback(pp)) {
> > > -			used_fallback = 1;
> > > +			if (udev_available || !(udev && pp-
> > > > uid_attribute))
> > > +				used_fallback = 1;
> > >  			len = uid_fallback(pp, path_state, &origin);
> > >  		}
> > >  	}
> > 
> > Uff, this logic was convoluted anyway, now it's even harder to
> > grasp.
> > IIUC, if !udev, used_fallback will be set to 1, even if 
> > pp->uid_attribute is the empty string. Is that intended?
> > To make this easier to read, I'd suggest something like this:
> 
> My though was that not having a udev device is an unexpected
> situation,
> and so we should continue to log the message at an increased logging
> level.  If you're against that, it's not that important. I can
> certainly
> go with your code logic, or "if (!udev || !empty_uid_attr)" if you
> think
> it's reasonable to log at an increased level whenever the udev_device
> is
> NULL, even if the device was configured to ignore the udev database

My main concern was not the !udev case, nor the logging. It was just
that my brain choked on the complex boolean expression. My suggestion
was intended to make it somewhat easier to grok, that's all.

Cheers,
Martin