On Fri, Dec 03, 2021 at 08:25:00AM +0000, Martin Wilck wrote:
> On Thu, 2021-11-18 at 16:47 -0600, Benjamin Marzinski wrote:
> > +int
> > +sysfs_get_ro (struct path *pp)
> > +{
> > + int ro;
> > + char buff[3]; /* Either "0\n\0" or "1\n\0" */
> > +
> > + if (!pp->udev)
> > + return -1;
> > +
> > + if (sysfs_attr_get_value(pp->udev, "ro", buff, sizeof(buff))
> > <= 0) {
> > + condlog(3, "%s: Cannot read ro attribute in sysfs",
> > pp->dev);
> > + return -1;
> > + }
> > +
> > + if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
>
> This is ok, but for self-evidently correct code in multipath-tools,
> it'd be better to read just 2 bytes and set buff[2] = '\0' explicitly.
> I haven't checked, but coverity might stumble on this.
Actually this is just the way sysfs_attr_get_value() works. You have to
provide a larger buffer than you will read (in this case, ro will only
ever have two bytes), otherwise it will think that it overflowed.
sysfs_attr_get_value() also does the NULL termination itself.
-Ben
> > + condlog(3, "%s: Cannot parse ro attribute", pp->dev);
> > + return -1;
> > + }
> > +
> > + return ro;
> > +}
> > +
>
> Does this function need to be in libmultipath? We could avoid extending
> the ABI by adding it as a static function to multipathd. After all,
> it's just sysfs_attr_get_value() + sscanf().
>
>
> > int sysfs_check_holders(char * check_devt, char * new_devt)
> > {
> > unsigned int major, new_minor, table_minor;
> > diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
> > index 72b39ab2..c948c467 100644
> > --- a/libmultipath/sysfs.h
> > +++ b/libmultipath/sysfs.h
> > @@ -13,6 +13,7 @@ ssize_t sysfs_attr_get_value(struct udev_device
> > *dev, const char *attr_name,
> > ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char
> > *attr_name,
> > unsigned char * value, size_t
> > value_len);
> > int sysfs_get_size (struct path *pp, unsigned long long * size);
> > +int sysfs_get_ro(struct path *pp);
> > int sysfs_check_holders(char * check_devt, char * new_devt);
> > bool sysfs_is_multipathed(struct path *pp, bool set_wwid);
> > #endif
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 08a8a592..a1665494 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -1440,6 +1440,35 @@ finish_path_init(struct path *pp, struct
> > vectors * vecs)
> > return -1;
> > }
> >
> > +static bool
> > +needs_ro_update(struct multipath *mpp, int ro)
> > +{
> > + struct pathgroup * pgp;
> > + struct path * pp;
> > + unsigned int i, j;
> > + struct dm_info *dmi = NULL;
> > +
> > + if (!mpp || ro < 0)
> > + return false;
> > + dm_get_info(mpp->alias, &dmi);
>
> I'm still not convinced that we need this call, in particular as you
> have added a call to dm_get_info() in the uev_update_map() call, and it
> was called in the dmevent handler, too.
>
> In general, I think that multipath-tools could rely more on the kernel
> to send events for status changes than it currently does.
> This is particularly true for dm properties, where we have not only one
> event mechanism but two (uevent + dmevent).
>
> Regards
> Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
