Re: [Xen-devel] [PATCH] tools: implement initial ramdisk support for ARM.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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