On Tue, 2025-11-11 at 12:59 +0100, Martin Wilck wrote:
> On Mon, 2025-11-10 at 16:51 -0500, Benjamin Marzinski wrote:
> > mpathpersist was incorrectly parsing the REPORT CAPABILITES service
> > action output. In reality, the type mask is two bytes where the
> > type
> > information is stored in bits 7, 6, 5, 3, & 1 (0xea) of the first
> > byte
> > and bit 0 (0x01) of the second byte. libmpathpersist was treating
> > these
> > two bytes as a big endian 16 bit number, but mpathpersist was
> > looking
> > for bits in that number as if it was little endian number.
> >
> > Ideally, libmpathpersist would treat prin_capdescr.pr_type_mask as
> > two bytes, like it does for the flags. But we already expose this
> > as a 16 bit number, where we treated the input bytes as a big
> > endian
> > number. There's no great reason to mess with the libmpathpersist
> > API,
> > when we can just make mpathpersist treat this data like
> > libmpathpersist
> > provides it. So, fix mpathpersist to print the data out correctly.
> >
> > Also, instead of printing a 1 or a 0, it was printing the value of
> > the
> > flag. Fix that too.
> >
> > Signed-off-by: Benjamin Marzinski <[email protected]>
>
> LGTM, thanks.
>
> The way libmpathpersist treats this field is totally counter-
> intuitive,
> but it has always been like that, and I agree we shouldn't change the
> API now. However, it is not unlikely that other users of this API (if
> any) also get the byte order wrong for pr_type_mask. Perhaps we
> should
> add a comment in mpath_persist.h. Could you add that?
>
> Also, while staring at the code in mpath_print_buf_readcap(), I
> noticed
> that the output for PTPL_C is also wrong (missing "& 0x01"). While at
> it, could you perhaps fix that, too?
>
> Regards,
> Martin
Please also add
Fixes: ae4e8a6 ("mpathpersist: Add new utility for managing persistent
reservation on dm multipath device")
