Yu Zhiguo
2010-Jul-14 06:15 UTC
[Xen-devel] [PATCH] 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 <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
Ian Campbell
2010-Jul-14 06:47 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu Zhiguo
2010-Jul-14 07:29 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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 > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Jul-14 08:10 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu Zhiguo
2010-Jul-14 09:36 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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 thisIt 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Jul-14 09:46 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu Zhiguo
2010-Jul-14 10:07 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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> > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu Zhiguo
2010-Jul-14 10:21 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Jul-14 10:22 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu Zhiguo
2010-Jul-14 11:01 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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. > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Jul-14 11:10 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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. > > > > > > > > > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu Zhiguo
2010-Jul-14 11:21 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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. >>> >>> >>> >>> >>> >>> >>> > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Jul-14 12:33 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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. > >>> > >>> > >>> > >>> > >>> > >>> > >>> > > > > > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu Zhiguo
2010-Jul-15 02:37 UTC
[Xen-devel] [PATCH] xm: needless to check ''kernel/ramdisk'' is existent or not
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Zhigang Wang
2010-Jul-15 03:44 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu Zhiguo
2010-Jul-15 05:03 UTC
Re: [Xen-devel] [PATCH] Don''t attach needless options when launch pygrub
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Jul-15 07:42 UTC
[Xen-devel] Re: [PATCH] xm: needless to check ''kernel/ramdisk'' is existent or not
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. > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Jul-15 07:56 UTC
[Xen-devel] Re: [PATCH] xm: needless to check ''kernel/ramdisk'' is existent or not
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. > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel