Re: [Xen-devel] [PATCH for-4.7 1/4] xen: remove usage of ENODATA error code

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

On Mon, May 02, 2016 at 12:22:35AM -0600, Jan Beulich wrote:
> >>> On 29.04.16 at 18:52, <roger.pau@xxxxxxxxxx> wrote:
> > On Fri, Apr 29, 2016 at 10:42:06AM -0600, Jan Beulich wrote:
> >> >>> On 29.04.16 at 18:34, <roger.pau@xxxxxxxxxx> wrote:
> >> > On Fri, Apr 29, 2016 at 10:19:41AM -0600, Jan Beulich wrote:
> >> >> >>> On 29.04.16 at 17:06, <roger.pau@xxxxxxxxxx> wrote:
> >> >> > On Fri, Apr 29, 2016 at 08:44:48AM -0600, Jan Beulich wrote:
> >> >> >> >>> On 29.04.16 at 16:21, <roger.pau@xxxxxxxxxx> wrote:
> >> >> >> > According to the POSIX standard for error codes [0], ENODATA is 
> >> >> >> > both
> >> >> >> > obsolescent (so it may be removed in the future) and optional.
> >> >> >> 
> >> >> >> It being optional still doesn't preclude us using it.
> >> >> >> 
> >> >> >> > Replace it's
> >> >> >> > usage with ENOENT, which seems like the closest match. Both 
> >> >> >> > FreeBSD and
> >> >> >> > OpenBSD don't have this error code in the native errno.h headers.
> >> >> >> 
> >> >> >> There's no rule saying that Xen's errno set must match any other 
> >> >> >> OS'es.
> >> >> >> That's one of the reasons why we (finally) separated ours.
> >> >> > 
> >> >> > I understand that, but doing this means that then on the toolstack 
> >> >> > side we 
> > 
> >> >> > need to start doing ifdefery in order to match values that are not 
> >> >> > present 
> > 
> >> >> > in the native OS. For example a check was added to libxl against 
> >> >> > XENVER_build_id returning ENODATA, this means that then on libxl I 
> >> >> > would 
> >> >> > have to add a:
> >> >> > 
> >> >> > #ifdef __FreeBSD__
> >> >> > #define ENODATA ENOENT
> >> >> > #endif
> >> >> 
> >> >> You ought to be using XEN_NODATA there anyway.
> >> > 
> >> > No, the privcmd driver is the one that performs the translation from Xen 
> >> > error codes into the OS error code space, so the tools just see error 
> >> > codes 
> > 
> >> > in the OS space. No tools at all use XEN_* error codes directly.
> >> 
> >> That's the supposed behavior for return values, but not for
> >> structure contents. The structures are Xen-specific, so them
> >> holding Xen-specific values is known to the callers, and they
> >> should (be made) cope.
> > 
> > And the usage of ENODATA I'm trying to fix here is from the direct return 
> > of 
> > 
> > an hypercall (__HYPERVISOR_xen_version). I don't mind adding this define, I 
> > just think it would be better to simply replace the usage of ENODATA with 
> > something else, so I could avoid adding more ifdefery to the tools.
> > 
> > Would you be fine with me just adjusting xen_build_id (in 
> > xen/common/version.c) to return ENOENT instead of ENODATA?
> 
> Well, I wouldn't be particularly happy, but I'd also not be as heavily
> opposed as to removing that error code altogether. In general (and
> I probably should have said this in the first reply, despite there
> having been the other more important reason to object) I don't view
> ENOENT as a good replacement for ENODATA. The two really mean
> different things, but in this particular case it would seem a reasonable
> replacement.

Right. But since the privcmd driver does the error code translation I think 
ENOENT is the closest match to ENODATA when doing automatic error 
translation (I'm open to suggestions if someone knows a better replacement 
for it).

> But how would that help you? Would you mean to monitor future
> patches for not again introducing some use of ENODATA that the
> tool stack wants to explicitly check for? That would be quite tedious
> a task...

Yes it is, but TBH I cannot find any other solution. Adding ENODATA to the 
FreeBSD list of error codes seems quite pointless when there's no in-tree 
user of it.

Roger.

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