Re: [Xen-devel] [PATCH for 4.7] pvusb: add missing definition to usbif.h

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, May 06, 2016 at 01:56:36AM -0600, Jan Beulich wrote:
> >>> On 06.05.16 at 09:49, <JBeulich@xxxxxxxx> wrote:
> >>>> On 06.05.16 at 07:01, <JGross@xxxxxxxx> wrote:
> >> On 05/05/16 11:22, Wei Liu wrote:
> >>> On Thu, May 05, 2016 at 11:10:33AM +0200, Juergen Gross wrote:
> >>>> On 05/05/16 11:02, Wei Liu wrote:
> >>>>> On Thu, May 05, 2016 at 08:36:45AM +0200, Juergen Gross wrote:
> >>>>>> The pvusb request structure contains the transfer_flags member which
> >>>>>> is missing definitions of it's semantics.
> >>>>>>
> >>>>>> Add the definition of the USBIF_SHORT_NOT_OK flag.
> >>>>>>
> >>>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> >>>>>> ---
> >>>>>> Please consider taking this patch for 4.7 release. I believe this is 
> >>>>>> the
> >>>>>> last bit missing for support of qemu based pvusb backend. The risk of 
> >>>>>> the
> >>>>>> patch should be zero, as no Xen component is using this header.
> >>>>>> ---
> >>>>>>  xen/include/public/io/usbif.h | 1 +
> >>>>>>  1 file changed, 1 insertion(+)
> >>>>>>
> >>>>>> diff --git a/xen/include/public/io/usbif.h 
> >>>>>> b/xen/include/public/io/usbif.h
> >>>>>> index 9ef0cdc..4053c24 100644
> >>>>>> --- a/xen/include/public/io/usbif.h
> >>>>>> +++ b/xen/include/public/io/usbif.h
> >>>>>> @@ -187,6 +187,7 @@ struct usbif_urb_request {
> >>>>>>        /* basic urb parameter */
> >>>>>>        uint32_t pipe;
> >>>>>>        uint16_t transfer_flags;
> >>>>>> +#define USBIF_SHORT_NOT_OK    0x0001
> >>>>>
> >>>>> Where does this come from? Should it be surrounded by define guard?
> >>>>
> >>>> I just wasn't defined up to now (to be precise: transfer_flags was just
> >>>> copied from the related URB struct member in the frontend, so the
> >>>> interface was based on some Linux kernel internals, and the qemu backend
> >>>> used a literal "1" for testing the flag).
> >>>>
> >>>>> #ifndef USBIF_SHORT_NOT_OK
> >>>>> #define USBIF_SHORT_NOT_OK 0x0001
> >>>>> #endif
> >>>>>
> >>>>> Why does it need to be in our public header? If we end up taking this
> >>>>> I think it should at least start with XEN_ prefix.
> >>>>
> >>>> This is just a part of the pvusb interface. So it should be defined in
> >>>> the appropriate header file.
> >>>>
> >>> 
> >>> OK. I get it now.
> >>> 
> >>>> Regarding prefix: I can do this, but in this case I'd prefer to add the
> >>>> prefix to all definitions in the header. As there are currently no
> >>>> in-tree users of this header, the risk would still be zero. :-)
> >>>>
> >>>> Thoughts?
> >>>>
> >>> 
> >>> Actually not all public #define are prefixed by XEN_ (netif.h does,
> >>> blkif.h doesn't) so I won't insists on this. But I still using XEN_
> >>> prefix is better.
> >> 
> >> Sure. But I think it should be consistent at header file level. So in
> >> my opinion the question is: should I change all definitions in usbif.h
> >> to use the XEN_ prefix or should I add the new definition without
> >> prefix?
> > 
> > Since changing them all is not even an option (breaking possible
> > existing users, even if we don't know of any, is not allowed), I
> > think leaving the XEN_ off of the new addition here is acceptable
> > (as being more consistent inside the header, as you validly say). So
> > since Wei already said he won't insist on the prefix, I think this can
> > go in as is.
> 
> Of course only if Wei would release-ack it...
> 
> Jan
> 

Release-acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>

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