Re: [Xen-devel] Returning errno values inside of hypercall structs (was: Re: [PATCH for-4.7 3/4] tools/xsplice: fix mixing system)

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Apr 29, 2016 at 12:22:32PM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Apr 29, 2016 at 06:16:19PM +0200, Roger Pau Monne wrote:
> > On Fri, Apr 29, 2016 at 04:30:16PM +0100, Wei Liu wrote:
> > > On Fri, Apr 29, 2016 at 05:12:51PM +0200, Roger Pau Monne wrote:
> > > > On Fri, Apr 29, 2016 at 04:02:33PM +0100, Wei Liu wrote:
> > > > > I have a gut feeling that returning XEN_ errno to userspace program is
> > > > > layering violation. They should always be translated to OS level errno
> > > > > by privcmd driver.
> > > > 
> > > > Yes, the error value returned from the hypercall executed is indeed 
> > > > translated into the native OS error space. The problem here is that 
> > > > those 
> > > > error codes are returned _inside_ of the specific hypercall struct, 
> > > > which 
> > > > sadly privcmd doesn't know anything about.
> > > > 
> > > > And of course teaching privcmd about every possible hypercall struct is 
> > > > simply impossible, since some of them are not stable (eg: domctls)
> > > >  
> > > > > Aren't FreeBSD and NetBSD already doing that?
> > > > 
> > > > As said above, this is only done for direct return codes, everything 
> > > > inside 
> > > > of the struct passed to the hypercall is returned as-is.
> > > > 
> > > > This is a complete mess, and TBH, I don't have a clever idea about how 
> > > > to 
> > > > solve it.
> > > > 
> > > 
> > > Me neither. Maybe a new thread should be started to discuss this.
> > 
> > So here we are.
> > 
> > In order to put everyone into context: the issue here is that some 
> > hypercalls (those that batch several operations) return an array of error 
> > codes inside of the hypercall structure. This array of error codes is not 
> > standardized, so the privcmd driver doesn't know anything about it, and 
> > thus 
> > cannot translate it into the native OS error space.
> > 
> > It has also been suggested that the privcmd driver simply doesn't translate 
> > error codes at all, and then let the applications figure out if the error 
> > code comes from Xen or from the OS. IMHO, this is impossible to achieve, 
> > because the ioctl syscall can return an error code that's been forwarded 
> > by Xen or a native one, and the application has no way of knowing where is 
> > it coming from.
> > 
> > I've identified at least the following hypercall structs that store XEN_* 
> > error codes inside:
> > 
> >  - xen_add_to_physmap_batch
> >  - xen_xsplice_status
> > 
> > TBH, it's quite hard to spot them, so I might have missed some. 
> > 
> > xen_add_to_physmap_batch is part of the public ABI, and cannot be changed. 
> > On the bright side, xen_add_to_physmap_batch is implemented as a different 
> > ioctl in privcmd usually (in order to map memory from other domains), so 
> > the 
> > error translation should be handled correctly.
> > 
> > Then the xsplice struct that uses XEN_* values is:
> > 
> > struct xen_xsplice_status {
> > #define XSPLICE_STATE_CHECKED      1
> > #define XSPLICE_STATE_APPLIED      2
> >     uint32_t state;                /* OUT: XSPLICE_STATE_*. */
> >     int32_t rc;                    /* OUT: 0 if no error, otherwise 
> > -XEN_EXX. */
> > };
> > 
> > Which is in turn used by:
> > 
> > struct xen_sysctl_xsplice_list {
> >     uint32_t version;                       /* OUT: Hypervisor stamps value.
> >                                                If varies between calls, we 
> > are
> >                                              * getting stale data. */
> >     uint32_t idx;                           /* IN: Index into hypervisor 
> > list. */
> >     uint32_t nr;                            /* IN: How many status, name, 
> > and len
> >                                                should fill out. Can be zero 
> > to get
> >                                                amount of payloads and 
> > version.
> >                                                OUT: How many payloads left. 
> > */
> >     uint32_t pad;                           /* IN: Must be zero. */
> >     XEN_GUEST_HANDLE_64(xen_xsplice_status_t) status;  /* OUT. Must have 
> > enough
> >                                                space allocate for nr of 
> > them. */
> >     XEN_GUEST_HANDLE_64(char) name;         /* OUT: Array of names. Each 
> > member
> >                                                MUST XEN_XSPLICE_NAME_SIZE 
> > in size.
> >                                                Must have nr of them. */
> >     XEN_GUEST_HANDLE_64(uint32) len;        /* OUT: Array of lengths of 
> > name's.
> >                                                Must have nr of them. */
> > };
> > 
> > IMHO, the best way to solve this is to define a set of XSPLICE_ERROR_* that 
> > covers the error codes returned by xsplice, and use that instead of XEN_* 
> > errno values. This would make it much more easier to avoid mistakes when 
> > coding the toolstack part of xsplice.
> 
> But why?
> 
> I must be missing something here - but the return from the hypercall
> can return say 0 but the status->rc can be -XEN_EAGAIN.
> 
> Why does it need to be XSPLICE_ERROR_?

Because nobody uses or enforces the correct usage of XEN_E* in the tools, so 
people just use native error codes, which works on Linux, but breaks on 
other OSes.

Using something like XSPLICE_ERROR_* prevents people from having the bad 
habit of directly using native OS error codes, by making it more obvious 
that the error code is in a different space.

Roger.

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