Re: [PATCH] include/public: add command result definitions to vscsiif.h

[Juergen Gross <jgross@xxxxxxxx>]

On 14.03.22 10:55, Jan Beulich wrote:
> On 28.02.2022 12:22, Juergen Gross wrote:
> > --- a/xen/include/public/io/vscsiif.h
> > +++ b/xen/include/public/io/vscsiif.h
> > @@ -315,6 +315,33 @@ struct vscsiif_response {
> >   };
> >   typedef struct vscsiif_response vscsiif_response_t;
> >   
> > +/* SCSI I/O status from vscsiif_response->rslt */
> > +#define XEN_VSCSIIF_RSLT_STATUS(x)  (x & 0x00ff)
> No #define-s for individual values for this? I see the backend use
> e.g. SUCCESS and FAILED, wherever these come from ...
Oh, right, those are being used for the reset actions. Thanks for
spotting.

The "normal" request result values are defined at the SCSI layer.

> Also please parenthesize x here and ...
>
> > +/* Host I/O status from vscsiif_response->rslt */
> > +#define XEN_VSCSIIF_RSLT_HOST(x)    ((unsigned)x >> 16)
> ... here.
Okay.

> You cast to unsigned here, but rslt is a signed field. Is it really
> the entire upper 16 bits that are the host I/O status?
I thought I have seen it being used this way, but now I've found the
definition of "host_byte()" indicating it is only 8 bits wide. Will
change that.

> > +#define XEN_VSCSIIF_RSLT_HOST_OK         0
> > +#define XEN_VSCSIIF_RSLT_HOST_NO_CONN    1 /* Couldn't connect before timeout 
> > */
> > +#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY   2 /* BUS busy through timeout */
> > +#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT   3 /* TIMED OUT for other reason */
> > +#define XEN_VSCSIIF_RSLT_HOST_BAD_TARG   4 /* BAD target */
> Are the all-upper-case words really in need of mirroring this
> aspect from Linux? To me it gives the impression of this being
> acronyms of some sort at the first glance.
The backend can return all these values, so I think I need to define
them here.

> > +#define XEN_VSCSIIF_RSLT_HOST_ABORT      5 /* Abort for some other reason */
> > +#define XEN_VSCSIIF_RSLT_HOST_PARITY     6 /* Parity error */
> > +#define XEN_VSCSIIF_RSLT_HOST_ERROR      7 /* Internal error */
> > +#define XEN_VSCSIIF_RSLT_HOST_RESET      8 /* Reset by somebody */
> > +#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR   9 /* Unexpected interrupt */
> > +#define XEN_VSCSIIF_RSLT_HOST_PASSTHR   10 /* Force command past mid-layer */
> > +#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERR  11 /* Retry requested */
> > +#define XEN_VSCSIIF_RSLT_HOST_IMM_RETR  12 /* Hidden retry requested */
> > +#define XEN_VSCSIIF_RSLT_HOST_REQUEUE   13 /* Requeue command requested */
> > +#define XEN_VSCSIIF_RSLT_HOST_DISRUPT   14 /* Transport error disrupted I/O */
> > +#define XEN_VSCSIIF_RSLT_HOST_FAILFAST  15 /* Transport class fastfailed */
> > +#define XEN_VSCSIIF_RSLT_HOST_TARG_FAIL 16 /* Permanent target failure */
> > +#define XEN_VSCSIIF_RSLT_HOST_NEX_FAIL  17 /* Permanent nexus failure on path 
> > */
> > +#define XEN_VSCSIIF_RSLT_HOST_NOMEM     18 /* Space allocation failed */
> > +#define XEN_VSCSIIF_RSLT_HOST_MED_ERR   19 /* Medium error */
> > +#define XEN_VSCSIIF_RSLT_HOST_MARGINAL  20 /* Transport marginal errors */
> Some of the name shortening that you did, comparing with the
> Linux names, has gone a little too far for my taste. But you're
> the maintainer ...
There are basically the following alternatives:

- use longer names (using the Linux names would end up in e.g.
  XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED, making it 10 chars longer
- drop some part of the common prefix, e.g. the "RSLT_HOST_" part
- keep it as is

I'm basically fine with any of those.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature