| Message ID | 1470667204-12077-1-git-send-email-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Wei Liu writes ("[PATCH RFC] libxl: make firmware_override able to cope with relative path"):
> And also document that option in xl.cfg(5).
...
> -Select the virtual firmware that is exposed to the guest.
> +Select the virtual bios that is exposed to the guest.
> By default, a guess is made based on the device model, but sometimes
> it may be useful to request a different one, like UEFI.
hvmloader is surely not a `virtual bios' for two reasons: one is that
technically something like UEFI firmware is not a bios. The other is
that hvmloader is responsible for doing some other stuff too, AIUI ?
> +=item B<firmware_override="STRING">
> +
> +Override the virtual firmware provided by Xen. The value can be an
> +absolute or relative path to a file.
> +
> +If the value is a relative path, searching is frist done in current
> +working directory then Xen firmware directory.
> +
> +If not set, the default hvmloader is used.
I think we want a bit more justification, and consideration, here, at
least. This is changing libxl so that the cwd of the process at the
time libxl_domain_create is called is relevant. AFAICT it mostly
isn't right now ?
I looked at xl.cfg(5) and the use of the cwd is not documented for any
of the other parameters. There are a bunch of other libxl__abs_path
calls.
So err, I guess, I think I would like to see a wider consideration of
relative path semantics in libxl, and the corresponding docs.
> + if (info->u.hvm.firmware && !stat(firmware, &stab))
> + firmware_path = firmware;
Failure to check errno.
Ian.
On Mon, Aug 08, 2016 at 04:09:49PM +0100, Ian Jackson wrote: > Wei Liu writes ("[PATCH RFC] libxl: make firmware_override able to cope with relative path"): > > And also document that option in xl.cfg(5). > ... > > -Select the virtual firmware that is exposed to the guest. > > +Select the virtual bios that is exposed to the guest. > > By default, a guess is made based on the device model, but sometimes > > it may be useful to request a different one, like UEFI. > > hvmloader is surely not a `virtual bios' for two reasons: one is that > technically something like UEFI firmware is not a bios. The other is > that hvmloader is responsible for doing some other stuff too, AIUI ? > This section is for bios=. I think it is better to not use "firmware" to describe bios in the context of Xen. It's easy to confuse this with firmware_override. > > +=item B<firmware_override="STRING"> > > + > > +Override the virtual firmware provided by Xen. The value can be an > > +absolute or relative path to a file. > > + > > +If the value is a relative path, searching is frist done in current > > +working directory then Xen firmware directory. > > + > > +If not set, the default hvmloader is used. > > I think we want a bit more justification, and consideration, here, at > least. This is changing libxl so that the cwd of the process at the > time libxl_domain_create is called is relevant. AFAICT it mostly > isn't right now ? > Correct. > I looked at xl.cfg(5) and the use of the cwd is not documented for any > of the other parameters. There are a bunch of other libxl__abs_path > calls. > It isn't clear to me either, hence the RFC tag in this patch. Currently libxl doesn't care about kernel= or initrd= and passes them directly to libxc. Libxc then just opens the path so it effectively supports relative paths. But for firmware_override, libxl massages the path before passing to libxc. Libxl forbids relative paths in current code. > So err, I guess, I think I would like to see a wider consideration of > relative path semantics in libxl, and the corresponding docs. Yes, I agree. How about we decide that libxl will search for files in the following order if the string is not an absolute path: 1. current working directory 2. Xen specific directory (case by case, if applicable) And then we document, for each xl.cfg option, the search path. Also we encourage people to use absolute path for consistent results. Thoughts? Wei.
Wei Liu writes ("Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path"):
> On Mon, Aug 08, 2016 at 04:09:49PM +0100, Ian Jackson wrote:
> > Wei Liu writes ("[PATCH RFC] libxl: make firmware_override able to cope with relative path"):
> > > And also document that option in xl.cfg(5).
> > ...
> > > -Select the virtual firmware that is exposed to the guest.
> > > +Select the virtual bios that is exposed to the guest.
> > > By default, a guess is made based on the device model, but sometimes
> > > it may be useful to request a different one, like UEFI.
> >
> > hvmloader is surely not a `virtual bios' for two reasons: one is that
> > technically something like UEFI firmware is not a bios. The other is
> > that hvmloader is responsible for doing some other stuff too, AIUI ?
>
> This section is for bios=. I think it is better to not use "firmware" to
> describe bios in the context of Xen. It's easy to confuse this with
> firmware_override.
Oh! Yes, right, of course.
> Yes, I agree.
>
> How about we decide that libxl will search for files in the following
> order if the string is not an absolute path:
>
> 1. current working directory
> 2. Xen specific directory (case by case, if applicable)
>
> And then we document, for each xl.cfg option, the search path. Also we
> encourage people to use absolute path for consistent results.
SGTM.
I wonder if we should be able to specify to libxl to "please don't use
relative paths" (or even "relative paths are relative to this
specified location"). libxl might be embedded in another program
whose cwd can't be adjusted and shouldn't be relied on.
Ian.
On 08/08/16 16:49, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path"): >> On Mon, Aug 08, 2016 at 04:09:49PM +0100, Ian Jackson wrote: >>> Wei Liu writes ("[PATCH RFC] libxl: make firmware_override able to cope with relative path"): >>>> And also document that option in xl.cfg(5). >>> ... >>>> -Select the virtual firmware that is exposed to the guest. >>>> +Select the virtual bios that is exposed to the guest. >>>> By default, a guess is made based on the device model, but sometimes >>>> it may be useful to request a different one, like UEFI. >>> hvmloader is surely not a `virtual bios' for two reasons: one is that >>> technically something like UEFI firmware is not a bios. The other is >>> that hvmloader is responsible for doing some other stuff too, AIUI ? >> This section is for bios=. I think it is better to not use "firmware" to >> describe bios in the context of Xen. It's easy to confuse this with >> firmware_override. > Oh! Yes, right, of course. > >> Yes, I agree. >> >> How about we decide that libxl will search for files in the following >> order if the string is not an absolute path: >> >> 1. current working directory >> 2. Xen specific directory (case by case, if applicable) >> >> And then we document, for each xl.cfg option, the search path. Also we >> encourage people to use absolute path for consistent results. > SGTM. > > I wonder if we should be able to specify to libxl to "please don't use > relative paths" (or even "relative paths are relative to this > specified location"). libxl might be embedded in another program > whose cwd can't be adjusted and shouldn't be relied on. The important bit which XTF and regular users want is relative to the .cfg file, rather than $CWD. Imagine your files are layed out like: dir/ dir/vm1/vm1.cfg,vm1-kernel,vm1-ramdisk dir/vm2/vm2.cfg,vm2-kernel,vm2-ramdisk In this case, it would be natural to use relative paths in vm1.cfg, with no path components even. For kernel and ramdisks, `xl create` can use $CWD, /etc/xen/, or an absolute address For firmware_override, `xl create` is relative to the ./configure $(libexecdir), or an absolute address What XTF wants to use is xl create tests/foo/foo.cfg with foo.cfg referring to kernels or firmware_overrides in the same directory. ~Andrew
Andrew Cooper writes ("Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path"):
> The important bit which XTF and regular users want is relative to the
> .cfg file, rather than $CWD.
Consider:
cd /root/foo
xl create dir/vm1/vm1.cfg
cd /root/bar
xl block-attach vm1 vdev=xvdc target=file.img
Where do you think `file.img' should be searched for:
- /root/foo/dir/vm1/vm1.cfg
- /root/bar/dir/vm1/vm1.cfg
- /root/bar/file.img
? (4 marks)
In your answer, consider how your proposal would be implemented, given
that there may be xl, a qemu device model, and blkback, to consider.
Sketch how any necessary additional state would be stored in libxl.
(4 marks)
Discuss what should happen when the domain is subsequently migrated.
(4 marks)
Ian.
On 08/08/16 17:09, Ian Jackson wrote: > Andrew Cooper writes ("Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path"): >> The important bit which XTF and regular users want is relative to the >> .cfg file, rather than $CWD. > Consider: > > cd /root/foo > xl create dir/vm1/vm1.cfg > > cd /root/bar > xl block-attach vm1 vdev=xvdc target=file.img > > Where do you think `file.img' should be searched for: > > - /root/foo/dir/vm1/vm1.cfg > - /root/bar/dir/vm1/vm1.cfg > - /root/bar/file.img > > ? (4 marks) In this case, /root/bar/file.img and nowhere else. Once `xl create` is complete, the location of vm1.cfg on disk, or the $CWD used to build it are not relevant. The subsequent block-attach is therefore unrelated. > > In your answer, consider how your proposal would be implemented, given > that there may be xl, a qemu device model, and blkback, to consider. > Sketch how any necessary additional state would be stored in libxl. > (4 marks) N/A > > Discuss what should happen when the domain is subsequently migrated. > (4 marks) No change. In both of these cases, they are boot-time artefacts with no further action on migrate; they live in the memory stream at that point. ~Andrew
Andrew Cooper writes ("Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path"):
> On 08/08/16 17:09, Ian Jackson wrote:
> > Discuss what should happen when the domain is subsequently migrated.
> > (4 marks)
>
> No change.
>
> In both of these cases, they are boot-time artefacts with no further
> action on migrate; they live in the memory stream at that point.
My example was disk images.
Ian.
On 08/08/16 17:24, Ian Jackson wrote: > Andrew Cooper writes ("Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path"): >> On 08/08/16 17:09, Ian Jackson wrote: >>> Discuss what should happen when the domain is subsequently migrated. >>> (4 marks) >> No change. >> >> In both of these cases, they are boot-time artefacts with no further >> action on migrate; they live in the memory stream at that point. > My example was disk images. I fail to see why any question of disk image is relevant here. My statement was about kernel=, ramdisk= and firmware_override=. ~Andrew
Andrew Cooper writes ("Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path"):
> On 08/08/16 17:24, Ian Jackson wrote:
> > My example was disk images.
>
> I fail to see why any question of disk image is relevant here.
It is relevant because we want a coherent answer to "how does libxl
interpret relative pathnames in domain and device configuration".
Ian.
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in index 48c9c0d..f1fffbf 100644 --- a/docs/man/xl.cfg.pod.5.in +++ b/docs/man/xl.cfg.pod.5.in @@ -1189,7 +1189,7 @@ accept the defaults for these options wherever possible. =item B<bios="STRING"> -Select the virtual firmware that is exposed to the guest. +Select the virtual bios that is exposed to the guest. By default, a guess is made based on the device model, but sometimes it may be useful to request a different one, like UEFI. @@ -1214,6 +1214,16 @@ Requires device_model_version=qemu-xen. =back +=item B<firmware_override="STRING"> + +Override the virtual firmware provided by Xen. The value can be an +absolute or relative path to a file. + +If the value is a relative path, searching is frist done in current +working directory then Xen firmware directory. + +If not set, the default hvmloader is used. + =item B<pae=BOOLEAN> Hide or expose the IA32 Physical Address Extensions. These extensions diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index eef5045..f05aae3 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -905,8 +905,15 @@ static int libxl__domain_firmware(libxl__gc *gc, if (rc == 0 && info->ramdisk != NULL) rc = xc_dom_ramdisk_file(dom, info->ramdisk); } else { - rc = xc_dom_kernel_file(dom, libxl__abs_path(gc, firmware, - libxl__xenfirmwaredir_path())); + const char *firmware_path; + struct stat stab; + + if (info->u.hvm.firmware && !stat(firmware, &stab)) + firmware_path = firmware; + else + firmware_path = + libxl__abs_path(gc, firmware, libxl__xenfirmwaredir_path()); + rc = xc_dom_kernel_file(dom, firmware_path); } if (rc != 0) {
And also document that option in xl.cfg(5). Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> I suppose this should be able to make xtf free from absolute paths in config files. --- docs/man/xl.cfg.pod.5.in | 12 +++++++++++- tools/libxl/libxl_dom.c | 11 +++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-)