Re: [Xen-devel] [PATCH v4 05/14] libxl: Load guest BIOS from file

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Tue, 2016-03-15 at 20:53 -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:40PM +0000, Anthony PERARD wrote:
> > 
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -1121,6 +1121,15 @@ Requires device_model_version=qemu-xen.
> >  
> >  =back
> >  
> > +=item B<bios_override="PATH">
> Perhaps 'bios_override_path' ?
> 
Or 'bios_path_override' ?

Wow, a French, an Italian and a Polish arguing about best looking
English-ish words... This must be a (bad) joke! :-P :-P 

> > +
> > +Override the path to the blob to be used as BIOS. The blob
> > provided here MUST
> Perhaps:
> 
> Override the path to search for the B<bios=>?
> 
AFAIUI, B<bios=> does not contain an exact file name, but the
indication of what kind of BIOS we want. Therefore, referencing it
directly like this may be confusing (IMHO).

So, maybe something like: "Override the path at which to look for the
BIOS binary. Such binary MUST be consistent with what has been
specified in B<bios=>."

> Or is this the full path including the name?? In which case should it
> mention that B<bios=> is overriden?
> 
If that is the case, indeed.

> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -862,6 +862,38 @@ err:
> >      return ret;
> >  }
> >  
> > +static int libxl__load_hvm_firmware_module(libxl__gc *gc,
> > +                                           const char *filename,
> > +                                           const char *what,
> > +                                           struct
> > xc_hvm_firmware_module *m)
> > +{
> > +    int datalen = 0;
> > +    void *data = NULL;
> > +    int e;
> That is interesting.
> 
> The CODING_STYLE in tools/libxl says:
> 
>  36   int rc;    /* a libxl error code - and not anything else
> */                   
>  37   int r;     /* the return value from a system call (or libxc
> call) */          
>  38   bool ok;   /* the success return value from a boolean function
> */             
> 
> And libxl_read_file_contents is quite weird. It does return an normal
> errno value, so .. one could say it should be 'r'? But the existing
> users
> of this are either 'e','ret' or 'rc'. 'rc' is not good, and 'ret'
> is 'rc' is clearly wrong.
> 
> How confusing.
> 
> Ian, Wei, maybe you could clarify please?
> 
Hehe... I'd also go for 'r', and call the (or at least some of the)
existing users not codying style compliant, but it's indeed a funny
case, and we should hear from maintainers what they prefer. :-)

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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