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
> ---
> mpathpersist/main.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index 97fd5a43..5033ee8a 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
> @@ -761,12 +761,23 @@ void mpath_print_buf_readcap( struct prin_resp
> *pr_buff)
> {
> printf(" Support indicated in Type mask:\n");
>
> - printf(" %s: %d\n", pr_type_strs[7], pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask & 0x80);
> - printf(" %s: %d\n", pr_type_strs[6], pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask & 0x40);
> - printf(" %s: %d\n", pr_type_strs[5], pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask & 0x20);
> - printf(" %s: %d\n", pr_type_strs[3], pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask & 0x8);
> - printf(" %s: %d\n", pr_type_strs[1], pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask & 0x2);
> - printf(" %s: %d\n", pr_type_strs[8], pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask & 0x100);
> + printf(" %s: %d\n", pr_type_strs[7],
> + !!(pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask &
> + 0x8000));
> + printf(" %s: %d\n", pr_type_strs[6],
> + !!(pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask &
> + 0x4000));
> + printf(" %s: %d\n", pr_type_strs[5],
> + !!(pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask &
> + 0x2000));
> + printf(" %s: %d\n", pr_type_strs[3],
> + !!(pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask &
> + 0x800));
> + printf(" %s: %d\n", pr_type_strs[1],
> + !!(pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask &
> + 0x200));
> + printf(" %s: %d\n", pr_type_strs[8],
> + !!(pr_buff-
> >prin_descriptor.prin_readcap.pr_type_mask & 0x1));
> }
> }
>
