Re: [Xen-devel] [PATCH] xenbus: Add support for xenbus backend in stub domain

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.01.12 at 18:42, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> On 01/13/2012 11:00 AM, Jan Beulich wrote:
>>>>> On 13.01.12 at 16:44, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
>>> On 01/13/2012 10:37 AM, Jan Beulich wrote:
>>>>>>> On 13.01.12 at 15:06, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
>>>>> On 01/13/2012 03:20 AM, Jan Beulich wrote:
>>>>>>>>> On 13.01.12 at 00:36, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
>>>>>>> --- a/include/xen/xenbus_dev.h
>>>>>>> +++ b/include/xen/xenbus_dev.h
>>>>>>> @@ -38,4 +38,18 @@
>>>>>>>  #define IOCTL_XENBUS_BACKEND_EVTCHN                    \
>>>>>>>         _IOC(_IOC_NONE, 'B', 0, 0)
>>>>>>>  
>>>>>>> +#define IOCTL_XENBUS_ALLOC                             \
>>>>>>> +       _IOC(_IOC_NONE, 'B', 1, sizeof(struct ioctl_xenbus_alloc))
>>>>>>> +struct ioctl_xenbus_alloc {
>>>>>>> +       /* IN */
>>>>>>> +       /* The domain ID (must exist) for xenstore */
>>>>>>> +       uint16_t dom;
>>>>>>> +       uint16_t pad;
>>>>>>> +       /* OUT */
>>>>>>> +       /* The port allocated for xenbus communication */
>>>>>>> +       uint32_t port;
>>>>>>> +       /* Always set to GNTTAB_RESERVED_XENSTORE */
>>>>>>> +       uint32_t grant_ref;
>>>>>>> +};
>>>>>>
>>>>>> As said in my reply to the previous patch version - if the functionality
>>>>>> differs, the number *and* name should be different from the legacy
>>>>>> implementation's. Otherwise, how should compatible user space code
>>>>>> be written?
>>>>>>
>>>>>> Jan
>>>>>>
>>>>>
>>>>> This implementation has the same functionality as the legacy ioctl,
>>>>
>>>> It doesn't - none of the "OUT" fields above exist there.
>>>
>>> Are you looking at the same legacy ioctl as I am? I found it in 2.6.18.hg
>>> where the structure was defined as:
>>>
>>> typedef struct xenbus_alloc {
>>>     domid_t dom;
>>>     __u32 port;
>>>     __u32 grant_ref;
>>> } xenbus_alloc_t;
>>>
>>> The port and grant_ref fields were outputs. I added padding for clarity
>>> since domid_t is uint16_t.
>> 
>> Ooops, I'm sorry, indeed - I didn't look at the names and types
>> closely enough. They were too different, so I assumed the whole
>> thing is different.
>> 
>> That doesn't eliminate the difference between the two
>> IOCTL_XENBUS_ALLOC definitions, though
>> [_IOC(_IOC_NONE, 'X', 0, sizeof(xenbus_alloc_t)) vs
>> _IOC(_IOC_NONE, 'B', 1, sizeof(struct ioctl_xenbus_alloc))] - one
>> still can't use both in the same source file.
> 
> Is compatibility with 2.6.18-xen more important than keeping the ioctl
> numbers in a single file consistent? The definition in 2.6.18 is also
> incorrect (as was my definition) - since the argument is used, it needs
> to use _IOWR() or _IOC_READ|_IOC_WRITE instead of _IOC_NONE.

Hmm, that would mean that apart from IOCTL_EVTCHN_RESET *all*
current ioctl number definitions under include/xen/ are wrong. Very
bad, and ugly to deal with. With generic ioctl handling code not
depending on this, I wonder though whether it's worth fixing at all.
Are you aware of any user mode code depending on the correctness
of these bits?

Which of course isn't to say that it shouldn't be done right for new
ones...

> Since this ioctl must already be performed a different file than the
> legacy ioctl, a new name seems to be the best solution.

That's what I was trying to ask you to do.

Jan


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