diff mbox series

public/io: xs_wire: Allow Xenstore to report EPERM

Message ID 20220624165151.940-1-julien@xen.org (mailing list archive)
State Superseded
Headers show
Series public/io: xs_wire: Allow Xenstore to report EPERM | expand

Commit Message

Julien Grall June 24, 2022, 4:51 p.m. UTC 
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(+)

Comments

Jan Beulich June 27, 2022, 6:57 a.m. UTC | #1 
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
Julien Grall June 27, 2022, 10 a.m. UTC | #2 
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,
Jürgen Groß June 27, 2022, 10:06 a.m. UTC | #3 
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
diff mbox series

Patch

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),