Re: [Xen-devel] [PATCH v2] xen: Implement hypercall for tracing of program counters

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/07/17 12:58, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Ian
>> Jackson
>> Sent: 31 July 2017 12:14
>> To: Wei Liu <wei.liu2@xxxxxxxxxx>
>> Cc: sstabellini@xxxxxxxxxx; Felix Schmoll <eggi.innovations@xxxxxxxxx>;
>> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Julien Grall
>> <julien.grall@xxxxxxx>; jbeulich@xxxxxxxx; xen-
>> devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [Xen-devel] [PATCH v2] xen: Implement hypercall for tracing of
>> program counters
>>
>> Wei Liu writes ("Re: [Xen-devel] [PATCH v2] xen: Implement hypercall for
>> tracing of program counters"):
>>> On Mon, Jul 31, 2017 at 09:22:35AM +0100, Julien Grall wrote:
>> ..
>>>> Should not it be -EOPNOTSUPP to match return error when
>> CONFIG_TRACE_PC is
>>>> not?
>>> AIUI EOPNOTSUPP means "This is a valid operation but I am not configured
>>> to support it" while EINVAL means "This is an invalid value
>>> (operation)".
>> EOPNOTSUPP means "someone somewhere might think this is valid, but I
>> don't understand it".  It can be used, for example, for unknown
>> operation numbers.
>>
>> "ENOSYS" is used in exactly the same situation, but where the enum
>> whose value is not understood is precisely the syscall number.  In the
>> context of the hypervisor I think ENOSYS is used for "unknown
>> hypercall number".  I haven't checked whether it is used for "unknown
>> operation number" but I suspect that the hypervisor users EOPNOTSUPP
>> for that.
> Nope. It's ENOSYS for that case too (certainly for hvm and memory ops... 
> which is what I checked).

History has been poor to us.  Quite a few of the ENOSYS should be
EOPNOTSUPP, but we can't change them for ABI reasons.

>
>   Paul
>
>> It would be sensible if hypervisor maintainers were to write this
>> stuff down somewhere.
>>
>> EINVAL means "I definitely know that this is invalid".  It should
>> rarely be used for an unknown value of an enum, since enums can gain
>> new values in future implementations.  It can be used for "value out
>> of range" or "I understand this combination of parameters, but it is
>> not meaningful".  It should not be used for "I understand this
>> combination of parameters, and I do not implement it, even though in
>> principle the combination might somehow be implemented in the future".
>>
>> In this particular case I suspect that EOPNOTSUPP is right.  EINVAL is
>> clearly wrong.
>>
>> While looking at the original patch, I saw this:
>>
>>> +    if ( !d )
>>> +        return -EINVAL; /* invalid domain */
>> Is this conventional ?  EINVAL is a remarkably unhelpful error code
>> for this case.  IMO the hypervisor ought to have a dedicated error
>> code for "referenced domain does not exist".  But maybe it doesn't.

ESRCH is "no such domain".  We also use ENOENT for "no such vcpu".

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel