[libvirt] [PATCH RFC] libxl: fix paths in capability string
Currently libxl driver hardcodes some paths in its capability string, which might not be the correct paths. This patch introduces --with-libxl-prefix, so that user can specify the prefix used to build Xen tools. The default value is /usr/local which is the default --prefix for Xen tools. Change emualtor from qemu-dm to qemu-system-i386 because libxl (in Xen) use this as the default emulator. No need to check hostarch to determine whether to use lib or lib64 anymore because we now always use lib. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- This patch is RFC because I introduce --with-libxl-prefix and I don't know whether it's acceptable. Right now libvirt's driver doesn't seem to use these default paths so everything just works. But it would be nice for libvirt to report the correct paths instead of hardcoded (and possibly wrong) ones. --- configure.ac | 4 ++++ src/libxl/libxl_conf.c | 6 ++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 167b875..8d10361 100644 --- a/configure.ac +++ b/configure.ac @@ -533,6 +533,9 @@ AC_ARG_WITH([libxl], [AS_HELP_STRING([--with-libxl], [add libxenlight support @<:@default=check@:>@])]) m4_divert_text([DEFAULTS], [with_libxl=check]) +AC_ARG_WITH([libxl-prefix], [AS_HELP_STRING([--with-libxl-prefix=path], + [prefix used to build libxl, default /usr/local])], + [LIBXL_PREFIX=$withval], [LIBXL_PREFIX="/usr/local"]) AC_ARG_WITH([vbox], [AS_HELP_STRING([--with-vbox=@<:@PFX@:>@], [VirtualBox XPCOMC location @<:@default=yes@:>@])]) @@ -893,6 +896,7 @@ fi if test "$with_libxl" = "yes"; then AC_DEFINE_UNQUOTED([WITH_LIBXL], 1, [whether libxenlight driver is enabled]) + AC_DEFINE_UNQUOTED([LIBXL_PREFIX], ["$LIBXL_PREFIX"], [libxl prefix]) fi AM_CONDITIONAL([WITH_LIBXL], [test "$with_libxl" = "yes"]) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 0555b91..fbb4e29 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -428,11 +428,9 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) if ((guest = virCapabilitiesAddGuest(caps, guest_archs[i].hvm ? "hvm" : "xen", guest_archs[i].arch, - ((hostarch == VIR_ARCH_X86_64) ? - "/usr/lib64/xen/bin/qemu-dm" : - "/usr/lib/xen/bin/qemu-dm"), + LIBXL_PREFIX "/lib/xen/bin/qemu-system-i386", (guest_archs[i].hvm ? - "/usr/lib/xen/boot/hvmloader" : + LIBXL_PREFIX "/lib/xen/boot/hvmloader" : NULL), 1, machines)) == NULL) { -- 1.7.10.4
2015-01-06 16:12 GMT+00:00 Wei Liu <wei.liu2@citrix.com>:
Currently libxl driver hardcodes some paths in its capability string, which might not be the correct paths.
This patch introduces --with-libxl-prefix, so that user can specify the prefix used to build Xen tools. The default value is /usr/local which is the default --prefix for Xen tools.
Change emualtor from qemu-dm to qemu-system-i386 because libxl (in Xen) use this as the default emulator.
No need to check hostarch to determine whether to use lib or lib64 anymore because we now always use lib.
Wouldn't then better to separate it in a different patch? Also this make libvirt not compatible with former Xen versions. Is this expected? Frediano
Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- This patch is RFC because I introduce --with-libxl-prefix and I don't know whether it's acceptable.
Right now libvirt's driver doesn't seem to use these default paths so everything just works. But it would be nice for libvirt to report the correct paths instead of hardcoded (and possibly wrong) ones. --- configure.ac | 4 ++++ src/libxl/libxl_conf.c | 6 ++---- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac index 167b875..8d10361 100644 --- a/configure.ac +++ b/configure.ac @@ -533,6 +533,9 @@ AC_ARG_WITH([libxl], [AS_HELP_STRING([--with-libxl], [add libxenlight support @<:@default=check@:>@])]) m4_divert_text([DEFAULTS], [with_libxl=check]) +AC_ARG_WITH([libxl-prefix], [AS_HELP_STRING([--with-libxl-prefix=path], + [prefix used to build libxl, default /usr/local])], + [LIBXL_PREFIX=$withval], [LIBXL_PREFIX="/usr/local"]) AC_ARG_WITH([vbox], [AS_HELP_STRING([--with-vbox=@<:@PFX@:>@], [VirtualBox XPCOMC location @<:@default=yes@:>@])]) @@ -893,6 +896,7 @@ fi
if test "$with_libxl" = "yes"; then AC_DEFINE_UNQUOTED([WITH_LIBXL], 1, [whether libxenlight driver is enabled]) + AC_DEFINE_UNQUOTED([LIBXL_PREFIX], ["$LIBXL_PREFIX"], [libxl prefix]) fi AM_CONDITIONAL([WITH_LIBXL], [test "$with_libxl" = "yes"])
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 0555b91..fbb4e29 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -428,11 +428,9 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) if ((guest = virCapabilitiesAddGuest(caps, guest_archs[i].hvm ? "hvm" : "xen", guest_archs[i].arch, - ((hostarch == VIR_ARCH_X86_64) ? - "/usr/lib64/xen/bin/qemu-dm" : - "/usr/lib/xen/bin/qemu-dm"), + LIBXL_PREFIX "/lib/xen/bin/qemu-system-i386", (guest_archs[i].hvm ? - "/usr/lib/xen/boot/hvmloader" : + LIBXL_PREFIX "/lib/xen/boot/hvmloader" : NULL), 1, machines)) == NULL) { -- 1.7.10.4
On Tue, Jan 06, 2015 at 04:50:53PM +0000, Frediano Ziglio wrote:
2015-01-06 16:12 GMT+00:00 Wei Liu <wei.liu2@citrix.com>:
Currently libxl driver hardcodes some paths in its capability string, which might not be the correct paths.
This patch introduces --with-libxl-prefix, so that user can specify the prefix used to build Xen tools. The default value is /usr/local which is the default --prefix for Xen tools.
Change emualtor from qemu-dm to qemu-system-i386 because libxl (in Xen) use this as the default emulator.
No need to check hostarch to determine whether to use lib or lib64 anymore because we now always use lib.
Wouldn't then better to separate it in a different patch?
Also this make libvirt not compatible with former Xen versions. Is this expected?
No. I don't want to break compatibility. However these strings are not used in libxl driver so there's nothing to break. I'm not very familiar with libvirt so I could be wrong. Corrections welcome. :-/ Wei.
On Tue, 2015-01-06 at 16:12 +0000, Wei Liu wrote:
No need to check hostarch to determine whether to use lib or lib64 anymore because we now always use lib.
What about people who use --libdir=/path/to/lib64, as I believe Red Hat derived distros still do, don't they? Ian.
On 06/01/15 17:03, Ian Campbell wrote:
On Tue, 2015-01-06 at 16:12 +0000, Wei Liu wrote:
No need to check hostarch to determine whether to use lib or lib64 anymore because we now always use lib. What about people who use --libdir=/path/to/lib64, as I believe Red Hat derived distros still do, don't they?
Ian.
Redhat et al do indeed use /usr/lib64, although some of the recent autoconf work has moved some of the binaries into /usr/libexec. ~Andrew
On Tue, Jan 06, 2015 at 05:03:59PM +0000, Ian Campbell wrote:
On Tue, 2015-01-06 at 16:12 +0000, Wei Liu wrote:
No need to check hostarch to determine whether to use lib or lib64 anymore because we now always use lib.
What about people who use --libdir=/path/to/lib64, as I believe Red Hat derived distros still do, don't they?
Good point. I can't say for sure. If they do so we might need to add more --with-libxl-FOO? Essentially we need to export all dirs configured for Xen? Wei.
Ian.
On Tue, 2015-01-06 at 17:20 +0000, Wei Liu wrote:
On Tue, Jan 06, 2015 at 05:03:59PM +0000, Ian Campbell wrote:
On Tue, 2015-01-06 at 16:12 +0000, Wei Liu wrote:
No need to check hostarch to determine whether to use lib or lib64 anymore because we now always use lib.
What about people who use --libdir=/path/to/lib64, as I believe Red Hat derived distros still do, don't they?
Good point. I can't say for sure.
If they do so we might need to add more --with-libxl-FOO? Essentially we need to export all dirs configured for Xen?
From your patch it looks like only two matter, libdir/boot and libdir/bin. Or maybe configure $qemupath and hvmloaderpath directly?
Ian.
On 01/06/2015 09:12 AM, Wei Liu wrote:
Currently libxl driver hardcodes some paths in its capability string, which might not be the correct paths.
This patch introduces --with-libxl-prefix, so that user can specify the prefix used to build Xen tools. The default value is /usr/local which is the default --prefix for Xen tools.
Why can't you just make '--with-libxl=/path/to/alt' work? That is, --with-libxl=yes uses a default (which matches the prefix where LIBVIRT will be installed [1]), --with-libxl=no turns it off, and specifying a path says to use that path. Then you don't need to add a new option. [1] The assumption generally is that whoever is building libvirt has also built libxl to be installed in the same location - which is a nicer default than hard-coding a /usr/local default that libxl uses in isolation. Of course, libvirt also defaults to /usr/local in isolation, so if you don't specify anything at all, then ./configure will see that libvirt is going into /usr/local and will therefore default to looking for libxl also in /usr/local; but for a distro packager, it is nicer to have the mere specification of --prefix switch all other relative defaults to play nicely with everything else in the distro. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jan 06, 2015 at 10:28:13AM -0700, Eric Blake wrote:
On 01/06/2015 09:12 AM, Wei Liu wrote:
Currently libxl driver hardcodes some paths in its capability string, which might not be the correct paths.
This patch introduces --with-libxl-prefix, so that user can specify the prefix used to build Xen tools. The default value is /usr/local which is the default --prefix for Xen tools.
Why can't you just make '--with-libxl=/path/to/alt' work? That is, --with-libxl=yes uses a default (which matches the prefix where LIBVIRT will be installed [1]), --with-libxl=no turns it off, and specifying a path says to use that path. Then you don't need to add a new option.
Yep, that would be preferrable. Alternatively the next best would be for xen to include a pkg-config configuration file, with a custom variable that reports the paths required Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jan 06, 2015 at 05:36:57PM +0000, Daniel P. Berrange wrote:
On Tue, Jan 06, 2015 at 10:28:13AM -0700, Eric Blake wrote:
On 01/06/2015 09:12 AM, Wei Liu wrote:
Currently libxl driver hardcodes some paths in its capability string, which might not be the correct paths.
This patch introduces --with-libxl-prefix, so that user can specify the prefix used to build Xen tools. The default value is /usr/local which is the default --prefix for Xen tools.
Why can't you just make '--with-libxl=/path/to/alt' work? That is, --with-libxl=yes uses a default (which matches the prefix where LIBVIRT will be installed [1]), --with-libxl=no turns it off, and specifying a path says to use that path. Then you don't need to add a new option.
Yep, that would be preferrable.
Alternatively the next best would be for xen to include a pkg-config configuration file, with a custom variable that reports the paths required
This might be a useful thing in its own right. Thanks for the suggestion. Wei.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Tue, Jan 06, 2015 at 10:28:13AM -0700, Eric Blake wrote:
On 01/06/2015 09:12 AM, Wei Liu wrote:
Currently libxl driver hardcodes some paths in its capability string, which might not be the correct paths.
This patch introduces --with-libxl-prefix, so that user can specify the prefix used to build Xen tools. The default value is /usr/local which is the default --prefix for Xen tools.
Why can't you just make '--with-libxl=/path/to/alt' work? That is, --with-libxl=yes uses a default (which matches the prefix where LIBVIRT will be installed [1]), --with-libxl=no turns it off, and specifying a path says to use that path. Then you don't need to add a new option.
But --with-libxl is used to specify build time path which can be different from runtime path if I build libvirt against a non-installed version of xen. Consider I have a non-installed build of libxl in /local/scratch/xen.git/dist/install/usr/local but not /usr/local. I don't want that "/local/scratch..." end up in capability string. Wei.
[1] The assumption generally is that whoever is building libvirt has also built libxl to be installed in the same location - which is a nicer default than hard-coding a /usr/local default that libxl uses in isolation. Of course, libvirt also defaults to /usr/local in isolation, so if you don't specify anything at all, then ./configure will see that libvirt is going into /usr/local and will therefore default to looking for libxl also in /usr/local; but for a distro packager, it is nicer to have the mere specification of --prefix switch all other relative defaults to play nicely with everything else in the distro.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jim, another idea: if those strings are likely to be wrong and in fact not used, can we just not print them? Wei.
Wei Liu wrote:
Jim, another idea: if those strings are likely to be wrong and in fact not used, can we just not print them?
IMO they are useful, particularly when they are correct :-). They allow users to see which emulators are available and their complete path, which in turn can be directly used in the <emulator> element in domainXML. I do agree that a configure option for libvirt is the way to go here, allowing packagers to specify where these binaries exist. I recall discussions at a past Xen hackathon around the preference to use the distro's qemu instead of the one provided by Xen. A configure option for libvirt would facilitate that. Regards, Jim
participants (7)
-
Andrew Cooper -
Daniel P. Berrange -
Eric Blake -
Frediano Ziglio -
Ian Campbell -
Jim Fehlig -
Wei Liu