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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 01/13/2012 11:00 AM, Jan Beulich wrote:
>>>> On 13.01.12 at 16:44, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> On 01/13/2012 10:37 AM, Jan Beulich wrote:
>>>>>> On 13.01.12 at 15:06, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>>>> On 01/13/2012 03:20 AM, Jan Beulich wrote:
>>>>>>>> On 13.01.12 at 00:36, Daniel De Graaf <dgdegra@tycho.nsa.gov> 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.

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

> Additionally (forgive if I'm not up to date in this respect) - is it okay
> these days to use uintX_t in exported Linux headers, and then
> without including the header that would define the necessary
> types?
> 
> Jan

It's done in a number of other header files (I was copying from gntdev.h)
but that doesn't mean it's officially okay. Using __u32 is probably safer.

> 
>>>> although it has a different number and is performed on a different file,
>>>> so it is already impossible to make automatically compatible userspace
>>>> code.
>>>
>>> That's not what I was aiming at. The problem you're introducing is
>>> that you name the ioctl IOCTL_XENBUS_ALLOC, identical to what
>>> the legacy code named it. Hence one won't be able to write user
>>> mode code to invoke, depending on runtime determination, either
>>> the old or the new function.
>>>
>>> Jan
>>>
>>>> The structure name was changed to match other ioctl arguments, but
>>>> the contents should be the same as in the legacy ioctl. If changing what
>>>> file the ioctl is performed on justifies changing the ioctl name, then I
>>>> would prefer the simpler interface where the domain ID is passed in as the
>>>> ioctl parameter instead of a structure that only has one useful output.
>>>