Re: [Xen-devel] [PATCH] Update pvSCSI protocol description

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.08.14 at 07:05, <"jgross@xxxxxxxx".non-mime.internet> wrote:
> Update the protocol description of the pvSCSI framework used to pass through
> SCSI devices to a guest (pv or hvm).
> 
> The main changes are:
> - added comments
> - adapt to Linux style guide

This is a Xen header, so no, Linux style isn't what we want here (it
is going to remain an exercise of changing the style when carrying
changes to this header over to the individual consuming OSes).

> - add support for larger SG-lists by putting them in an own granted page
> - remove stale definitions

With the significant amount of formatting changes it doesn't really
become clear which ones these are. If, after reverting the Linux
style changes, this still ends up being hard to spot, please enumerate
them here.

> @@ -30,11 +33,144 @@
>  #include "ring.h"
>  #include "../grant_table.h"
>  
> -/* commands between backend and frontend */
> -#define VSCSIIF_ACT_SCSI_CDB         1    /* SCSI CDB command */
> -#define VSCSIIF_ACT_SCSI_ABORT       2    /* SCSI Device(Lun) Abort*/
> -#define VSCSIIF_ACT_SCSI_RESET       3    /* SCSI Device(Lun) Reset*/
> -#define VSCSIIF_ACT_SCSI_SG_PRESET   4    /* Preset SG elements */
> +/*
> + * Front->back notifications: When enqueuing a new request, sending a
> + * notification can be made conditional on req_event (i.e., the generic
> + * hold-off mechanism provided by the ring macros). Backends must set
> + * req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()).
> + *
> + * Back->front notifications: When enqueuing a new response, sending a
> + * notification can be made conditional on rsp_event (i.e., the generic
> + * hold-off mechanism provided by the ring macros). Frontends must set
> + * rsp_event appropriately (e.g., using RING_FINAL_CHECK_FOR_RESPONSES()).
> + */

I don't think this belongs here; a referral to ring.h would be the most
to be put here (but I think the mere fact that this header includes
ring.h is enough of a reference).

> +/*
> + * Feature and Parameter Negotiation
> + * =================================
> + * The two halves of a Xen pvSCSI driver utilize nodes within the XenStore to
> + * communicate capabilities and to negotiate operating parameters.  This
> + * section enumerates these nodes which reside in the respective front and
> + * backend portions of the XenStore, following the XenBus convention.
> + *
> + * All data in the XenStore is stored as strings.  Nodes specifying numeric
> + * values are encoded in decimal.  Integer value ranges listed below are
> + * expressed as fixed sized integer types capable of storing the conversion
> + * of a properly formated node string, without loss of information.

Mostly the same very likely goes for this paragraph.

> + * feature-sg-grant
> + *      Values:         <uint16_t>

Since when can XenStore encode fixed-width values? Same further
down - they're all plain (perhaps unsigned) integer values at this
layer.

> -
> -#endif  /*__XEN__PUBLIC_IO_SCSI_H__*/
> -/*
> - * Local variables:
> - * mode: C
> - * c-file-style: "BSD"
> - * c-basic-offset: 4
> - * tab-width: 4
> - * indent-tabs-mode: nil
> - * End:
> - */
> +#endif /*__XEN__PUBLIC_IO_SCSI_H__*/

These markers shouldn't get removed either.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel