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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Nov 15, 2018 at 10:45:52AM +0000, Edwin Török wrote:
> 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?

I can use libxl_read_exactly instead. That saves me from writing some
code to handle every corner case.

> 
> > +    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.
> 

OK.

> 
> > +        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.
> 

OK.

> > +        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?
> 

We can print errno here.

> > +        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?

No, it doesn't mind.

Wei.

> 
> Best regards,
> --Edwin

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