Re: [PATCH v2 19/49] libmultipath: add libmp_mapinfo()

[Benjamin Marzinski <bmarzins@redhat.com>]

On Tue, Jul 16, 2024 at 05:51:21PM +0200, Martin Wilck wrote:
> On Mon, 2024-07-15 at 18:41 -0400, Benjamin Marzinski wrote:
> > On Fri, Jul 12, 2024 at 07:14:27PM +0200, Martin Wilck wrote:

> > I wonder if we should wrap ibmp_mapinfo in a macro that grabs the
> > function name of the calling function and passes it down to
> > libmp_mapinfo__() otherwise there will be a lot of messages like this
> > or
> > 
> > condlog(2, "%s: map %s doesn't exist", __func__, map_id);
> > 
> > that use "libmp_mapinfo__" as the function, which isn't super useful.
> 
> Right. I think what I'll do is just hardcode "libmp_mapinfo" in these
> messages instead of using __func__.

Actually, I was thinking more of something like:

int do_libmp_mapinfo(int flags, mapid_t id, mapinfo_t info,
		     const char *func)
{
        char idbuf[BLK_DEV_SIZE];

        return libmp_mapinfo__(flags, id, info,
                               libmp_map_identifier(flags, id, idbuf));
}

#define libmp_mapinfo(flags, id, info) \
do_libmp_mapinfo(flags, id, info, __func__)

So we would retain the caller info, but I don't feel that strongly about
it.

> 
> > 
> > > +			condlog(2, "%s: map %s not found",
> > > __func__, map_id);
> > > +			return DMP_NOT_FOUND;
> > > +		} else
> > > +			return DMP_ERR;
> > > +	}
> > > +


> > 
> > I mentioned tihs before, but the DM_MAP_BY_* identifiers only take up
> > 3
> > bytes so __DM_MAP_BY_MASK can just be 3 or (1 << 2) - 1, unless you
> > are
> > reserving space for more identifiers which is fine but probably
> > unnecessary, since libmultipath isn't a stable API. Or is there some
> > other reason I'm missing.
> 
> Is it important that this bit field is "dense"? Indeed I wanted to 
> leave some space, even though your're right that it most probably won't
> be necessary. I thought it might be helpful during debugging, for
> example.
> 
> Perhaps I should have used the entire low byte for the map_by part.
> That might even provide optimization opportunities for the compiler.
> Unless you object, this is what I'll do.

That's fine. Like I said, it's not a stable API. If it turns out we
don't like it later, we can always change it then.

-Ben