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

[Chun Yan Liu]

>>> On 6/13/2014 at 04:28 PM, in message
<address@hidden>, Ian Campbell
<address@hidden> wrote: 
> 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)? 

Or simply check basename="hvmloader".

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