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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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




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