xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for 4.7] pvusb: add missing definition to usbif.h
@ 2016-05-05  6:36 Juergen Gross
  2016-05-05  9:02 ` Wei Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Juergen Gross @ 2016-05-05  6:36 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, sstabellini, wei.liu2, George.Dunlap,
	andrew.cooper3, ian.jackson, jbeulich

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@suse.com>
---
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
 	uint16_t buffer_length;
 	union {
 		uint8_t ctrl[8]; /* setup_packet (Ctrl) */
-- 
2.6.6


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH for 4.7] pvusb: add missing definition to usbif.h
  2016-05-05  6:36 [PATCH for 4.7] pvusb: add missing definition to usbif.h Juergen Gross
@ 2016-05-05  9:02 ` Wei Liu
  2016-05-05  9:10   ` Juergen Gross
  0 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2016-05-05  9:02 UTC (permalink / raw)
  To: Juergen Gross
  Cc: sstabellini, wei.liu2, George.Dunlap, andrew.cooper3,
	ian.jackson, xen-devel, jbeulich

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@suse.com>
> ---
> 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?

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

>  	uint16_t buffer_length;
>  	union {
>  		uint8_t ctrl[8]; /* setup_packet (Ctrl) */
> -- 
> 2.6.6
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for 4.7] pvusb: add missing definition to usbif.h
  2016-05-05  9:02 ` Wei Liu
@ 2016-05-05  9:10   ` Juergen Gross
  2016-05-05  9:22     ` Wei Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Juergen Gross @ 2016-05-05  9:10 UTC (permalink / raw)
  To: Wei Liu
  Cc: sstabellini, George.Dunlap, andrew.cooper3, ian.jackson,
	xen-devel, jbeulich

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@suse.com>
>> ---
>> 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.

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?


Juergen


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for 4.7] pvusb: add missing definition to usbif.h
  2016-05-05  9:10   ` Juergen Gross
@ 2016-05-05  9:22     ` Wei Liu
  2016-05-06  5:01       ` Juergen Gross
  0 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2016-05-05  9:22 UTC (permalink / raw)
  To: Juergen Gross
  Cc: sstabellini, Wei Liu, George.Dunlap, andrew.cooper3, ian.jackson,
	xen-devel, jbeulich

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@suse.com>
> >> ---
> >> 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.

Wei.

> 
> Juergen
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for 4.7] pvusb: add missing definition to usbif.h
  2016-05-05  9:22     ` Wei Liu
@ 2016-05-06  5:01       ` Juergen Gross
  2016-05-06  7:49         ` Jan Beulich
  2016-05-06  9:48         ` Wei Liu
  0 siblings, 2 replies; 9+ messages in thread
From: Juergen Gross @ 2016-05-06  5:01 UTC (permalink / raw)
  To: Wei Liu
  Cc: sstabellini, George.Dunlap, andrew.cooper3, ian.jackson,
	xen-devel, jbeulich

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@suse.com>
>>>> ---
>>>> 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?


Juergen

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for 4.7] pvusb: add missing definition to usbif.h
  2016-05-06  5:01       ` Juergen Gross
@ 2016-05-06  7:49         ` Jan Beulich
  2016-05-06  7:56           ` Jan Beulich
  2016-05-06  9:48         ` Wei Liu
  1 sibling, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2016-05-06  7:49 UTC (permalink / raw)
  To: Wei Liu, Juergen Gross
  Cc: George.Dunlap, andrew.cooper3, sstabellini, ian.jackson, xen-devel

>>> On 06.05.16 at 07:01, <JGross@suse.com> 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@suse.com>
>>>>> ---
>>>>> 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@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for 4.7] pvusb: add missing definition to usbif.h
  2016-05-06  7:49         ` Jan Beulich
@ 2016-05-06  7:56           ` Jan Beulich
  2016-05-06  9:48             ` Wei Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2016-05-06  7:56 UTC (permalink / raw)
  To: Wei Liu, Juergen Gross
  Cc: George.Dunlap, andrew.cooper3, sstabellini, ian.jackson, xen-devel

>>> On 06.05.16 at 09:49, <JBeulich@suse.com> wrote:
>>>> On 06.05.16 at 07:01, <JGross@suse.com> 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@suse.com>
>>>>>> ---
>>>>>> 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


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for 4.7] pvusb: add missing definition to usbif.h
  2016-05-06  5:01       ` Juergen Gross
  2016-05-06  7:49         ` Jan Beulich
@ 2016-05-06  9:48         ` Wei Liu
  1 sibling, 0 replies; 9+ messages in thread
From: Wei Liu @ 2016-05-06  9:48 UTC (permalink / raw)
  To: Juergen Gross
  Cc: sstabellini, Wei Liu, George.Dunlap, andrew.cooper3, ian.jackson,
	xen-devel, jbeulich

On Fri, May 06, 2016 at 07:01:12AM +0200, Juergen Gross 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@suse.com>
> >>>> ---
> >>>> 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?
> 

Yes we need to be consistent in usbif.h

> 
> Juergen

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH for 4.7] pvusb: add missing definition to usbif.h
  2016-05-06  7:56           ` Jan Beulich
@ 2016-05-06  9:48             ` Wei Liu
  0 siblings, 0 replies; 9+ messages in thread
From: Wei Liu @ 2016-05-06  9:48 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Juergen Gross, sstabellini, Wei Liu, George.Dunlap,
	andrew.cooper3, ian.jackson, xen-devel

On Fri, May 06, 2016 at 01:56:36AM -0600, Jan Beulich wrote:
> >>> On 06.05.16 at 09:49, <JBeulich@suse.com> wrote:
> >>>> On 06.05.16 at 07:01, <JGross@suse.com> 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@suse.com>
> >>>>>> ---
> >>>>>> 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@citrix.com>

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-05-06  9:48 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-05  6:36 [PATCH for 4.7] pvusb: add missing definition to usbif.h Juergen Gross
2016-05-05  9:02 ` Wei Liu
2016-05-05  9:10   ` Juergen Gross
2016-05-05  9:22     ` Wei Liu
2016-05-06  5:01       ` Juergen Gross
2016-05-06  7:49         ` Jan Beulich
2016-05-06  7:56           ` Jan Beulich
2016-05-06  9:48             ` Wei Liu
2016-05-06  9:48         ` Wei Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).