| Message ID | 1397550789-43232-2-git-send-email-roger.pau@citrix.com |
|---|---|
| State | New |
| Headers | show |
On 15.04.14 10:33, Roger Pau Monne wrote: > ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the > hypervisor are translated to ENOENT. > > Also, the error code is returned in errno if the call returns -1, so > compare the error code with the value in errno instead of the value > returned by the function. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Cc: xen-devel@lists.xenproject.org > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Anthony Perard <anthony.perard@citrix.com> > --- > xen-all.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/xen-all.c b/xen-all.c > index 0b4d934..4026b7b 100644 > --- a/xen-all.c > +++ b/xen-all.c > @@ -499,11 +499,15 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > start_addr >> TARGET_PAGE_BITS, npages, > bitmap); > if (rc < 0) { > - if (rc != -ENODATA) { > +#ifdef ENODATA > + if (errno == ENODATA) { > +#else > + if (errno == ENOENT) { > +#endif This does not look good to me. I suggest this: #ifndef ENODATA #define ENODATA ENOENT #endif Christoph > memory_region_set_dirty(framebuffer, 0, size); > DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx > ", 0x" TARGET_FMT_plx "): %s\n", > - start_addr, start_addr + size, strerror(-rc)); > + start_addr, start_addr + size, strerror(errno)); > } > return; > } >
On Tue, Apr 15, 2014 at 10:33:08AM +0200, Roger Pau Monne wrote: > ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the > hypervisor are translated to ENOENT. > > Also, the error code is returned in errno if the call returns -1, so > compare the error code with the value in errno instead of the value > returned by the function. But in xenctrl.h, the comment on the prototype of the function says that it return the error code, there is nothing about errno been updated. So I think rc should still be check for the error code instead of errno. > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Cc: xen-devel@lists.xenproject.org > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Anthony Perard <anthony.perard@citrix.com> > --- > xen-all.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/xen-all.c b/xen-all.c > index 0b4d934..4026b7b 100644 > --- a/xen-all.c > +++ b/xen-all.c > @@ -499,11 +499,15 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > start_addr >> TARGET_PAGE_BITS, npages, > bitmap); > if (rc < 0) { > - if (rc != -ENODATA) { > +#ifdef ENODATA > + if (errno == ENODATA) { > +#else > + if (errno == ENOENT) { > +#endif > memory_region_set_dirty(framebuffer, 0, size); > DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx > ", 0x" TARGET_FMT_plx "): %s\n", > - start_addr, start_addr + size, strerror(-rc)); > + start_addr, start_addr + size, strerror(errno)); > } > return; > } > -- > 1.7.7.5 (Apple Git-26) >
On 15/04/14 18:15, Anthony PERARD wrote: > On Tue, Apr 15, 2014 at 10:33:08AM +0200, Roger Pau Monne wrote: >> ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the >> hypervisor are translated to ENOENT. >> >> Also, the error code is returned in errno if the call returns -1, so >> compare the error code with the value in errno instead of the value >> returned by the function. > > But in xenctrl.h, the comment on the prototype of the function says that > it return the error code, there is nothing about errno been updated. So > I think rc should still be check for the error code instead of errno. Then I think the comment is wrong (or I'm missing something), xc_hvm_track_dirty_vram return code comes from do_xen_hypercall, which is basically a wrapper around ioctl, and according to the ioctl man page (both Linux and FreeBSD versions): "On error, -1 is returned, and errno is set appropriately." Roger.
On 15/04/14 17:28, Roger Pau Monné wrote: > On 15/04/14 18:15, Anthony PERARD wrote: >> On Tue, Apr 15, 2014 at 10:33:08AM +0200, Roger Pau Monne wrote: >>> ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the >>> hypervisor are translated to ENOENT. >>> >>> Also, the error code is returned in errno if the call returns -1, so >>> compare the error code with the value in errno instead of the value >>> returned by the function. >> But in xenctrl.h, the comment on the prototype of the function says that >> it return the error code, there is nothing about errno been updated. So >> I think rc should still be check for the error code instead of errno. > Then I think the comment is wrong (or I'm missing something), > xc_hvm_track_dirty_vram return code comes from do_xen_hypercall, which > is basically a wrapper around ioctl, and according to the ioctl man page > (both Linux and FreeBSD versions): > > "On error, -1 is returned, and errno is set appropriately." > > Roger. The only thing you can safely assume about libxc and error values is that there is no consistency whatsoever (and in some cases a completely inability to determine success or failure from the return value). This is acknowledged as needing to be fixed. The only way you have of actually working out the error semantics are to follow the function you care about all the way to the ioctl. ~Andrew
On 15/04/14 14:33, Egger, Christoph wrote: > On 15.04.14 10:33, Roger Pau Monne wrote: >> ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the >> hypervisor are translated to ENOENT. >> >> Also, the error code is returned in errno if the call returns -1, so >> compare the error code with the value in errno instead of the value >> returned by the function. >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> Cc: xen-devel@lists.xenproject.org >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Cc: Anthony Perard <anthony.perard@citrix.com> >> --- >> xen-all.c | 8 ++++++-- >> 1 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/xen-all.c b/xen-all.c >> index 0b4d934..4026b7b 100644 >> --- a/xen-all.c >> +++ b/xen-all.c >> @@ -499,11 +499,15 @@ static void xen_sync_dirty_bitmap(XenIOState *state, >> start_addr >> TARGET_PAGE_BITS, npages, >> bitmap); >> if (rc < 0) { >> - if (rc != -ENODATA) { >> +#ifdef ENODATA >> + if (errno == ENODATA) { >> +#else >> + if (errno == ENOENT) { >> +#endif > > This does not look good to me. > I suggest this: > > #ifndef ENODATA > #define ENODATA ENOENT > #endif Hello Christoph, Thanks for the review, I slightly prefer my original proposal over what you suggest, I think it's more clear that there are different error codes, while your suggestion simply masks the fact than FreeBSD doesn't have ENODATA. Anyway, I wouldn't mind changing it if the maintainer prefers your approach. Roger.
diff --git a/xen-all.c b/xen-all.c index 0b4d934..4026b7b 100644 --- a/xen-all.c +++ b/xen-all.c @@ -499,11 +499,15 @@ static void xen_sync_dirty_bitmap(XenIOState *state, start_addr >> TARGET_PAGE_BITS, npages, bitmap); if (rc < 0) { - if (rc != -ENODATA) { +#ifdef ENODATA + if (errno == ENODATA) { +#else + if (errno == ENOENT) { +#endif memory_region_set_dirty(framebuffer, 0, size); DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx ", 0x" TARGET_FMT_plx "): %s\n", - start_addr, start_addr + size, strerror(-rc)); + start_addr, start_addr + size, strerror(errno)); } return; }
ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the hypervisor are translated to ENOENT. Also, the error code is returned in errno if the call returns -1, so compare the error code with the value in errno instead of the value returned by the function. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Cc: xen-devel@lists.xenproject.org Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Cc: Anthony Perard <anthony.perard@citrix.com> --- xen-all.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)