Re: [Xen-devel] [PATCH v2 1/5] libxl: Add ERROR_NOTFOUND for libxl_domain_info when it cannot find the domain

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-03-13 at 16:26 -0400, Konrad Rzeszutek Wilk wrote:
> And use that for all of its callers in the tree.

I wonder -- is it better to have a generic ERROR_NOTFOUND for anything
which is not found, or to have a specific ERROR_DOMAIN_NOTFOUND
(likewise for other things).

Any opinions? I'm inclined towards the latter, e.g. imagine
disk_remove(domid, disk) -- ERROR_NOTFOUND doesn't tell you if the disk
or the domain is missing. (this example may or may not be hypothetical,
I didn't chekc).

> @@ -454,6 +454,8 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
>  
>      /* update /vm/<uuid>/name */
>      rc = libxl_domain_info(ctx, &info, domid);
> +    if (rc == ERROR_NOTFOUND)
> +        goto x_rc;
>      if (rc)
>          goto x_fail;

Might as well us x_rc for all failures, there seem little point in
squashing whatever libxl_domain_info returned into ERROR_FAIL for the
other cases either.

> @@ -5415,11 +5417,12 @@ static int libxl__set_vcpuonline_xenstore(libxl__gc 
> *gc, uint32_t domid,
>      libxl_dominfo info;
>      char *dompath;
>      xs_transaction_t t;
> -    int i, rc = ERROR_FAIL;
> +    int i, rc;
>  
>      libxl_dominfo_init(&info);
>  
> -    if (libxl_domain_info(CTX, &info, domid) < 0) {
> +    rc = libxl_domain_info(CTX, &info, domid);
> +    if (rc < 0) {
>          LOGE(ERROR, "getting domain info list");
>          goto out;
>      }

I think rc needs setting to ERROR_FAIL after this to retain the same
behaviour for the subsequent failures of e.g. libxl__xs_get_dompath
which doesn't otherwise set rc and previous relied on the ERROR_FAIL
from above.

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 5eec092..5164371 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1088,7 +1088,9 @@ int libxl_console_get_tty(libxl_ctx *ctx, uint32_t 
> domid, int cons_num,
>   */
>  int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char 
> **path);
>  
> -/* May be called with info_r == NULL to check for domain's existance */
> +/* May be called with info_r == NULL to check for domain's existance.

Could you fix the spelling of "existence" while you touch this line
please ;-)

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 47af340..69a91cc 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -63,6 +63,7 @@ libxl_error = Enumeration("error", [
>      (-17, "DEVICE_EXISTS"),
>      (-18, "REMUS_DEVOPS_DOES_NOT_MATCH"),
>      (-19, "REMUS_DEVICE_NOT_SUPPORTED"),
> +    (-20, "NOTFOUND"),
>      ], value_namespace = "")

Do we need a #define LIBXL_HAVE_ERROR_NOTFOUND to indicate this is
available, I think we probably do.

> @@ -7495,7 +7495,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
>          goto out;
>      }
>      for (c = 0; c < 10; c++) {
> -        if (libxl_domain_info(ctx, &info, 0)) {
> +        ret = -libxl_domain_info(ctx, &info, 0);
> +        if (ret) {

I think we don't need to do this bit. (The current inconsistent exit
codes form xl notwithstanding)



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