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

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

[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