| Message ID | 1600923569-17412-4-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | add library to check if device is a valid path | expand |
On Wed, 2020-09-23 at 23:59 -0500, Benjamin Marzinski wrote: > If uid_attribute is explicitly set to an empty string, multipath > should > log the uid at the default log level, since using the fallback code > is > the expected behavior. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmultipath/discovery.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > 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: bool valid_uid_attr = pp->uid_attribute && *pp->uid_attribute; bool empty_uid_attr = pp->uid_attribute && !*pp->uid_attribute; bool udev_available = udev && valid_uid_attr; ... if ((!udev_available || (len <= 0 && allow_fallback)) && has_uid_fallback(pp)) { if (!empty_uid_attr) used_fallback = 1; len = uid_fallback(pp, path_state, &origin); Regards, Martin
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: > > If uid_attribute is explicitly set to an empty string, multipath > > should > > log the uid at the default log level, since using the fallback code > > is > > the expected behavior. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > libmultipath/discovery.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > 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. -Ben > > bool valid_uid_attr = pp->uid_attribute > && *pp->uid_attribute; > bool empty_uid_attr = pp->uid_attribute > && !*pp->uid_attribute; > bool udev_available = udev && valid_uid_attr; > ... > if ((!udev_available || (len <= 0 && allow_fallback)) > && has_uid_fallback(pp)) { > if (!empty_uid_attr) > used_fallback = 1; > len = uid_fallback(pp, path_state, &origin); > > Regards, > Martin > > -- > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 > SUSE Software Solutions Germany GmbH > HRB 36809, AG Nürnberg GF: Felix > Imendörffer > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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); } }
If uid_attribute is explicitly set to an empty string, multipath should log the uid at the default log level, since using the fallback code is the expected behavior. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/discovery.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)