Skip to contentSkip to conversation

Don't attach needless options when launch pygrub

PATCH In review Yu Zhiguo 4 participants 18 messages Jul 14, 2010 – Jul 15, 2010

lore
Share & tools
lore

Yu Zhiguo

author
#0 lore raw

PATCH 1/1 Don't attach needless options when launch pygrub

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

Signed-off-by Yu Zhiguo <[email protected]>
tools/python/xen/xend/XendBootloader.py +1 -6
diff -r d867eb643fe4 -r 38d9dac1de56 tools/python/xen/xend/XendBootloader.py
@@ -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:
#1 lore raw
Quoted 2 lines · Yu Zhiguo

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.

Quoted 33 lines
Signed-off-by Yu Zhiguo <[email protected]>

diff -r d867eb643fe4 -r 38d9dac1de56 tools/python/xen/xend/XendBootloader.py

tools/python/xen/xend/XendBootloader.py +1 -6
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

author
#2 lore raw

Hi Ian,

Quoted 8 lines · Ian Campbell
Quoted 2 lines · Yu Zhiguo

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.

Quoted 4 lines

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.

Quoted 4 lines

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

Oh, sorry for this mistake.

Quoted 4 lines

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

Quoted 40 lines

Ian.

Quoted 32 lines
Signed-off-by Yu Zhiguo <[email protected]>

diff -r d867eb643fe4 -r 38d9dac1de56 tools/python/xen/xend/XendBootloader.py

tools/python/xen/xend/XendBootloader.py +1 -6
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

#3 lore raw
Quoted 7 lines · Yu Zhiguo

Hi Ian,

Quoted 4 lines · Ian Campbell
Quoted 2 lines · Yu Zhiguo

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

Quoted 7 lines
Quoted 4 lines

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.

Quoted 2 lines

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.

author
#4 lore raw
Quoted 10 lines · Ian Campbell

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?

Quoted 10 lines

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

Quoted 10 lines

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.

Signature · 14 lines
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: [email protected]
--------------------------------------------------
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.
#5 lore raw
Quoted 14 lines · Yu Zhiguo
Quoted 10 lines · Ian Campbell

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,

Quoted 8 lines

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?
[...]

Quoted 3 lines

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.

author
#6 lore raw
Quoted 10 lines · Ian Campbell
Quoted 10 lines
Quoted 3 lines

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

Quoted 7 lines
Quoted 3 lines

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.

Quoted 8 lines
Quoted 3 lines

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

Quoted 4 lines
author
#7 lore raw
Quoted 23 lines · Yu Zhiguo
Quoted 10 lines · Ian Campbell
Quoted 10 lines
Quoted 3 lines

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

Quoted 6 lines
Quoted 3 lines

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.

Quoted 3 lines
Quoted 3 lines

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.

Quoted 8 lines
Quoted 4 lines

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

#8 lore raw
In reply to Yu Zhiguo
Quoted 4 lines · Yu Zhiguo

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.

Quoted 1 line

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.

author
#9 lore raw
Quoted 11 lines · Ian Campbell
Quoted 3 lines · Yu Zhiguo

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

Quoted 9 lines

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

Quoted 17 lines
Quoted 1 line

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.

#10 lore raw
Quoted 18 lines · Yu Zhiguo
Quoted 17 lines · Ian Campbell
Quoted 3 lines · Yu Zhiguo

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"

Quoted 8 lines
Quoted 3 lines

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.

Quoted 26 lines

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

Quoted 17 lines
Quoted 1 line

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.

author
#11 lore raw
Quoted 26 lines · Ian Campbell
Quoted 17 lines · Yu Zhiguo
Quoted 16 lines · Ian Campbell
Quoted 3 lines · Yu Zhiguo

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"

Quoted 7 lines
Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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

Quoted 30 lines

Ian.

Quoted 23 lines

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

Quoted 16 lines
Quoted 1 line

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.

#12 lore raw
Quoted 29 lines · Yu Zhiguo
Quoted 28 lines · Ian Campbell
Quoted 17 lines · Yu Zhiguo
Quoted 16 lines · Ian Campbell
Quoted 3 lines · Yu Zhiguo

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"

Quoted 7 lines
Quoted 3 lines

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:

Quoted 7 lines
Quoted 5 lines
# 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

Quoted 7 lines

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.

Quoted 35 lines

Yu

Quoted 30 lines

Ian.

Quoted 23 lines

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

Quoted 16 lines
Quoted 1 line

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.

author
#13 lore raw

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

Hi,

Quoted 9 lines · Ian Campbell
Quoted 6 lines

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 <[email protected]>
tools/python/xen/xm/create.py +10 -2
diff -r d867eb643fe4 -r 05e57f4db35d tools/python/xen/xm/create.py
@@ -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.
Acked-by on v1 1/1
#14 lore raw

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

Quoted 6 lines · Yu Zhiguo

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 <[email protected]>

Thanks Yu.

Acked-by Ian Campbell <[email protected]>
Quoted 31 lines

diff -r d867eb643fe4 -r 05e57f4db35d tools/python/xen/xm/create.py

tools/python/xen/xm/create.py +10 -2
@@ -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.
Acked-by on v1 1/1
#15 lore raw

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

In reply to Yu Zhiguo
Quoted 6 lines · Yu Zhiguo

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 <[email protected]>

Thanks Yu.

Acked-by Ian Campbell <[email protected]>
Quoted 32 lines

diff -r d867eb643fe4 -r 05e57f4db35d tools/python/xen/xm/create.py

tools/python/xen/xm/create.py +10 -2
@@ -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.
#16 lore raw
In reply to Ian Campbell
Quoted 44 lines · Ian Campbell
Quoted 7 lines · Yu Zhiguo

Hi Ian,

Quoted 4 lines · Ian Campbell
Quoted 2 lines · Yu Zhiguo

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

Quoted 7 lines
Quoted 4 lines

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.

Quoted 2 lines

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)

Quoted 5 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

author
#17 lore raw

Hi,

Quoted 7 lines · Zhigang Wang
Quoted 6 lines

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.

Quoted 6 lines

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.

Signature · 2 lines
Regards
Yu Zhiguo
Quoted 3 lines

Thanks,

Zhigang