Re: [PATCH RFC] libxl: fix paths in capability string

Re: [PATCH RFC] libxl: fix paths in capability string

[Frediano Ziglio]

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
>

Re: [PATCH RFC] libxl: fix paths in capability string

[Wei Liu]

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.

Re: [PATCH RFC] libxl: fix paths in capability string

[Ian Campbell]

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.

Re: [PATCH RFC] libxl: fix paths in capability string

[Andrew Cooper]

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

Re: [PATCH RFC] libxl: fix paths in capability string

[Wei Liu]

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.

Re: [PATCH RFC] libxl: fix paths in capability string

[Ian Campbell]

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.

Re: [libvirt] [PATCH RFC] libxl: fix paths in capability string

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1377 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [libvirt] [PATCH RFC] libxl: fix paths in capability string

[Daniel P. Berrange]

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 :|

Re: [libvirt] [PATCH RFC] libxl: fix paths in capability string

[Wei Liu]

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

Re: [libvirt] [PATCH RFC] libxl: fix paths in capability string

[Wei Liu]

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

Re: [PATCH RFC] libxl: fix paths in capability string

[Wei Liu]

Jim, another idea: if those strings are likely to be wrong and in fact
not used, can we just not print them?

Wei.

Re: [PATCH RFC] libxl: fix paths in capability string

[Jim Fehlig]

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

[PATCH RFC] libxl: fix paths in capability string

[Wei Liu]

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