[PATCH RFC] libxl: make firmware_override able to cope with relative path

[PATCH RFC] libxl: make firmware_override able to cope with relative path

[Wei Liu]

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(-)

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) {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path

[Wei Liu]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC] libxl: make firmware_override able to cope with relative path

[Ian Jackson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel