[v1] public/io: xs_wire: Allow Xenstore to report EPERM

[PATCH] public/io: xs_wire: Allow Xenstore to report EPERM

Posted by Julien Grall 1 year, 10 months ago

From: Julien Grall <jgrall@amazon.com>

C Xenstored is using EPERM when the client is not allowed to change
the owner (see GET_PERMS). However, the xenstore protocol doesn't
describe EPERM so EINVAL will be sent to the client.

When writing test, it would be useful to differentiate between EINVAL
(e.g. parsing error) and EPERM (i.e. no permission). So extend
xsd_errors[] to support return EPERM.

Looking at previous time xsd_errors was extended (8b2c441a1b), it was
considered to be safe to add a new error because at least Linux driver
and libxenstore treat an unknown error code as EINVAL.

This statement doesn't cover other possible OSes, however I am not
aware of any breakage.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/public/io/xs_wire.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
index c1ec7c73e3b1..c23b63cdfeaf 100644
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
 __attribute__((unused))
 #endif
     = {
+    XSD_ERROR(EPERM),
     XSD_ERROR(EINVAL),
     XSD_ERROR(EACCES),
     XSD_ERROR(EEXIST),
-- 
2.32.0

Re: [PATCH] public/io: xs_wire: Allow Xenstore to report EPERM

Posted by Jan Beulich 1 year, 10 months ago

On 24.06.2022 18:51, Julien Grall wrote:
> --- a/xen/include/public/io/xs_wire.h
> +++ b/xen/include/public/io/xs_wire.h
> @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
>  __attribute__((unused))
>  #endif
>      = {
> +    XSD_ERROR(EPERM),
>      XSD_ERROR(EINVAL),
>      XSD_ERROR(EACCES),
>      XSD_ERROR(EEXIST),

Inserting ahead of EINVAL looks to break xenstored_core.c:send_error(),
which - legitimately or not - assumes EINVAL to be first.

Jan

Re: [PATCH] public/io: xs_wire: Allow Xenstore to report EPERM

Posted by Julien Grall 1 year, 10 months ago

Hi Jan,

On 27/06/2022 07:57, Jan Beulich wrote:
> On 24.06.2022 18:51, Julien Grall wrote:
>> --- a/xen/include/public/io/xs_wire.h
>> +++ b/xen/include/public/io/xs_wire.h
>> @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
>>   __attribute__((unused))
>>   #endif
>>       = {
>> +    XSD_ERROR(EPERM),
>>       XSD_ERROR(EINVAL),
>>       XSD_ERROR(EACCES),
>>       XSD_ERROR(EEXIST),
> 
> Inserting ahead of EINVAL looks to break xenstored_core.c:send_error(),

:(.

> which - legitimately or not - assumes EINVAL to be first.

I am not sure who else is relying on this. So I would consider this to 
be bake in the ABI. I think the minimum is to add a BUILD_BUG_ON() in 
send_error().

I will also move EPERM towards the end (I added first because EPERM is 1).

Cheers,

-- 
Julien Grall

Re: [PATCH] public/io: xs_wire: Allow Xenstore to report EPERM

Posted by Juergen Gross 1 year, 10 months ago

On 27.06.22 12:00, Julien Grall wrote:
> Hi Jan,
> 
> On 27/06/2022 07:57, Jan Beulich wrote:
>> On 24.06.2022 18:51, Julien Grall wrote:
>>> --- a/xen/include/public/io/xs_wire.h
>>> +++ b/xen/include/public/io/xs_wire.h
>>> @@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
>>>   __attribute__((unused))
>>>   #endif
>>>       = {
>>> +    XSD_ERROR(EPERM),
>>>       XSD_ERROR(EINVAL),
>>>       XSD_ERROR(EACCES),
>>>       XSD_ERROR(EEXIST),
>>
>> Inserting ahead of EINVAL looks to break xenstored_core.c:send_error(),
> 
> :(.
> 
>> which - legitimately or not - assumes EINVAL to be first.
> 
> I am not sure who else is relying on this. So I would consider this to be bake 
> in the ABI. I think the minimum is to add a BUILD_BUG_ON() in send_error().
> 
> I will also move EPERM towards the end (I added first because EPERM is 1).

I agree to both plans.


Juergen