Re: [Xen-devel] [PATCH v4 16/17] libxl: build a device tree for ARM guests

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[PATCH v4 16/17] libxl: build a device tree for ARM 
guests"):
> Uses xc_dom_devicetree_mem which was just added. The call to this needs to be
> carefully sequenced to be after xc_dom_parse_image (so we can tell which kind
> of guest we are building, although we don't use this yet) and before
> xc_dom_mem_init which tries to decide where to place the FDT in guest RAM.
> 
> Removes libxl_noarch which would only have been used by IA64 after this
> change. Remove IA64 as part of this patch.
> 
> There is no attempt to expose this as a configuration setting for the user.
> 
> Includes a debug hook to dump the dtb to a file for inspection.
...

The debug dump function has an anomalous error handling style[1] and
still leaks fd under some conditions.

([1] Well, actually it has the mistake-prone error handling style
found in the hypervisor, libxc, etc.)

> +static void debug_dump_fdt(libxl__gc *gc, void *fdt)
> +{
> +    int fd, rc;

You mean
       int fd=-1, rc, r;

We use "rc" inside libxl for a libxl error code.

> +    const char *dtb = getenv("LIBXL_DEBUG_DUMP_DTB");
> +
> +    if (!dtb)
> +        return;

           goto out;

> +    fd = open(dtb, O_CREAT|O_TRUNC|O_WRONLY, 0666);
> +    if (fd < 0) {
> +        LOGE(DEBUG, "cannot open %s for LIBXL_DEBUG_DUMP_DTB", dtb);
> +        return;

           goto out;

> +    }
> +
> +    rc = libxl_write_exactly(CTX, fd, fdt, fdt_totalsize(fdt), dtb, "dtb");
> +    if (rc < 0) {
> +        LOG(DEBUG, "unable to write DTB debug dump output %d", rc);
> +        return;

           goto out;

You probably don't need the LOG because libxl_write_exactly already
logs.

> +    }
> +

     out:

> +    rc = close(fd);
> +    if (rc < 0) {
> +        LOGE(DEBUG, "unable to close DTB debug dump output");

and this return is not needed:

> +        return;

> +    }
> +}

Apart from that, everything is fine.

(Sorry to find problems only in an unimportant debug function...)

Ian.

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