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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [PATCH] tools: implement initial ramdisk support for 
ARM."):
> On Thu, 2014-04-03 at 15:41 +0100, Ian Jackson wrote:
> > 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 :-(

Either of those places.  I spotted the comment in the code, but I
think it's part of the specification, not just the implmementation.
Moving the comment out to the header would do fine.

> > 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.

Oh, sorry.

> > 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.

Heh.  Fair enough.

> > > +    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.

Heh, everyone has a different idea about these things.

> I'll just use chosen here and reserve res for the setprop_inplace_calls
> later on

Fair enough.

> > This whole function is rather flabby, with two near-idential 9-line
> > stanzas.
> > 
> > How can fdt_setprop_inplace fail ?  Can you justify assert() ?
...
> 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.

Excellent.

Thanks,
Ian.

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