| Message ID | 1461939680-32574-4-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote: > Avoid using system errno values when comparing with Xen errno values. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Ross Lagerwall <ross.lagerwall@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > --- > Using errno values inside of hypercall structs is not right IMHO, but there > are already several occurrences of this. Although I'm adding the correct XEN_ > prefixes here, it's very likely that new additions/modifications to this > file will not take this into account, breaking it for OSes != Linux. This seems to be a rather thorny issue. 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. Aren't FreeBSD and NetBSD already doing that? Wei. > --- > tools/misc/xen-xsplice.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/tools/misc/xen-xsplice.c b/tools/misc/xen-xsplice.c > index 695be7f..b31115c 100644 > --- a/tools/misc/xen-xsplice.c > +++ b/tools/misc/xen-xsplice.c > @@ -13,6 +13,8 @@ > #include <xenctrl.h> > #include <xenstore.h> > > +#include <xen/errno.h> > + > static xc_interface *xch; > > void show_help(void) > @@ -233,7 +235,7 @@ struct { > .function = xc_xsplice_revert, > }, > { .allow = XSPLICE_STATE_CHECKED, > - .expected = -ENOENT, > + .expected = -XEN_ENOENT, > .name = "unload", > .function = xc_xsplice_unload, > }, > @@ -276,7 +278,7 @@ int action_func(int argc, char *argv[], unsigned int idx) > name, errno, strerror(errno)); > return -1; > } > - if ( status.rc == -EAGAIN ) > + if ( status.rc == -XEN_EAGAIN ) > { > fprintf(stderr, "%s failed. Operation already in progress\n", name); > return -1; > @@ -319,7 +321,7 @@ int action_func(int argc, char *argv[], unsigned int idx) > > if ( status.state != original_state ) > break; > - if ( status.rc && status.rc != -EAGAIN ) > + if ( status.rc && status.rc != -XEN_EAGAIN ) > { > rc = status.rc; > break; > -- > 2.6.4 (Apple Git-63) >
On 29/04/16 16:02, Wei Liu wrote: > On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote: >> Avoid using system errno values when comparing with Xen errno values. >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> Cc: Ross Lagerwall <ross.lagerwall@citrix.com> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Wei Liu <wei.liu2@citrix.com> >> --- >> Using errno values inside of hypercall structs is not right IMHO, but there >> are already several occurrences of this. Although I'm adding the correct XEN_ >> prefixes here, it's very likely that new additions/modifications to this >> file will not take this into account, breaking it for OSes != Linux. > This seems to be a rather thorny issue. > > 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. > > Aren't FreeBSD and NetBSD already doing that? It is not practical for the privcmd driver to do this translation, as most hypercalls through the privcmd driver are toolstack calls, and have an unstable layout. Another thorny issue is that the ioctl() call for privcmd returns errors via errno, which may be in system errno space, or xen errno space depending on the source of the errno. This is very large swamp in desperate need of some attention. ~Andrew
On Fri, Apr 29, 2016 at 04:02:33PM +0100, Wei Liu wrote: > On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote: > > Avoid using system errno values when comparing with Xen errno values. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Ross Lagerwall <ross.lagerwall@citrix.com> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Wei Liu <wei.liu2@citrix.com> > > --- > > Using errno values inside of hypercall structs is not right IMHO, but there > > are already several occurrences of this. Although I'm adding the correct XEN_ > > prefixes here, it's very likely that new additions/modifications to this > > file will not take this into account, breaking it for OSes != Linux. > > This seems to be a rather thorny issue. > > 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. Roger.
On Fri, Apr 29, 2016 at 04:11:47PM +0100, Andrew Cooper wrote: > On 29/04/16 16:02, Wei Liu wrote: > > On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote: > >> Avoid using system errno values when comparing with Xen errno values. > >> > >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >> --- > >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >> Cc: Ross Lagerwall <ross.lagerwall@citrix.com> > >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> Cc: Wei Liu <wei.liu2@citrix.com> > >> --- > >> Using errno values inside of hypercall structs is not right IMHO, but there > >> are already several occurrences of this. Although I'm adding the correct XEN_ > >> prefixes here, it's very likely that new additions/modifications to this > >> file will not take this into account, breaking it for OSes != Linux. > > This seems to be a rather thorny issue. > > > > 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. > > > > Aren't FreeBSD and NetBSD already doing that? > > It is not practical for the privcmd driver to do this translation, as > most hypercalls through the privcmd driver are toolstack calls, and have > an unstable layout. > If what you mean is what Roger said in his other reply then I agree translating in privcmd is not doable. > Another thorny issue is that the ioctl() call for privcmd returns errors > via errno, which may be in system errno space, or xen errno space > depending on the source of the errno. > > This is very large swamp in desperate need of some attention. > Indeed. Wei. > ~Andrew
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: > > On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote: > > > Avoid using system errno values when comparing with Xen errno values. > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > --- > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > > Cc: Ross Lagerwall <ross.lagerwall@citrix.com> > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > --- > > > Using errno values inside of hypercall structs is not right IMHO, but there > > > are already several occurrences of this. Although I'm adding the correct XEN_ > > > prefixes here, it's very likely that new additions/modifications to this > > > file will not take this into account, breaking it for OSes != Linux. > > > > This seems to be a rather thorny issue. > > > > 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. Wei. > Roger.
On Fri, Apr 29, 2016 at 04:21:19PM +0200, Roger Pau Monne wrote: > Avoid using system errno values when comparing with Xen errno values. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Ross Lagerwall <ross.lagerwall@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > --- > Using errno values inside of hypercall structs is not right IMHO, but there > are already several occurrences of this. Although I'm adding the correct XEN_ > prefixes here, it's very likely that new additions/modifications to this > file will not take this into account, breaking it for OSes != Linux. While the discussion of errno is on-going, I think we should accept this patch for the time being. Wei.
diff --git a/tools/misc/xen-xsplice.c b/tools/misc/xen-xsplice.c index 695be7f..b31115c 100644 --- a/tools/misc/xen-xsplice.c +++ b/tools/misc/xen-xsplice.c @@ -13,6 +13,8 @@ #include <xenctrl.h> #include <xenstore.h> +#include <xen/errno.h> + static xc_interface *xch; void show_help(void) @@ -233,7 +235,7 @@ struct { .function = xc_xsplice_revert, }, { .allow = XSPLICE_STATE_CHECKED, - .expected = -ENOENT, + .expected = -XEN_ENOENT, .name = "unload", .function = xc_xsplice_unload, }, @@ -276,7 +278,7 @@ int action_func(int argc, char *argv[], unsigned int idx) name, errno, strerror(errno)); return -1; } - if ( status.rc == -EAGAIN ) + if ( status.rc == -XEN_EAGAIN ) { fprintf(stderr, "%s failed. Operation already in progress\n", name); return -1; @@ -319,7 +321,7 @@ int action_func(int argc, char *argv[], unsigned int idx) if ( status.state != original_state ) break; - if ( status.rc && status.rc != -EAGAIN ) + if ( status.rc && status.rc != -XEN_EAGAIN ) { rc = status.rc; break;
Avoid using system errno values when comparing with Xen errno values. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Ross Lagerwall <ross.lagerwall@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- Using errno values inside of hypercall structs is not right IMHO, but there are already several occurrences of this. Although I'm adding the correct XEN_ prefixes here, it's very likely that new additions/modifications to this file will not take this into account, breaking it for OSes != Linux. --- tools/misc/xen-xsplice.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)