Re: [Xen-devel] [PATCH] tools: adjust --datadir option passed to qemu-upstream's configure script

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.06.12 at 12:54, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
wrote:
> On Fri, 8 Jun 2012, Jan Beulich wrote:
>> >>> On 08.06.12 at 13:42, Stefano Stabellini 
>> >>> <stefano.stabellini@xxxxxxxxxxxxx>
>> wrote:
>> > On Fri, 8 Jun 2012, Jan Beulich wrote:
>> >> >>> On 08.06.12 at 13:11, Stefano Stabellini 
>> >> >>> <stefano.stabellini@xxxxxxxxxxxxx>
>> >> wrote:
>> >> > On Mon, 4 Jun 2012, Jan Beulich wrote:
>> >> >> Passing $(SHAREDIR)/qemu-xen isn't compatible with the way
>> >> >> os-posix.c:os_find_datadir() works. While one would generally expect
>> >> >> that function to honour the --datadir configure option, fixing this is
>> >> >> quite a bit more involved than simply adjusting the configure options
>> >> >> to match the function's behavior (in that, for an installed binary, it
>> >> >> looks in $bindir/../share/qemu). Afaict, so far this can have worked
>> >> >> only by virtue of what CONFIG_QEMU_DATADIR points to being populated
>> >> >> due to (presumably) some other qemu instance being installed on
>> >> >> people's systems (but being absent when running qemu-system-i386 from
>> >> >> the dist/ portion of the build tree).
>> >> >> 
>> >> >> Signed-off-by: Jan Beulich <JBeulich@xxxxxxxx>
>> >> >> 
>> >> >> --- a/tools/Makefile
>> >> >> +++ b/tools/Makefile
>> >> >> @@ -155,7 +155,7 @@ subdir-all-qemu-xen-dir subdir-install-q
>> >> >>                --extra-ldflags="-L$(XEN_ROOT)/tools/libxc \
>> >> >>                -L$(XEN_ROOT)/tools/xenstore" \
>> >> >>                --bindir=$(LIBEXEC) \
>> >> >> -              --datadir=$(SHAREDIR)/qemu-xen \
>> >> >> +              --datadir=$(dir $(LIBEXEC))share/qemu \
>> >> >>                --disable-kvm \
>> >> >>                --python=$(PYTHON) \
>> >> >>                $(IOEMU_CONFIGURE_CROSS); \
>> >> >> 
>> >> >> 
>> >> >> 
>> >> >> 
>> >> > 
>> >> > This is a QEMU bug, so I would rather fix it there.
>> >> 
>> >> That's not as simple as you appear to think, as it would be necessary
>> >> to meaningfully express an arbitrary datadir relative to bindir.
>> >> 
>> >> > In fact the fix could be as simple as the appended patch.
>> >> > Does it work for you?
>> >> > 
>> >> > diff --git a/os-posix.c b/os-posix.c
>> >> > index dc4a6bb..d0c873d 100644
>> >> > --- a/os-posix.c
>> >> > +++ b/os-posix.c
>> >> > @@ -136,6 +136,9 @@ char *os_find_datadir(const char *argv0)
>> >> >              g_free(res);
>> >> >              res = NULL;
>> >> >          }
>> >> > +    } else {
>> >> > +        g_free(res);
>> >> > +        res = NULL;
>> >> 
>> >> How can this work? Afaict it would even break currently working
>> >> cases, as it now makes the function return NULL not only when
>> >> both access() checks failed, but also when the first one succeeded.
>> > 
>> > Yes, you are right, I misread the error condition of "access".
>> > 
>> > Like you wrote, os_find_datadir checks for the existence of
>> > $bindir/../share/qemu that normally means /usr/lib/xen/share/qemu, then
>> > it checks for /usr/lib/xen/pc-bios and if they both don't exist return
>> > NULL and data_dir is set to CONFIG_QEMU_DATADIR (that is what we want).
>> 
>> That is what you want in the installed case.
>> 
>> > While it is certainly not how I would have written this code, it should
>> > work correctly. How is that you system has /usr/lib/xen/pc-bios or
>> > /usr/lib/xen/share/qemu?
>> 
>> As I had written in the description, I'm after the non-installed
>> case instead (i.e. running from the build tree's dist/), and that
>> case can only be covered by either fixing os_find_datadir() or
>> forcing the value of --datadir to match the function's
>> expectations.
>> 
>> Additionally, for the installed case it doesn't matter where the
>> bits are put, as it'll always be the configured value that gets
>> used.
> 
> However your patch changes the location of data_dir for the
> installed case too, from /usr/share/qemu-xen to /usr/lib/xen/share/qemu.

Sure. But it doesn't matter where exactly the bits get put (and I
actually view the pace they're currently at as less consistent then
where they would end up with the patch, but admittedly that's
likely a matter of taste).

> If you are after the non-installed case, you must be already setting
> device_model_override in your VM config file to point to the right
> binary. You might as well pass:

No, I did not have to set anything like that - xl/libxl appear to
be figuring out quite fine where the binary is. Having to add such
or this ...

> device_model_args = ['-L', '/path/to/share/file' ]

... would additionally require the VM config files to be updated
each time I update to the tip of -unstable, as I'm having a date
tag somewhere in the path.

> that should solve your problem.

It certainly would, but at a price I consider too high.

Jan

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