Re: [Xen-devel] [PATCH RFC 1/9] libxl idl: add comments to error enum

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Rob Hoes writes ("[PATCH RFC 1/9] libxl idl: add comments to error enum"):
...
>  libxl_error = Enumeration("error", [

Thanks for this.  It think at this stage it is probably most helpful
to concentrate on the specification, that is the list of error codes
and corresponding docs.

So:


I think it would be worth trying to categorise these errors by whose
fault they are likely to be.

> +    # Generic failure; code should be avoided (often seen as "rc = -1")
>      (-1, "NONSPECIFIC"),
> +    
> +    # Libxl version mismatch
>      (-2, "VERSION"),
> +    
> +    # General failure; code should be avoided
>      (-3, "FAIL"),

I think you should probably write `deprecated in new libxl code, due to
being too vague' instead of `should be avoided'.

> +    # Not implemented
>      (-4, "NI"),

Seems not to have any use sites.  Should presumably be deprecated due
to having a daft name.

> +    # Out of memory (malloc or similar failed)
>      (-5, "NOMEM"),

Should say whether this includes "host does not have enough memory to
create the specified domain" as well as "malloc failed in toolstack".

NB that we are trying to phase out the handling of the latter as
anything except a fatal error.

> +    # General failure; code should be avoided
>      (-6, "INVAL"),

ERROR_INVAL means "there is something wrong with the parameters to the
operation (but we don't say exactly what).

> +    # General failure; code should be avoided (used only in "xl")
>      (-7, "BADFAIL"),

Maybe it should be renamed.

> +    # Domain responded to suspend request
>      (-8, "GUEST_TIMEDOUT"),

Failed to respond, you mean.

> +    # A xenstore watch has timed out
>      (-9, "TIMEDOUT"),

Not true, I'm afraid.  xenstore watches cannot time out, and this is
used for other things besides `we were waiting for something in
xenstore and decided it was taking too long' (which anyway does not
really say what it was that we were waiting for - lots of things
communicate via xenstore).

Also this error doesn't say what timed out, so this error code should
be deprecated.

> +    # Event has not happened (libxl_event_check)
>      (-11, "NOT_READY"),

I would say "Requested event has not (yet) happened".

> +    # Could not acquire lock
>      (-15, "LOCK_FAIL"),

This seems to be used entirely for the userdata lock.  I would be
tempted to argue that it's not particularly be useful to report this
distinctly to the application, because what it really means is "either
the system is totally hosed, or the permissions or existence of the
userdata storage area are wrong".  The latter ought to be a separate
error code.

> +    # Unable to find JSON domain config
>      (-16, "JSON_CONFIG_EMPTY"),

There is clearly something wrong here.  This is supposed to be handled
by most callers I think.  The comment at the one generation site says:

        /* No logging, not necessary an error from caller's PoV. */
        rc = ERROR_JSON_CONFIG_EMPTY;
        goto out;

But I can find no special handling of it by callers.

> +    # Remus ops do not match device
>      (-18, "REMUS_DEVOPS_DOES_NOT_MATCH"),

I think this is supposed to be internal-only.

> +    # Remus device not supported
>      (-19, "REMUS_DEVICE_NOT_SUPPORTED"),

Starting remus was attempted but the domain has a device which is not
supported by remus.

> +    # Requested domain was not found
>      (-21, "DOMAIN_NOTFOUND"),

AFAICT might also sometimes happen if the domain disappears during an
operation ?

Ian.

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