|
[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
On Thu, 2013-11-07 at 17:30 +0000, Ian Jackson wrote:
> 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.
I'll nuke it.
>
> > +typedef uint32_t __be32;
> > +typedef __be32 gic_interrupt_t[3];
>
> Identifiers starting with __, and ones ending _t, are reserved for the
> C implementation.
Oops, types carried over from the h/v side.
> > +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 ?
Another hypervisor-ism which I carried over without enough thought.
>
> > +/*
> > + * 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.
Good idea.
> > +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.)
I agree, I'll change things along these lines.
>
> > + 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 ?
The FDT macro has a check and error if it gets too big. I'll see about
moving the <<= into the macro, which I suspect will seem more natural
with a loop called "retry".
>
> > +#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.
I wasn't entirely sure about keeping it at all, I wrote it for my own
debugging purposes. I'm happy with it unconditional if you are.
> > + 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.
Yes, I suspect it will.
> > + }
> > +
> > + 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.)
Ack. Will do a thorough sweep.
> > 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.
I agree.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |