|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 14/14] libxl: build a device tree for ARM guests
Ian Campbell writes ("[PATCH v3 14/14] 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.
...
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index abe6685..3de2d76 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -19,4 +19,8 @@
> int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
> uint32_t domid);
>
> +/* */
> +int libxl__arch_domain_configure(libxl__gc *gc,
> + libxl_domain_build_info *info,
> + struct xc_dom_image *dom);
Missing content for comment ? Or maybe the comment just wants to go.
> +typedef uint32_t __be32;
> +typedef __be32 gic_interrupt_t[3];
Identifiers starting with __, and ones ending _t, are reserved for the
C implementation.
> +static void set_cell(__be32 **cellp, int size, uint64_t val)
> +{
> + int cells = size;
> +
> + while ( size-- )
The spaces inside ( ) aren't expected inside libxl.
> +static int make_memory_node(libxl__gc *gc, void *fdt,
> + unsigned long long base,
> + unsigned long long size)
> +{
> + int res;
> + char name[strlen("memory@") + 8 + 1];
> +
> + snprintf(name, sizeof(name), "memory@%08llx", base);
Why not use GCSPRINTF ?
> +/*
> + * Call "call" handling FDR_ERR_*. Will either:
> + * - loop back to retry_resize
> + * - set rc and goto out
> + * - fall through successfully
> + *
> + * On FDT_ERR_NOSPACE we start again from scratch rather than
> + * realloc+libfdt_open_into because "call" may have failed half way
> + * through a series of steps leaving the partial tree in an
> + * inconsistent state, e.g. leaving a node open.
> + */
> +#define FDT( call ) do { \
> + int fdt_res = (call); \
> + if (fdt_res == -FDT_ERR_NOSPACE && fdt_size < FDT_MAX_SIZE) \
> + goto retry_resize; \
> + else if (fdt_res < 0) { \
> + LOG(ERROR, "FDT: %s failed: %d = %s", \
> + #call, fdt_res, fdt_strerror(fdt_res)); \
> + rc = ERROR_FAIL; \
> + goto out; \
> + } \
> +} while(0)
My preference would be for this to be defined inside
libxl__arch_domain_configure and #undef'd at the end, so as to limit
the scope of the implicit reference to the label retry_resize.
> +int libxl__arch_domain_configure(libxl__gc *gc,
> + libxl_domain_build_info *info,
> + struct xc_dom_image *dom)
> +{
> + void *fdt;
> + int rc, res, fd;
> + size_t fdt_size = 0;
> +
> + const libxl_version_info *vers;
> + const struct arch_info *ainfo;
> +
> + assert(info->type == LIBXL_DOMAIN_TYPE_PV);
> +
> + vers = libxl_get_version_info(CTX);
> + if (vers == NULL) return ERROR_FAIL;
> +
> + ainfo = get_arch_info(gc, dom);
> + if (ainfo == NULL) return ERROR_FAIL;
> +
> + LOG(DEBUG, "constructing DTB for Xen version %d.%d guest",
> + vers->xen_version_major, vers->xen_version_minor);
> +
> +retry_resize:
For form's sake, how about making this an explicit loop ? e.g.
for (;;) {
next_resize:;
blah blah
if (something)
goto next_resize;
Then the end of the loop would be more obvious. You'd have to write
"break". (And you can #undef FDT at the end of the loop rather than
the end of the function, as the label becomes semantically void after
the end of the loop.)
(If C had labelled loops you could do
for resize (;;) {
blah blah
next resize;
but it doesn't. Emulating this with goto is a lot nicer IMO than
the naked goto.)
> + if (fdt_size) {
> + fdt_size <<= 1;
> + LOG(DEBUG, "Increasing FDT size to %zd and retrying", fdt_size);
> + } else {
> + fdt_size = 4096;
> + }
Can we rule out this loop getting out of control ?
> +#ifndef NDEBUG
Please don't key off NDEBUG. libxl ought never to be compiled
with NDEBUG. I don't see why you can't just have this code present
all the time.
> + if ( getenv("LIBXL_DEBUG_DUMP_DTB") ) {
> + const char *dtb = getenv("LIBXL_DEBUG_DUMP_DTB");
> +
> + fd = open(dtb, O_CREAT|O_TRUNC|O_WRONLY, 0666);
> + if ( fd < 0 ) {
> + LOGE(DEBUG, "cannot open %s for LIBXL_DEBUG_DUMP_DTB", dtb);
> + goto no_debug_dump;
This construction with "goto" is pretty ugly. Perhaps if you made
the whole thing a separate function it would be less so.
> + }
> +
> + 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);
> + goto no_debug_dump;
For example, if you had made it a separate function with the "goto
out" error handling style you wouldn't leak fd in this particular
error case :-). (Also, no spaces inside ( ) in ifs, please.)
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 10f976c..6351ebd 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -400,6 +400,10 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> LOGE(ERROR, "xc_dom_parse_image failed");
> goto out;
> }
> + if ( (ret = libxl__arch_domain_configure(gc, info, dom)) != 0 ) {
> + LOGE(ERROR, "libxl__arch_domain_configure failed");
> + goto out;
> + }
> if ( (ret = xc_dom_mem_init(dom, info->target_memkb / 1024)) != 0 ) {
> LOGE(ERROR, "xc_dom_mem_init failed");
> goto out;
The style in this function is anomalous:
* spaces inside ( )
* use of
if ((ret = blah) !=0 )
which hides the meat in the if and has !=0 clobber, instead of
rc = blah;
if (rc) goto out;
* use of "ret" for a libxl error code, rather than rc.
But I won't insist on you fixing it. Adding an extra occurrence is
probably better than having one call which is different to all the
others.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |