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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 ...

Also please parenthesize x here and ...

> +/* Host I/O status from vscsiif_response->rslt */
> +#define XEN_VSCSIIF_RSLT_HOST(x)    ((unsigned)x >> 16)

... here.

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?

> +#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.

> +#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 ...

Jan