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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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_?

> 
> Roger.

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