Re: [Qemu-devel] [Xen-devel] [RFC PATCH V2 1/2] xen: pass kernel initrd to qemu

[Ian Campbell]

On Thu, 2014-06-12 at 23:43 -0600, Chun Yan Liu wrote:
> > > +static char *parse_cmdline(XLU_Config *config) 
> > > +{ 
> > > +    char *cmdline = NULL; 
> > > +    const char *root = NULL, *extra = ""; 
> > > + 
> > > +    xlu_cfg_get_string (config, "root", &root, 0); 
> > > +    xlu_cfg_get_string (config, "extra", &extra, 0); 
> >  
> > I sort of hate this root=/extra= stuff which comes from the PV world, 
> > since it is sort of exposing Linux-isms (e.g. root=%s etc). 
> >  
> > I'd far rather just have cmdline=. Since for PV this is needed for xm 
> > cfg file compatibility we are sort of stuck with it but for this new HVM 
> > functionality we don't have those backward compat concerns. 
> >  
> > If you want to just do xlu_cfg_get_string(..., "cmdline", ...) for the 
> > HVM case I would be OK with that but if you feel like it would you mind 
> > very much going a bit further and implementing cmdline for PV, such that 
> > it takes precedence over root/extra? Something like: 
> >  
> >     xlu_cfg_get_string (config, "cmdline", &cmdline, 0); 
> >     xlu_cfg_get_string (config, "root", &root, 0); 
> >     xlu_cfg_get_string (config, "extra", &extra, 0); 
> >  
> >     if (cmdline && root) 
> >        fprintf(stderr, "Warning: ignoring deprecated root= in favour of  
> > cmdline=\n"); 
> >     if (cmdline && extra) 
> >        fprintf(stderr, "Warning: ignoring deprecated extra= in favour of  
> > cmdline=\n"); 
> >     if (!cmdline) { 
> >         /* The existing code for dealing with extra/root etc */ 
> >         ...asprintf(&cmdline, ...) 
> >     } 
> >     return cmdline 
> >  
> > ? (In doing this I think it would be simpler to allow root=/extra= for 
> > HVM guests too even though they are immediately deprecated rather than 
> > making all of the above conditional) 
> >  
> I'll take this way and update code, and update manpage as well.

Awesome, thank you!

> > > @@ -1007,9 +1030,16 @@ static void parse_config_data(const char  
> > *config_source, 
> > >   
> > >      switch(b_info->type) { 
> > >      case LIBXL_DOMAIN_TYPE_HVM: 
> > > -        if (!xlu_cfg_get_string (config, "kernel", &buf, 0)) 
> > > -            fprintf(stderr, "WARNING: ignoring \"kernel\" directive for 
> > > HVM  
> > guest. " 
> > > -                    "Use \"firmware_override\" instead if you really 
> > > want a  
> > non-default firmware\n"); 
> > > +        if (!xlu_cfg_get_string (config, "kernel", &buf, 0)) { 
> > > +            if (strstr(buf, "hvmloader")) 
> > > +                fprintf(stderr, "WARNING: ignoring \"kernel\" directive  
> > for HVM guest. " 
> > > +                        "Use \"firmware_override\" instead if you really 
> > >  
> > want a non-default firmware\n"); 
> >  
> > I think we've had this for a few releases now, I wonder if it has served 
> > its purpose? Especially since the strstr check could have false 
> > positives and negatives. 
> 
> I think it's mainly to handle the old config file format, like:
> kernel="/usr/lib/xen/boot/hvmloader"
> in fact, in most of our hvm config files, this line still exists.

Hrm. I wonder if we could check for that specific string then? Perhaps
also $FOODIR/.../hvmloader (where $FOODIR is whichever bit of that path
comes from autoconf)?
 
> > IOW perhaps we should just delete this check and handle kernel the 
> > natural way. Trouble is I cannot estimate what sort of support burden 
> > this will make for us. Perhaps keep the warning but continue on having 
> > set u.hvm.kernel? e.g. 
> >     fprintf(stderr, 
> >             "WARNING: You seem to be using the \"kernel\" directive to  
> > override the firmware (hvmloader) for an HVM guest.\n" 
> >             "This option will boot the named kernel directly with no  
> > firmware present.\n" 
> >             "Use \"firmware_override\" instread if you really want a  
> > non-default firmware.\n"); 
> > ? 
> 
> Guest behavior will change for those using config file including:
> kernel="/usr/lib/xen/boot/hvmloader" (as does in xm config file)
> Till now, this is allowed and guest can be booted normally.
> Without check and set u.hvm.kernel,  guest will fail to boot.
> I'm afraid this is too risky?

If you guys still have config files with that in around then yes I think
it is a bit risky.

Ian.