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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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

  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.
> 
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel