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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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