On Thu, 2024-07-11 at 14:25 +0200, Martin Wilck wrote:
> On Wed, 2024-07-10 at 19:34 -0400, Benjamin Marzinski wrote:
> > On Tue, Jul 09, 2024 at 11:39:14PM +0200, Martin Wilck wrote:
> > >
> > >
> > > +/**
> > > + * dm_get_wwid(): return WWID for a multipath map
> > > + * @returns:
> > > + * DMP_OK if successful
> > > + * DMP_NOT_FOUND if the map doesn't exist
> > > + * DMP_NO_MATCH if the map exists but is not a multipath map
> > > + * DMP_ERR for other errors
> > > + */
> > > int dm_get_wwid(const char *name, char *uuid, int uuid_len)
> > > {
> > > char tmp[DM_UUID_LEN];
> > > + int rc = dm_get_dm_uuid(name, tmp);
> > >
> > > - if (dm_get_dm_uuid(name, tmp) != DMP_OK)
> > > - return 1;
> > > + if (rc != DMP_OK)
> > > + return rc;
> > >
> > > if (!strncmp(tmp, UUID_PREFIX, UUID_PREFIX_LEN))
> > > strlcpy(uuid, tmp + UUID_PREFIX_LEN, uuid_len);
> > > - else
> > > + else {
> >
> > This seems a little inconsistent with the change in our returns.
> > Before, if we found a device but it wasn't multipath device,
> > dm_get_wwid() would exit with 0 but set uuid to an empty string.
> > Now
> > we
> > return DMP_NO_MATCH in this case but we still clear uuid. It seems
> > like
> > perhaps we should either set uuid to an empty string for all cases
> > except DM_OK or we should not touch it for all cases except DM_OK.
> > Alternatively, perhaps I'm over-thinking this.
>
> Right. In libmp_mapinfo(), I followed the idea that no output should
> be
> touched if any errors happened. Here, I wanted to preserve the
> behavior
> but didn't do it correctly.
>
> I tend to think that "don't touch output in error case" is the
> correct
> behavior, but we'd need to review the callers and make sure that they
> don't assume the WWID will be automatically 0-terminated.
>
> I'll do that.
Turns out that the callers that need changes because of the changed
semantics of dm_get_wwid() will be changed later on anyway, and the
respective modifications of the callers will be removed again. But I'll
do this anyway, to make the patch set consistent.
Martin
