[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] tools: implement initial ramdisk support for ARM.
On Thu, 2014-04-03 at 15:41 +0100, Ian Jackson wrote: > Ian Campbell writes ("[PATCH] tools: implement initial ramdisk support > for ARM."): > > [stuff] > > Thanks. This will be useful. I have some fairly minor comments: > > > Furthermore if the ramdisk has been explicitly placed then > > xc_dom_build_image() > > assumes that it is not to be decompressed (since that would muck up the > > sizings > > used on placement). > > This strikes me as rather subtle. Perhaps it should be documented in > the header file. I did mention it in the comment in the code, I could add it to the header too. Would you put it next to the struct defining ramdisk_seg or the xc_dom_build_image? Neither of them are well documented right now :-( > > @@ -326,7 +364,6 @@ int xc_dom_feature_translated(struct xc_dom_image *dom) > > * mode: C > > * c-file-style: "BSD" > > * c-basic-offset: 4 > > - * tab-width: 4 > > Not mentioned in your commit message, but fair enough. It was: The emacs magic in tools/libxc/xc_dom_arm.c was wrong, so I corrected it while I was there. > > > +/* setup arch specific hardware description, i.e. DTB on ARM */ > > +int libxl__arch_domain_init_hw_description(libxl__gc *gc, > > + libxl_domain_build_info *info, > > + struct xc_dom_image *dom); > > +/* finalize arch specific hardware description. */ > > +int libxl__arch_domain_finalise_hw_description(libxl__gc *gc, > > + libxl_domain_build_info *info, > > + struct xc_dom_image *dom); > > These comments mostly simply repeat the function name. I would delete > the second one (but won't object if you want to keep it ...) I mostly added them for the "i.e. DTB On ARM bit" on the first one, and having added that I thought I should say something on the other one too. I'm inclined to leave them, but to make my spelling of finalise be consistent. > > @@ -475,7 +488,7 @@ next_resize: > > FDT( fdt_begin_node(fdt, "") ); > > > > FDT( make_root_properties(gc, vers, fdt) ); > > - FDT( make_chosen_node(gc, fdt, info) ); > > + FDT( make_chosen_node(gc, fdt, !!dom->ramdisk_blob, info) ); > > The !! are redundant when converting an expression to a bool. But you > can leave them in if you feel it's clearer. > > > + if (ramdisk) { > > + int chosen, res = fdt_path_offset(fdt, "/chosen"); > > That's rather confusing. At the very least can we have these as two > lines so it doesn't look like a multiple-values-bind to Lisp > programmers :-) ? Sure. > But I'm not sure of the need for chosen to be separate from res. Later on I reuse res but still need chosen in my hand. I thought it might be clearer to use res for the fdt_path_offset result while I was still treating it as a potential error code and launder it into chosen (a node offset) once it was shown to be fine. I suppose the fact you queried it means I was wrong. I'll just use chosen here and reserve res for the setprop_inplace_calls later on > This whole function is rather flabby, with two near-idential 9-line > stanzas. > > How can fdt_setprop_inplace fail ? Can you justify assert() ? * -FDT_ERR_NOSPACE, if len is not equal to the property's current length * -FDT_ERR_NOTFOUND, node does not have the named property * -FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag * -FDT_ERR_BADMAGIC, * -FDT_ERR_BADVERSION, * -FDT_ERR_BADSTATE, * -FDT_ERR_BADSTRUCTURE, * -FDT_ERR_TRUNCATED, standard meanings All of which I think would indicate a previous coding error in this file. Since the property must certainly exist and have the expected length etc. I think I can justify an assert. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |