[PATCH] Don't attach needless options when launch pygrub

[PATCH] Don't attach needless options when launch pygrub

[Yu Zhiguo]

We should always run grub if bootloader is specified,
options 'kernel' and 'ramdisk' are needless.

Signed-off-by: Yu Zhiguo <yuzg@cn.fujitsu.com>

diff -r d867eb643fe4 -r 38d9dac1de56 tools/python/xen/xend/XendBootloader.py
--- a/tools/python/xen/xend/XendBootloader.py	Tue Jul 13 18:17:28 2010 +0100
+++ b/tools/python/xen/xend/XendBootloader.py	Wed Jul 14 22:18:58 2010 +0800
@@ -24,8 +24,7 @@
 import pty, termios, fcntl
 from xen.lowlevel import ptsname
 
-def bootloader(blexec, disk, dom, quiet = False, blargs = '', kernel = '',
-               ramdisk = '', kernel_args = ''):
+def bootloader(blexec, disk, dom, quiet = False, blargs = '', kernel_args = ''):
     """Run the boot loader executable on the given disk and return a
     config image.
     @param blexec  Binary to use as the boot loader
@@ -96,10 +95,6 @@
     (child, m2) = pty.fork()
     if (not child):
         args = [ blexec ]
-        if kernel:
-            args.append("--kernel=%s" % kernel)
-        if ramdisk:
-            args.append("--ramdisk=%s" % ramdisk)
         if kernel_args:
             args.append("--args=%s" % kernel_args)
         if quiet:

Re: [PATCH] Don't attach needless options when launch pygrub

[Ian Campbell]

On Wed, 2010-07-14 at 07:15 +0100, Yu Zhiguo wrote:
> We should always run grub if bootloader is specified,
> options 'kernel' and 'ramdisk' are needless.

Not quite. If you specify both bootloader and kernel then this instructs
pygrub to extract the specific named file from the guest file system,
similarly for the ramdisk.

With your patch the functionality is still available using the
bootloader_args field to add the "--kernel=FOO" yourself but I'm not
sure it is a big enough issue to be worth changing. Is there some
specific reason you don't like the current behaviour?

In any case you seem to have forgotten to update the caller in
tools/python/xen/xend/XendDomainInfo.py.

I also have patches pending on the list from Monday which add bootloader
functionality, including this behaviour, to libxl so if we decide to
change it here it will need to change there as well.

Ian.


> 
> Signed-off-by: Yu Zhiguo <yuzg@cn.fujitsu.com>
> 
> diff -r d867eb643fe4 -r 38d9dac1de56 tools/python/xen/xend/XendBootloader.py
> --- a/tools/python/xen/xend/XendBootloader.py	Tue Jul 13 18:17:28 2010 +0100
> +++ b/tools/python/xen/xend/XendBootloader.py	Wed Jul 14 22:18:58 2010 +0800
> @@ -24,8 +24,7 @@
>  import pty, termios, fcntl
>  from xen.lowlevel import ptsname
>  
> -def bootloader(blexec, disk, dom, quiet = False, blargs = '', kernel = '',
> -               ramdisk = '', kernel_args = ''):
> +def bootloader(blexec, disk, dom, quiet = False, blargs = '', kernel_args = ''):
>      """Run the boot loader executable on the given disk and return a
>      config image.
>      @param blexec  Binary to use as the boot loader
> @@ -96,10 +95,6 @@
>      (child, m2) = pty.fork()
>      if (not child):
>          args = [ blexec ]
> -        if kernel:
> -            args.append("--kernel=%s" % kernel)
> -        if ramdisk:
> -            args.append("--ramdisk=%s" % ramdisk)
>          if kernel_args:
>              args.append("--args=%s" % kernel_args)
>          if quiet:
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] Don't attach needless options when launch pygrub

[Yu Zhiguo]

Hi Ian,

Ian Campbell wrote:
> On Wed, 2010-07-14 at 07:15 +0100, Yu Zhiguo wrote:
>> We should always run grub if bootloader is specified,
>> options 'kernel' and 'ramdisk' are needless.
> 
> Not quite. If you specify both bootloader and kernel then this instructs
> pygrub to extract the specific named file from the guest file system,
> similarly for the ramdisk.
> 

 Do you mean in this case, pygrub will use specified kernel
that lie in the filesystem of the DomU?

 I think this is good. But now pygrub's action is using the
specified kernel in Dom0, but not run grub.

> With your patch the functionality is still available using the
> bootloader_args field to add the "--kernel=FOO" yourself but I'm not
> sure it is a big enough issue to be worth changing. Is there some
> specific reason you don't like the current behaviour?

 Yes, all the bootloader_args will be attach to pygrub option......
 I think if bootloader is specified, omit kernel is simple.

> 
> In any case you seem to have forgotten to update the caller in
> tools/python/xen/xend/XendDomainInfo.py.
> 
 Oh, sorry for this mistake.

> I also have patches pending on the list from Monday which add bootloader
> functionality, including this behaviour, to libxl so if we decide to
> change it here it will need to change there as well.
> 

 OK, I'll try after patchs are applied.

Yu

> Ian.
> 
> 
>> Signed-off-by: Yu Zhiguo <yuzg@cn.fujitsu.com>
>>
>> diff -r d867eb643fe4 -r 38d9dac1de56 tools/python/xen/xend/XendBootloader.py
>> --- a/tools/python/xen/xend/XendBootloader.py	Tue Jul 13 18:17:28 2010 +0100
>> +++ b/tools/python/xen/xend/XendBootloader.py	Wed Jul 14 22:18:58 2010 +0800
>> @@ -24,8 +24,7 @@
>>  import pty, termios, fcntl
>>  from xen.lowlevel import ptsname
>>  
>> -def bootloader(blexec, disk, dom, quiet = False, blargs = '', kernel = '',
>> -               ramdisk = '', kernel_args = ''):
>> +def bootloader(blexec, disk, dom, quiet = False, blargs = '', kernel_args = ''):
>>      """Run the boot loader executable on the given disk and return a
>>      config image.
>>      @param blexec  Binary to use as the boot loader
>> @@ -96,10 +95,6 @@
>>      (child, m2) = pty.fork()
>>      if (not child):
>>          args = [ blexec ]
>> -        if kernel:
>> -            args.append("--kernel=%s" % kernel)
>> -        if ramdisk:
>> -            args.append("--ramdisk=%s" % ramdisk)
>>          if kernel_args:
>>              args.append("--args=%s" % kernel_args)
>>          if quiet:
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 
> 
> 
> 
> 

Re: [PATCH] Don't attach needless options when launch pygrub

[Ian Campbell]

On Wed, 2010-07-14 at 08:29 +0100, Yu Zhiguo wrote:
> Hi Ian,
> 
> Ian Campbell wrote:
> > On Wed, 2010-07-14 at 07:15 +0100, Yu Zhiguo wrote:
> >> We should always run grub if bootloader is specified,
> >> options 'kernel' and 'ramdisk' are needless.
> > 
> > Not quite. If you specify both bootloader and kernel then this instructs
> > pygrub to extract the specific named file from the guest file system,
> > similarly for the ramdisk.
> > 
> 
>  Do you mean in this case, pygrub will use specified kernel
> that lie in the filesystem of the DomU?

I thought so, I looks like I was mistaken though.

>  I think this is good. But now pygrub's action is using the
> specified kernel in Dom0, but not run grub.

Hmm, pygrub is certainly run, regardless of having a kernel configured
or not. What is in question is what --kernel and --ramdisk actually
cause pygrub to do and whether that is useful.

As far as I can see the --kernel and --ramdisk options end up in the
incfg map which only used in a handful of places, most of which just
extract incfg["args"]. The only places which do not do this are the
calls to sniff_solaris and sniff_netware both of which appear to make
use of incfg["kernel"] (but not incfg["ramdisk"]).

So it looks like specifying the kernel option in addition to bootloader
is infact useful if you are booting a Solaris or Netware domU but is
harmless/ignored otherwise. I think we need to continue to support this
use case and I don't see any particular reason to force those users to
change their configuration file syntax for this issue (if it's even an
issue, I still don't really see the problem).

Perhaps it would be better to update pygrub so that --kernel actually
does something consistent in the non-{Solaris,Netware} case, such as
perhaps selecting the configuration entry with the match kernel path
instead of defaulting to entry 0? (e.g. make "-q --kernel=/boot/FOO"
select the entry with kernel /boot/FOO)

It looks like --ramdisk (and the associated plumbing through xend) may
in fact be useless at this time. I'd say it is harmless to plumb it
through for consistency though -- perhaps in the future pygrub (or
another bootloader) might want to use it.

Ian.

Re: [PATCH] Don't attach needless options when launch pygrub

[Yu Zhiguo]

Ian Campbell wrote:
> 
> As far as I can see the --kernel and --ramdisk options end up in the
> incfg map which only used in a handful of places, most of which just
> extract incfg["args"]. The only places which do not do this are the
> calls to sniff_solaris and sniff_netware both of which appear to make
> use of incfg["kernel"] (but not incfg["ramdisk"]).
> 
> So it looks like specifying the kernel option in addition to bootloader
> is infact useful if you are booting a Solaris or Netware domU but is
> harmless/ignored otherwise. I think we need to continue to support this

 It seems that incfg will be returned directly if DomU is not Solaris,

def sniff_solaris(fs, cfg):
    if not fs.file_exists("/platform/i86xpv/kernel/unix"):
        return cfg


    chosencfg = sniff_solaris(fs, incfg)

 So, incfg change to chosencfg and then will be used.

 Should we fix sniff_solaris let it don't modify chosencfg if DomU
is not Solaris at least?


> use case and I don't see any particular reason to force those users to
> change their configuration file syntax for this issue (if it's even an
> issue, I still don't really see the problem).
> 
> Perhaps it would be better to update pygrub so that --kernel actually
> does something consistent in the non-{Solaris,Netware} case, such as
> perhaps selecting the configuration entry with the match kernel path
> instead of defaulting to entry 0? (e.g. make "-q --kernel=/boot/FOO"
> select the entry with kernel /boot/FOO)
> 

 What about copy the specified 'kernel' from DomU to a temp file.
If there are 'bootloader' but no 'kernel', pygrub will copy and create temp file.
We can do the same things.

Yu

> It looks like --ramdisk (and the associated plumbing through xend) may
> in fact be useless at this time. I'd say it is harmless to plumb it
> through for consistency though -- perhaps in the future pygrub (or
> another bootloader) might want to use it.
> 
> Ian.
> 
> 
> 
> 


-- 
Regards
Yu Zhiguo
--------------------------------------------------
Yu Zhiguo
Development Dept.I
Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)
8/F., Civil Defense Building, No.189 Guangzhou Road,
Nanjing, 210029, China
TEL: +86+25-86630566-853
COINS: 79955-853
FAX: +86+25-83317685
MAIL: yuzg@cn.fujitsu.com
--------------------------------------------------
This communication is for use by the intended recipient(s) only and may contain information that is privileged, confidential and exempt from disclosure under applicable law. If you are not an intended recipient of this communication, you are hereby notified that any dissemination, distribution or copying hereof is strictly prohibited.  If you have received this communication in error, please notify me by reply e-mail, permanently delete this communication from your system, and destroy any hard copies you may have printed.

Re: [PATCH] Don't attach needless options when launch pygrub

[Ian Campbell]

On Wed, 2010-07-14 at 10:36 +0100, Yu Zhiguo wrote:
> Ian Campbell wrote:
> > 
> > As far as I can see the --kernel and --ramdisk options end up in the
> > incfg map which only used in a handful of places, most of which just
> > extract incfg["args"]. The only places which do not do this are the
> > calls to sniff_solaris and sniff_netware both of which appear to make
> > use of incfg["kernel"] (but not incfg["ramdisk"]).
> > 
> > So it looks like specifying the kernel option in addition to bootloader
> > is infact useful if you are booting a Solaris or Netware domU but is
> > harmless/ignored otherwise. I think we need to continue to support this
> 
>  It seems that incfg will be returned directly if DomU is not Solaris,
> 
> def sniff_solaris(fs, cfg):
>     if not fs.file_exists("/platform/i86xpv/kernel/unix"):
>         return cfg
> 
> 
>     chosencfg = sniff_solaris(fs, incfg)
> 
>  So, incfg change to chosencfg and then will be used.

Oh yes, this stuff seems needlessly complex, or at least prone to
misreading, by me at least ;-)

You are right that chosencfg will be updated by sniff_solaris to be the
same as incfg if Solaris is not present (similarly for netware). 
Eventually we do "fs.open_file(chosencfg["kernel"]).read()" i.e. the
specified kernel from incfg will be read from the guest filesystem, as I
first expected.

All this is long standing behaviour, remind me why do you want to change
it?
[...]
>  What about copy the specified 'kernel' from DomU to a temp file.
> If there are 'bootloader' but no 'kernel', pygrub will copy and create temp file.
> We can do the same things.

I think this is the current behaviour.

Ian.

Re: [PATCH] Don't attach needless options when launch pygrub

[Yu Zhiguo]

Ian Campbell wrote:
>>> So it looks like specifying the kernel option in addition to bootloader
>>> is infact useful if you are booting a Solaris or Netware domU but is
>>> harmless/ignored otherwise. I think we need to continue to support this
>>  It seems that incfg will be returned directly if DomU is not Solaris,
>>
>> def sniff_solaris(fs, cfg):
>>     if not fs.file_exists("/platform/i86xpv/kernel/unix"):
>>         return cfg
>>
>>
>>     chosencfg = sniff_solaris(fs, incfg)
>>
>>  So, incfg change to chosencfg and then will be used.
> 
> Oh yes, this stuff seems needlessly complex, or at least prone to
> misreading, by me at least ;-)
> 

 OK, I'll try to fix this point.

>>  What about copy the specified 'kernel' from DomU to a temp file.
>> If there are 'bootloader' but no 'kernel', pygrub will copy and create temp file.
>> We can do the same things.
> 
> I think this is the current behaviour.
> 
> Ian.
> 

 Oh, yes. It seems that path of specified 'kernel' will be checked 
in configure_image(). We can delete this check if 'bootloader' is
existent.
 What's your opinion?


Yu

> 
> 
> 
> 

Re: [PATCH] Don't attach needless options when launch pygrub

[Yu Zhiguo]

Yu Zhiguo wrote:
> Ian Campbell wrote:
>>>> So it looks like specifying the kernel option in addition to bootloader
>>>> is infact useful if you are booting a Solaris or Netware domU but is
>>>> harmless/ignored otherwise. I think we need to continue to support this
>>>  It seems that incfg will be returned directly if DomU is not Solaris,
>>>
>>> def sniff_solaris(fs, cfg):
>>>     if not fs.file_exists("/platform/i86xpv/kernel/unix"):
>>>         return cfg
>>>
>>>
>>>     chosencfg = sniff_solaris(fs, incfg)
>>>
>>>  So, incfg change to chosencfg and then will be used.
>> Oh yes, this stuff seems needlessly complex, or at least prone to
>> misreading, by me at least ;-)
>>
> 
>  OK, I'll try to fix this point.
> 
>>>  What about copy the specified 'kernel' from DomU to a temp file.
>>> If there are 'bootloader' but no 'kernel', pygrub will copy and create temp file.
>>> We can do the same things.
>> I think this is the current behaviour.
>>
>> Ian.
>>
> 
>  Oh, yes. It seems that path of specified 'kernel' will be checked 
> in configure_image(). We can delete this check if 'bootloader' is
> existent.

delete -> not do.
Because in this case, the path of 'kernel' is in DomU rather than Dom0.
Otherwise, xm create will break here.

Yu

Re: [PATCH] Don't attach needless options when launch pygrub

[Ian Campbell]

On Wed, 2010-07-14 at 11:07 +0100, Yu Zhiguo wrote:
> 
>  Oh, yes. It seems that path of specified 'kernel' will be checked 
> in configure_image(). We can delete this check if 'bootloader' is
> existent.

That's only for kernel specified on the xm command line, not from the
configuration file, isn't it?

I just confirmed that for a guest with grub configuration entries for
2.6.32-5-686-bigmem and 2.6.26-2-686-bigmem (in that order) then

        bootloader = "/usr/bin/pygrub"
        bootloader_args = "--quiet"
        
        kernel = "/boot/vmlinuz-2.6.26-2-686-bigmem"
        ramdisk = "/boot/initrd.img-2.6.26-2-686-bigmem"
        
will boot 2.6.26-2-686-bigmem whereas without the kernel and ramdisk
lines it will boot 2.6.32-5-686-bigmem.

>  What's your opinion?

This is long standing behaviour of the toolstack and although it's a
little odd it's not totally useless (it could be very useful if you
botch you grub configuration for example).

I'll ask again, what do you think is wrong with the current behaviour
and what are the benefits to changing it?

Ian.

Re: [PATCH] Don't attach needless options when launch pygrub

[Yu Zhiguo]

Ian Campbell wrote:
> On Wed, 2010-07-14 at 11:07 +0100, Yu Zhiguo wrote:
>>  Oh, yes. It seems that path of specified 'kernel' will be checked 
>> in configure_image(). We can delete this check if 'bootloader' is
>> existent.
> 
> That's only for kernel specified on the xm command line, not from the
> configuration file, isn't it?
> 
> I just confirmed that for a guest with grub configuration entries for
> 2.6.32-5-686-bigmem and 2.6.26-2-686-bigmem (in that order) then
> 
>         bootloader = "/usr/bin/pygrub"
>         bootloader_args = "--quiet"
>         
>         kernel = "/boot/vmlinuz-2.6.26-2-686-bigmem"
>         ramdisk = "/boot/initrd.img-2.6.26-2-686-bigmem"
>         
> will boot 2.6.26-2-686-bigmem whereas without the kernel and ramdisk
> lines it will boot 2.6.32-5-686-bigmem.
> 

I guess this DomU's grub.conf must has "kernel /boot/vmlinuz-2.6.26-2-686-bigmem".

In other words, path of specified 'kernel' must be existent and *same* in both Dom0 and DomU.
This is a problem, it is difficult for using.

I want to resolve this by using 'kernel' as DomU's kernel path, so it should not
check it is existent or not in Dom0.


Yu


>>  What's your opinion?
> 
> This is long standing behaviour of the toolstack and although it's a
> little odd it's not totally useless (it could be very useful if you
> botch you grub configuration for example).
> 
> I'll ask again, what do you think is wrong with the current behaviour
> and what are the benefits to changing it?
> 
> Ian.
> 
> 
> 
> 
> 
> 
> 

Re: [PATCH] Don't attach needless options when launch pygrub

[Ian Campbell]

On Wed, 2010-07-14 at 12:01 +0100, Yu Zhiguo wrote:
> Ian Campbell wrote:
> > On Wed, 2010-07-14 at 11:07 +0100, Yu Zhiguo wrote:
> >>  Oh, yes. It seems that path of specified 'kernel' will be checked 
> >> in configure_image(). We can delete this check if 'bootloader' is
> >> existent.
> > 
> > That's only for kernel specified on the xm command line, not from the
> > configuration file, isn't it?
> > 
> > I just confirmed that for a guest with grub configuration entries for
> > 2.6.32-5-686-bigmem and 2.6.26-2-686-bigmem (in that order) then
> > 
> >         bootloader = "/usr/bin/pygrub"
> >         bootloader_args = "--quiet"
> >         
> >         kernel = "/boot/vmlinuz-2.6.26-2-686-bigmem"
> >         ramdisk = "/boot/initrd.img-2.6.26-2-686-bigmem"
> >         
> > will boot 2.6.26-2-686-bigmem whereas without the kernel and ramdisk
> > lines it will boot 2.6.32-5-686-bigmem.
> > 
> 
> I guess this DomU's grub.conf must has "kernel /boot/vmlinuz-2.6.26-2-686-bigmem".
> 
> In other words, path of specified 'kernel' must be existent and *same* in both Dom0 and DomU.
> This is a problem, it is difficult for using.

No, in my domain 0:

# ls /boot/vmlinuz-2.6.26-2-686-bigmem
ls: cannot access /boot/vmlinuz-2.6.26-2-686-bigmem: No such file or directory
# ls /boot/vmlinuz-2.6.32-5-686-bigmem
ls: cannot access /boot/vmlinuz-2.6.32-5-686-bigmem: No such file or directory

However I've just remembered that I am using xl so maybe the behaviour
is different. If xend is checking for the kernels existence in dom0 and
therefore breaking this behaviour then perhaps that is worth changing.

Ian.

> I want to resolve this by using 'kernel' as DomU's kernel path, so it should not
> check it is existent or not in Dom0.

> 
> 
> Yu
> 
> 
> >>  What's your opinion?
> > 
> > This is long standing behaviour of the toolstack and although it's a
> > little odd it's not totally useless (it could be very useful if you
> > botch you grub configuration for example).
> > 
> > I'll ask again, what do you think is wrong with the current behaviour
> > and what are the benefits to changing it?
> > 
> > Ian.
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> 

Re: [PATCH] Don't attach needless options when launch pygrub

[Yu Zhiguo]

Ian Campbell wrote:
> On Wed, 2010-07-14 at 12:01 +0100, Yu Zhiguo wrote:
>> Ian Campbell wrote:
>>> On Wed, 2010-07-14 at 11:07 +0100, Yu Zhiguo wrote:
>>>>  Oh, yes. It seems that path of specified 'kernel' will be checked 
>>>> in configure_image(). We can delete this check if 'bootloader' is
>>>> existent.
>>> That's only for kernel specified on the xm command line, not from the
>>> configuration file, isn't it?
>>>
>>> I just confirmed that for a guest with grub configuration entries for
>>> 2.6.32-5-686-bigmem and 2.6.26-2-686-bigmem (in that order) then
>>>
>>>         bootloader = "/usr/bin/pygrub"
>>>         bootloader_args = "--quiet"
>>>         
>>>         kernel = "/boot/vmlinuz-2.6.26-2-686-bigmem"
>>>         ramdisk = "/boot/initrd.img-2.6.26-2-686-bigmem"
>>>         
>>> will boot 2.6.26-2-686-bigmem whereas without the kernel and ramdisk
>>> lines it will boot 2.6.32-5-686-bigmem.
>>>
>> I guess this DomU's grub.conf must has "kernel /boot/vmlinuz-2.6.26-2-686-bigmem".
>>
>> In other words, path of specified 'kernel' must be existent and *same* in both Dom0 and DomU.
>> This is a problem, it is difficult for using.
> 
> No, in my domain 0:
> 
> # ls /boot/vmlinuz-2.6.26-2-686-bigmem
> ls: cannot access /boot/vmlinuz-2.6.26-2-686-bigmem: No such file or directory
> # ls /boot/vmlinuz-2.6.32-5-686-bigmem
> ls: cannot access /boot/vmlinuz-2.6.32-5-686-bigmem: No such file or directory
> 

yes. But what about menu.lst?

for example, my menu.lst has '/vmlinuz-2.6.31.5-127.fc12.i686.PAE' but 
in fact it is /boot/vmlinuz-2.6.31.5-127.fc12.i686.PAE.


> However I've just remembered that I am using xl so maybe the behaviour
> is different. If xend is checking for the kernels existence in dom0 and
> therefore breaking this behaviour then perhaps that is worth changing.
> 
I think so.
xl now cannot use 'bootloader' format (before your patchs),
it just use 'kernel + ramdisk' format, so it should check 'kernel'
is existent in Dom0 or not.

But xm can use 'bootloader' format, in this format, 'kernel' should not be
check in Dom0 because it is path in DomU.


Yu

> Ian.
> 
>> I want to resolve this by using 'kernel' as DomU's kernel path, so it should not
>> check it is existent or not in Dom0.
> 
>>
>> Yu
>>
>>
>>>>  What's your opinion?
>>> This is long standing behaviour of the toolstack and although it's a
>>> little odd it's not totally useless (it could be very useful if you
>>> botch you grub configuration for example).
>>>
>>> I'll ask again, what do you think is wrong with the current behaviour
>>> and what are the benefits to changing it?
>>>
>>> Ian.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
> 
> 
> 
> 
> 

Re: [PATCH] Don't attach needless options when launch pygrub

[Ian Campbell]

On Wed, 2010-07-14 at 12:21 +0100, Yu Zhiguo wrote:
> Ian Campbell wrote:
> > On Wed, 2010-07-14 at 12:01 +0100, Yu Zhiguo wrote:
> >> Ian Campbell wrote:
> >>> On Wed, 2010-07-14 at 11:07 +0100, Yu Zhiguo wrote:
> >>>>  Oh, yes. It seems that path of specified 'kernel' will be checked 
> >>>> in configure_image(). We can delete this check if 'bootloader' is
> >>>> existent.
> >>> That's only for kernel specified on the xm command line, not from the
> >>> configuration file, isn't it?
> >>>
> >>> I just confirmed that for a guest with grub configuration entries for
> >>> 2.6.32-5-686-bigmem and 2.6.26-2-686-bigmem (in that order) then
> >>>
> >>>         bootloader = "/usr/bin/pygrub"
> >>>         bootloader_args = "--quiet"
> >>>         
> >>>         kernel = "/boot/vmlinuz-2.6.26-2-686-bigmem"
> >>>         ramdisk = "/boot/initrd.img-2.6.26-2-686-bigmem"
> >>>         
> >>> will boot 2.6.26-2-686-bigmem whereas without the kernel and ramdisk
> >>> lines it will boot 2.6.32-5-686-bigmem.
> >>>
> >> I guess this DomU's grub.conf must has "kernel /boot/vmlinuz-2.6.26-2-686-bigmem".
> >>
> >> In other words, path of specified 'kernel' must be existent and *same* in both Dom0 and DomU.
> >> This is a problem, it is difficult for using.
> > 
> > No, in my domain 0:
> > 
> > # ls /boot/vmlinuz-2.6.26-2-686-bigmem
> > ls: cannot access /boot/vmlinuz-2.6.26-2-686-bigmem: No such file or directory
> > # ls /boot/vmlinuz-2.6.32-5-686-bigmem
> > ls: cannot access /boot/vmlinuz-2.6.32-5-686-bigmem: No such file or directory
> > 
> 
> yes. But what about menu.lst?

It contains /boot/vmlinuz-etcetc

> I think so.
> xl now cannot use 'bootloader' format (before your patchs),
> it just use 'kernel + ramdisk' format, so it should check 'kernel'
> is existent in Dom0 or not.
> 
> But xm can use 'bootloader' format, in this format, 'kernel' should not be
> check in Dom0 because it is path in DomU.

Correct.

> 
> 
> Yu
> 
> > Ian.
> > 
> >> I want to resolve this by using 'kernel' as DomU's kernel path, so it should not
> >> check it is existent or not in Dom0.
> > 
> >>
> >> Yu
> >>
> >>
> >>>>  What's your opinion?
> >>> This is long standing behaviour of the toolstack and although it's a
> >>> little odd it's not totally useless (it could be very useful if you
> >>> botch you grub configuration for example).
> >>>
> >>> I'll ask again, what do you think is wrong with the current behaviour
> >>> and what are the benefits to changing it?
> >>>
> >>> Ian.
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> > 
> > 
> > 
> > 
> > 
> 

[PATCH] xm: needless to check 'kernel/ramdisk' is existent or not

[Yu Zhiguo]

Hi,

Ian Campbell wrote:
>> xl now cannot use 'bootloader' format (before your patchs),
>> it just use 'kernel + ramdisk' format, so it should check 'kernel'
>> is existent in Dom0 or not.
>>
>> But xm can use 'bootloader' format, in this format, 'kernel' should not be
>> check in Dom0 because it is path in DomU.
> 
> Correct.
> 

Just ignore path check of kernel if bootloader is specified can fix this problem.
Please refer to this patch.

Yu
------------

When create DomU, if bootloader is specified,
'kernel/ramdisk' will be used by bootloader when
boots DomU. So it is needless to check the path
is existent or not.

Signed-off-by: Yu Zhiguo <yuzg@cn.fujitsu.com>

diff -r d867eb643fe4 -r 05e57f4db35d tools/python/xen/xm/create.py
--- a/tools/python/xen/xm/create.py	Tue Jul 13 18:17:28 2010 +0100
+++ b/tools/python/xen/xm/create.py	Thu Jul 15 18:33:04 2010 +0800
@@ -708,7 +708,12 @@
         return None
     config_image = [ vals.builder ]
     if vals.kernel:
-        if os.path.dirname(vals.kernel) != "" and os.path.exists(vals.kernel):
+        if vals.bootloader:
+            # If bootloader is specified, vals.kernel will be used
+            # by bootloader when boots DomU. So it is needless to
+            # check the path is existent or not.
+            config_image.append([ 'kernel', vals.kernel ])
+        elif os.path.dirname(vals.kernel) != "" and os.path.exists(vals.kernel):
             config_image.append([ 'kernel', vals.kernel ])
         elif vals.kernel == 'hvmloader':
             # Keep hvmloader w/o a path and let xend find it.
@@ -721,7 +726,10 @@
         else:
             raise ValueError('Cannot find kernel "%s"' % vals.kernel)
     if vals.ramdisk:
-        if os.path.dirname(vals.ramdisk) != "" and os.path.exists(vals.ramdisk):
+        if vals.bootloader:
+            # Same with 'kernel' above
+            config_image.append([ 'ramdisk', vals.ramdisk ])
+        elif os.path.dirname(vals.ramdisk) != "" and os.path.exists(vals.ramdisk):
             config_image.append([ 'ramdisk', vals.ramdisk ])
         elif os.path.exists(os.path.abspath(vals.ramdisk)):
             # Keep old behaviour, if path is valid.

Re: [PATCH] xm: needless to check 'kernel/ramdisk' is existent or not

[Ian Campbell]

On Thu, 2010-07-15 at 03:37 +0100, Yu Zhiguo wrote:
> When create DomU, if bootloader is specified,
> 'kernel/ramdisk' will be used by bootloader when
> boots DomU. So it is needless to check the path
> is existent or not.
> 
> Signed-off-by: Yu Zhiguo <yuzg@cn.fujitsu.com>

Thanks Yu.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> diff -r d867eb643fe4 -r 05e57f4db35d tools/python/xen/xm/create.py
> --- a/tools/python/xen/xm/create.py	Tue Jul 13 18:17:28 2010 +0100
> +++ b/tools/python/xen/xm/create.py	Thu Jul 15 18:33:04 2010 +0800
> @@ -708,7 +708,12 @@
>          return None
>      config_image = [ vals.builder ]
>      if vals.kernel:
> -        if os.path.dirname(vals.kernel) != "" and os.path.exists(vals.kernel):
> +        if vals.bootloader:
> +            # If bootloader is specified, vals.kernel will be used
> +            # by bootloader when boots DomU. So it is needless to
> +            # check the path is existent or not.
> +            config_image.append([ 'kernel', vals.kernel ])
> +        elif os.path.dirname(vals.kernel) != "" and os.path.exists(vals.kernel):
>              config_image.append([ 'kernel', vals.kernel ])
>          elif vals.kernel == 'hvmloader':
>              # Keep hvmloader w/o a path and let xend find it.
> @@ -721,7 +726,10 @@
>          else:
>              raise ValueError('Cannot find kernel "%s"' % vals.kernel)
>      if vals.ramdisk:
> -        if os.path.dirname(vals.ramdisk) != "" and os.path.exists(vals.ramdisk):
> +        if vals.bootloader:
> +            # Same with 'kernel' above
> +            config_image.append([ 'ramdisk', vals.ramdisk ])
> +        elif os.path.dirname(vals.ramdisk) != "" and os.path.exists(vals.ramdisk):
>              config_image.append([ 'ramdisk', vals.ramdisk ])
>          elif os.path.exists(os.path.abspath(vals.ramdisk)):
>              # Keep old behaviour, if path is valid.
> 
> 

Re: [PATCH] xm: needless to check 'kernel/ramdisk' is existent or not

[Ian Campbell]

On Thu, 2010-07-15 at 03:37 +0100, Yu Zhiguo wrote: 
> When create DomU, if bootloader is specified,
> 'kernel/ramdisk' will be used by bootloader when
> boots DomU. So it is needless to check the path
> is existent or not.
> 
> Signed-off-by: Yu Zhiguo <yuzg@cn.fujitsu.com>

Thanks Yu.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> diff -r d867eb643fe4 -r 05e57f4db35d tools/python/xen/xm/create.py
> --- a/tools/python/xen/xm/create.py	Tue Jul 13 18:17:28 2010 +0100
> +++ b/tools/python/xen/xm/create.py	Thu Jul 15 18:33:04 2010 +0800
> @@ -708,7 +708,12 @@
>          return None
>      config_image = [ vals.builder ]
>      if vals.kernel:
> -        if os.path.dirname(vals.kernel) != "" and os.path.exists(vals.kernel):
> +        if vals.bootloader:
> +            # If bootloader is specified, vals.kernel will be used
> +            # by bootloader when boots DomU. So it is needless to
> +            # check the path is existent or not.
> +            config_image.append([ 'kernel', vals.kernel ])
> +        elif os.path.dirname(vals.kernel) != "" and os.path.exists(vals.kernel):
>              config_image.append([ 'kernel', vals.kernel ])
>          elif vals.kernel == 'hvmloader':
>              # Keep hvmloader w/o a path and let xend find it.
> @@ -721,7 +726,10 @@
>          else:
>              raise ValueError('Cannot find kernel "%s"' % vals.kernel)
>      if vals.ramdisk:
> -        if os.path.dirname(vals.ramdisk) != "" and os.path.exists(vals.ramdisk):
> +        if vals.bootloader:
> +            # Same with 'kernel' above
> +            config_image.append([ 'ramdisk', vals.ramdisk ])
> +        elif os.path.dirname(vals.ramdisk) != "" and os.path.exists(vals.ramdisk):
>              config_image.append([ 'ramdisk', vals.ramdisk ])
>          elif os.path.exists(os.path.abspath(vals.ramdisk)):
>              # Keep old behaviour, if path is valid.
> 
> 

Re: [PATCH] Don't attach needless options when launch pygrub

[Zhigang Wang]

[-- Attachment #1: Type: text/plain, Size: 2542 bytes --]

On 07/14/2010 04:10 PM, Ian Campbell wrote:
> On Wed, 2010-07-14 at 08:29 +0100, Yu Zhiguo wrote:
>> Hi Ian,
>>
>> Ian Campbell wrote:
>>> On Wed, 2010-07-14 at 07:15 +0100, Yu Zhiguo wrote:
>>>> We should always run grub if bootloader is specified,
>>>> options 'kernel' and 'ramdisk' are needless.
>>>
>>> Not quite. If you specify both bootloader and kernel then this instructs
>>> pygrub to extract the specific named file from the guest file system,
>>> similarly for the ramdisk.
>>>
>>
>>  Do you mean in this case, pygrub will use specified kernel
>> that lie in the filesystem of the DomU?
> 
> I thought so, I looks like I was mistaken though.
> 
>>  I think this is good. But now pygrub's action is using the
>> specified kernel in Dom0, but not run grub.
> 
> Hmm, pygrub is certainly run, regardless of having a kernel configured
> or not. What is in question is what --kernel and --ramdisk actually
> cause pygrub to do and whether that is useful.
> 
> As far as I can see the --kernel and --ramdisk options end up in the
> incfg map which only used in a handful of places, most of which just
> extract incfg["args"]. The only places which do not do this are the
> calls to sniff_solaris and sniff_netware both of which appear to make
> use of incfg["kernel"] (but not incfg["ramdisk"]).
> 
> So it looks like specifying the kernel option in addition to bootloader
> is infact useful if you are booting a Solaris or Netware domU but is
> harmless/ignored otherwise. I think we need to continue to support this
> use case and I don't see any particular reason to force those users to
> change their configuration file syntax for this issue (if it's even an
> issue, I still don't really see the problem).
> 
> Perhaps it would be better to update pygrub so that --kernel actually
> does something consistent in the non-{Solaris,Netware} case, such as
> perhaps selecting the configuration entry with the match kernel path
> instead of defaulting to entry 0? (e.g. make "-q --kernel=/boot/FOO"
> select the entry with kernel /boot/FOO)
> 
> It looks like --ramdisk (and the associated plumbing through xend) may
> in fact be useless at this time. I'd say it is harmless to plumb it
> through for consistency though -- perhaps in the future pygrub (or
> another bootloader) might want to use it.
> 

Yes. Please don't remove --kernel/--ramdisk: pygrub is not the only pv guest
bootloader.

Here is a pv guest boot loader we are using which grab vmlinuz/initrd from
network. It uses --kernle/--ramdisk parameters.

Thanks,

Zhigang

[-- Attachment #2: xenpvboot-0.1.tar.gz --]
[-- Type: application/x-gzip, Size: 4063 bytes --]

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Don't attach needless options when launch pygrub

[Yu Zhiguo]

Hi,

Zhigang Wang wrote:
>>
>> It looks like --ramdisk (and the associated plumbing through xend) may
>> in fact be useless at this time. I'd say it is harmless to plumb it
>> through for consistency though -- perhaps in the future pygrub (or
>> another bootloader) might want to use it.
>>
> 
> Yes. Please don't remove --kernel/--ramdisk: pygrub is not the only pv guest
> bootloader.
> 
> Here is a pv guest boot loader we are using which grab vmlinuz/initrd from
> network. It uses --kernle/--ramdisk parameters.
> 
 I made a mistake here, sorry for that.
 I committed another patch and just ignore the kernel path check of Dom0
in 'xm create' when use bootloader, it works well.

-- 
Regards
Yu Zhiguo

> Thanks,
> 
> Zhigang