Re: [Xen-devel] [PATCH] add trim command to blkback interface

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 06.12.10 at 12:28, Owen Smith <owen.smith@xxxxxxxxxx> wrote:
> @@ -56,13 +67,23 @@ struct blkif_request {
>                uint8_t        nr_segments;  /* number of segments            
>        */
>                blkif_vdev_t   handle;       /* only for read/write requests  
>        */
>                uint64_t       id;           /* private guest value, echoed in 
> resp  */
> -              blkif_sector_t sector_number;/* start sector idx on disk (r/w 
> only)  */
> -              struct blkif_request_segment {
> -                              grant_ref_t gref;        /* reference to I/O 
> buffer frame        */
> -                              /* @first_sect: first sector in frame to 
> transfer (inclusive).   */
> -                              /* @last_sect: last sector in frame to 
> transfer (inclusive).     */
> -                              uint8_t     first_sect, last_sect;
> -              } seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> +
> +             union {
> +                             struct blkif_request_rw {
> +                                             blkif_sector_t sector_number;/* 
> start sector idx on disk (r/w only)  */
> +                                             struct blkif_request_segment {
> +                                                             grant_ref_t 
> gref;        /* reference to I/O buffer frame        */
> +                                                             /* @first_sect: 
> first sector in frame to transfer (inclusive).   */
> +                                                             /* @last_sect: 
> last sector in frame to transfer (inclusive).     */
> +                                                             uint8_t     
> first_sect, last_sect;
> +                                             } 
> seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> +                             } rw;
> +
> +                             struct blkif_request_trim {
> +                             blkif_sector_t sector_number;
> +                                     uint64_t       nr_sectors;
> +                             } trim;
> +             };

Wouldn't the whole patch be quite a bit smaller if you kept
sector_number outside the union? If using anonymous
structs/unions is okay here (which I don't think it is), there
would also not have been a need to name the struct
blkif_request_rw instance, thus eliminating the need to
touch code just to add the new intermediate field name.

Isn't the whole patch also incomplete as it doesn't touch
blkfront at all (and hence will presumably cause build
errors)?

Finally, shouldn't the patch be split (or at least accompanied
by a second patch) to modify the master io/blkif.h (in
-unstable) rather than the (edited) copy in the pv-ops Linux
tree only?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel