Re: [Xen-devel] [PATCH 3/3] xen-init-dom0: set Dom0 UUID if requested

[Edwin Török <edvin.torok@xxxxxxxxxx>]

On 14/11/2018 18:17, Wei Liu wrote:
> Read from XEN_CONFIG_DIR/dom0-uuid. If it contains a valid UUID, set
> it for Dom0.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>

[snip]
In general this looks good, however I am not familiar with libxl
conventions, so just some generic comments below.

> +static void get_dom0_uuid(libxl_uuid *uuid)
> +{
> +    int fd = -1;
> +    ssize_t r;
> +    char uuid_buf[LIBXL_UUID_FMTLEN+1];
> +
> +    libxl_uuid_clear(uuid);
> +
> +    fd = open(DOM0_UUID_PATH, O_RDONLY);
> +    if (fd < 0) {
> +        fprintf(stderr, "failed to open %s\n", DOM0_UUID_PATH);
> +        goto out;
> +    }
> +
> +    r = read(fd, uuid_buf, LIBXL_UUID_FMTLEN);

Could this be a short read? I'm not familiar with libxl conventions, but
would there be a utility function that repeats the read if it is short,
or would fread be better?

> +    if (r == -1) {
> +        fprintf(stderr, "failed to read %s, errno %d\n", DOM0_UUID_PATH, 
> errno);
> +        goto out;
> +    }
> +
> +    if (r != LIBXL_UUID_FMTLEN) {
> +        fprintf(stderr, "file too short\n");

Would be nice to print which file is too short.


> +        goto out;
> +    }
> +
> +    uuid_buf[LIBXL_UUID_FMTLEN] = 0;
> +
> +    if (libxl_uuid_from_string(uuid, uuid_buf)) {
> +        fprintf(stderr, "failed to parse UUID\n");

As above, would be nice to print which file this error is from.

> +        libxl_uuid_clear(uuid);
> +    }
> +
> +out:
> +    if (fd >= 0) close(fd);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int rc;
> -    struct xs_handle *xsh;
> +    struct xs_handle *xsh = NULL;
> +    xc_interface *xch = NULL;
>      char *domname_string = NULL, *domid_string = NULL;
> +    libxl_uuid uuid;
>  
>      xsh = xs_open(0);
>      if (!xsh) {
>          fprintf(stderr, "cannot open xenstore connection\n");
> -        exit(1);
> +        rc = 1;
> +        goto out;
> +    }
> +
> +    xch = xc_interface_open(NULL, NULL, 0);
> +    if (!xch) {
> +        fprintf(stderr, "xc_interface_open() failed\n");
> +        rc = 1;
>      }
>  
>      /* Sanity check: this program can only be run once. */
> @@ -31,7 +82,16 @@ int main(int argc, char **argv)
>          goto out;
>      }
>  
> -    rc = gen_stub_json_config(0, NULL);
> +    get_dom0_uuid(&uuid);
> +
> +    if (!libxl_uuid_is_nil(&uuid) &&
> +        xc_domain_sethandle(xch, 0, libxl_uuid_bytearray(&uuid))) {
> +        fprintf(stderr, "failed to set Dom0 UUID\n");

Can xc_domain_sethandle tell us why it failed?

> +        rc = 1;
> +        goto out;
> +    }
> +
> +    rc = gen_stub_json_config(0, &uuid);
>      if (rc)
>          goto out;
>  
> @@ -55,6 +115,7 @@ out:
>      free(domid_string);
>      free(domname_string);
>      xs_close(xsh);
> +    xc_interface_close(xch);

I assume this function doesn't mind getting called with NULL, right?

Best regards,
--Edwin

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel