Re: [libvirt] [Xen-devel] [PATCH V2 0/4] libxl: support qemu's network-based block backends

[Ian Campbell]

On Wed, 2016-02-17 at 17:33 -0700, Jim Fehlig wrote:
> xl/libxl already supports qemu's network-based block backends
> such as nbd and rbd. libvirt has supported configuring network
> disks for long time too. This series marries the two in the
> libxl driver and in the xl<->xml converter. Only rbd supported
> is added in this series. Support for other backends such as nbd
> and iscsi can be added as a follow-up improvement.

All looks good to me, so FWIW:
Acked-by: Ian Campbell <ian.campb...@citrix.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH 0/4] libxl: support qemu's network-based block backends

[Ian Campbell]

On Tue, 2016-02-16 at 14:45 -0700, Jim Fehlig wrote:
> xl/libxl already supports qemu's network-based block backends
> such as nbd and rbd. libvirt has supported configuring network
> disks for long time too. This series marries the two in the
> libxl driver and in the xl<->xml converter. Only rbd supported
> is added in this series. Support for other backends such as nbd
> and iscsi can be added as a follow-up improvement.

This all looks sensible to me, FWIW.

One question, in patch 3's commit log should the example be double escaping
the \\ or not? Based on your updates to $xen/docs/misc/xl-disk-
configuration.txt (posted separately on xen-devel) I had expected they
would.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH LIBVIRT v3] libxl: Support cmdline= in xl config files

[Ian Campbell]

... and consolidate the cmdline/extra/root parsing to facilitate doing
so.

The logic is the same as xl's parse_cmdline from the current xen.git master
branch (e6f0e099d2c17de47fd86e817b1998db903cab61).

On the formatting side switch to producing cmdline= instead of extra=.

Update a few tests and add serveral more.
  - test-cmdline is added to test the exclusive use of cmdline.
  - test-fullvirt-direct-kernel-boot.cfg is updated due to the switch
on the formatting side and now tests the exclusive use of cmdline=.
  - Tests are added for both paravirt and fullvirt where the .cfg uses
extra= and (paravirt only) root=. These are format (xl->xml) only
since the inverse will generate cmdline= hence is not a round trip
(which was already true if using root=, which used to generate
extra= on the way back).
  - Tests are added for both paravirt and fullvirt where the .cfg
declares cmdline= as well as bogus extra= and (paravirt only) root=
entries which should be ignored. Again these are format only tests
since the inverse won't include the bogus lines.

The last two bullets here required splitting the DO_TEST macro into
two halves, as is done in the xmconfigtest.c case.

In order to introduce a use of VIR_WARN for logging I had to add
virerror.h and VIR_LOG_INIT.

Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
---
v2: Use VIR_INFO (adding necessary infra)
Don't initialise things to NULL when there is no need.
v3: I know know the answer re VIR_FROM_THIS, because Jim fixed it.
Initialise cmdline to NULL, since neither I nor gcc were smart
 enough to spot the uninitialised path I did this in preference to
 adding the else case, since that apparently won't be masking the
 compiler's ability to spot uninitialised vars in this function.
Add tests
Addjust xenFormatXLOS to produce cmdline= instead of extra=.
---
 src/xenconfig/xen_xl.c | 70 +-
 ...est-fullvirt-direct-kernel-boot-bogus-extra.cfg | 31 ++
 ...est-fullvirt-direct-kernel-boot-bogus-extra.xml | 51 
 .../test-fullvirt-direct-kernel-boot-extra.cfg | 30 ++
 .../test-fullvirt-direct-kernel-boot-extra.xml | 51 
 .../test-fullvirt-direct-kernel-boot.cfg   |  2 +-
 .../test-paravirt-cmdline-bogus-extra-root.cfg | 13 
 .../test-paravirt-cmdline-bogus-extra-root.xml | 32 ++
 .../test-paravirt-cmdline-extra-root.cfg   | 15 +
 .../test-paravirt-cmdline-extra-root.xml   | 32 ++
 tests/xlconfigdata/test-paravirt-cmdline.cfg   | 14 +
 tests/xlconfigdata/test-paravirt-cmdline.xml   | 32 ++
 tests/xlconfigtest.c   | 24 ++--
 13 files changed, 365 insertions(+), 32 deletions(-)
 create mode 100644 
tests/xlconfigdata/test-fullvirt-direct-kernel-boot-bogus-extra.cfg
 create mode 100644 
tests/xlconfigdata/test-fullvirt-direct-kernel-boot-bogus-extra.xml
 create mode 100644 
tests/xlconfigdata/test-fullvirt-direct-kernel-boot-extra.cfg
 create mode 100644 
tests/xlconfigdata/test-fullvirt-direct-kernel-boot-extra.xml
 create mode 100644 
tests/xlconfigdata/test-paravirt-cmdline-bogus-extra-root.cfg
 create mode 100644 
tests/xlconfigdata/test-paravirt-cmdline-bogus-extra-root.xml
 create mode 100644 tests/xlconfigdata/test-paravirt-cmdline-extra-root.cfg
 create mode 100644 tests/xlconfigdata/test-paravirt-cmdline-extra-root.xml
 create mode 100644 tests/xlconfigdata/test-paravirt-cmdline.cfg
 create mode 100644 tests/xlconfigdata/test-paravirt-cmdline.xml

diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 026cbcc..be194e3 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -27,6 +27,7 @@
 
 #include "virconf.h"
 #include "virerror.h"
+#include "virlog.h"
 #include "domain_conf.h"
 #include "viralloc.h"
 #include "virstring.h"
@@ -35,6 +36,8 @@
 
 #define VIR_FROM_THIS VIR_FROM_XENXL
 
+VIR_LOG_INIT("xen.xen_xl");
+
 /*
  * Xen provides a libxl utility library, with several useful functions,
  * specifically xlu_disk_parse for parsing xl disk config strings.
@@ -58,11 +61,46 @@ extern int xlu_disk_parse(XLU_Config *cfg,
   libxl_device_disk *disk);
 #endif
 
+static int xenParseCmdline(virConfPtr conf, char **r_cmdline)
+{
+char *cmdline = NULL;
+const char *root, *extra, *buf;
+
+if (xenConfigGetString(conf, "cmdline", , NULL) < 0)
+return -1;
+
+if (xenConfigGetString(conf, "root", , NULL) < 0)
+return -1;
+
+if (xenConfigGetString(conf, "extra", , NULL) < 0)
+return -1;
+
+if (buf) {
+if (VIR_STRDUP(cmdline, buf) < 0)
+return -1;
+if (root || extra)
+VIR_WARN("ignoring root= and extra= in favour of cmdline=");
+} else {
+

Re: [libvirt] [PATCH 2/2] Xen: add XENXL to virErrorDomain enum

[Ian Campbell]

On Wed, 2016-01-20 at 11:58 -0700, Jim Fehlig wrote:
> Add "Xen XL Config" to the virErrorDomain enum and use it in
> src/xenconfig/xen_xl.c.
> 
> Signed-off-by: Jim Fehlig <jfeh...@suse.com>

Acked-by: Ian Campbell <ian.campb...@citrix.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] Xen: VIR_FROM_THIS cleanup

[Ian Campbell]

On Wed, 2016-01-20 at 11:58 -0700, Jim Fehlig wrote:
> The virErrorDomain enum has VIR_FROM_XEN, VIR_FROM_XEND,
> VIR_FROM_XENSTORE, VIR_FROM_SEXPR, and VIR_FROM_XENXM. Use
> these elements in the corresponding .c files. While at it,
> remove the VIR_FROM_THIS define in src/xenconfig/xenxs_private.h.
> 
> Signed-off-by: Jim Fehlig <jfeh...@suse.com>

Acked-by: Ian Campbell <ian.campb...@citrix.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files

[Ian Campbell]

On Wed, 2016-01-20 at 11:16 -0700, Jim Fehlig wrote:
> Ian Campbell wrote:
> > ... and consolidate the cmdline/extra/root parsing to facilitate doing
> > so.
> > 
> > The logic is the same as xl's parse_cmdline from the current xen.git
> > master
> > branch (e6f0e099d2c17de47fd86e817b1998db903cab61).
> > 
> > In order to introduce a use of VIR_WARN for logging I had to add
> > virerror.h and VIR_LOG_INIT.
> > 
> > Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
> > ---
> > NB: I was unsure if I was supposed to change VIR_FROM_NONE into
> > VIR_FROM_XEN, so I didn't (but will on advice).
> 
> It seems some of the VIR_FROM_ needs cleanup throughout the various 
> Xen-related
> files. We already have VIR_FROM_SEXPR and VIR_FROM_XENXM.
> src/xenconfig/xen_sxpr.c should use the former, src/xenconfig/xen_xm.c the
> latter. And given the pattern, I think we should introduce VIR_FROM_XENXL to
> cover xl.cfg related code. I can take care of this.

I've acked you patches about this.

> > +static int xenParseCmdline(virConfPtr conf, char **r_cmdline)
> > +{
> > +char *cmdline;
> 
> One too many initializers removed :-).
> 
> > +const char *root, *extra, *buf;
> > +
> > +if (xenConfigGetString(conf, "cmdline", , NULL) < 0)
> > +return -1;
> > +
> > +if (xenConfigGetString(conf, "root", , NULL) < 0)
> > +return -1;
> > +
> > +if (xenConfigGetString(conf, "extra", , NULL) < 0)
> > +return -1;
> > +
> > +if (buf) {
> > +if (VIR_STRDUP(cmdline, buf) < 0)
> > +return -1;
> > +if (root || extra)
> > +VIR_WARN("ignoring root= and extra= in favour of
> > cmdline=");
> > +} else {
> > +if (root && extra) {
> > +if (virAsprintf(, "root=%s %s", root, extra) < 0)
> > +return -1;
> > +} else if (root) {
> > +if (virAsprintf(, "root=%s", root) < 0)
> > +return -1;
> > +} else if (extra) {
> > +if (VIR_STRDUP(cmdline, extra) < 0)
> > +return -1;
> > +}
> > +}
> > +
> > +*r_cmdline = cmdline;
> 
> If none of cmdline, extra, or root are set in the config, def->os.cmdline gets
> set to garbage. xlconfigtest explodes when running 'make check'.

I even thought about this quite carefully but missed this case :-/

Would you prefer and = NULL on the decl or an else cmdline = NULL at the
end of that chain?

> Sorry for not mentioning it in my previous review, but we should add a
> test for cmdline, root, and extra.

Ack, will do so for v3.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH LIBVIRT] libxl: Support cmdline= in xl config files

[Ian Campbell]

On Tue, 2016-01-19 at 21:46 -0700, Jim Fehlig wrote:
> On 01/19/2016 05:03 AM, Ian Campbell wrote:
> > I went to ping this but noticed that I had sent it to "jimfehlig" (i.e.
> > no
> > domain), so no wonder there was no reply!
> > 
> > To: line fixed here, let me know if you would prefer a resend.
> 
> That would be much appreciated, thanks!
> 
> > 
> > Ian.
> > 
> > On Wed, 2015-12-16 at 12:09 +, Ian Campbell wrote:
> > > ... and consolidate the cmdline/extra/root parsing to facilitate
> > > doing
> > > so.
> > > 
> > > The logic is the same as xl's parse_cmdline from the current xen.git
> > > master
> > > branch (e6f0e099d2c17de47fd86e817b1998db903cab61), except I was
> > > unable
> > > to figure out how/where to route the warning about ignoring
> > > root+extra if cmdline was specified.
> 
> I think VIR_WARN() would be appropriate.

Ok, will do, thanks.

> > > 
> > > Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
> > > ---
> > >  src/xenconfig/xen_xl.c | 62 ++
> > > 
> > > 
> > >  1 file changed, 37 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> > > index 91cdff6..ba8b938 100644
> > > --- a/src/xenconfig/xen_xl.c
> > > +++ b/src/xenconfig/xen_xl.c
> > > @@ -58,11 +58,45 @@ extern int xlu_disk_parse(XLU_Config *cfg,
> > >    libxl_device_disk *disk);
> > >  #endif
> > >  
> > > +static int xenParseCmdline(virConfPtr conf, char **r_cmdline)
> > > +{
> > > +char *cmdline = NULL;
> > > +const char *root = NULL, *extra = NULL, *buf = NULL;
> 
> In theory, these three don't need to be initialized since
> xenConfigGetString
> will do that. But in practice, I worry that Coverity might complain :-/.

It looks like some of the callers of xenConfigGetString initialise the
value to NULL, while others don't. I can't see any public libvirt scan
results to look if some of the ones which don't have been picked up or not.

I've just noticed also that the code I am moving/removing didn't initialise
to NULL, so I think I'll remove these initialisers.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files

[Ian Campbell]

... and consolidate the cmdline/extra/root parsing to facilitate doing
so.

The logic is the same as xl's parse_cmdline from the current xen.git master
branch (e6f0e099d2c17de47fd86e817b1998db903cab61).

In order to introduce a use of VIR_WARN for logging I had to add
virerror.h and VIR_LOG_INIT.

Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
---
NB: I was unsure if I was supposed to change VIR_FROM_NONE into
VIR_FROM_XEN, so I didn't (but will on advice).

v2: Use VIR_INFO (adding necessary infra)
Don't initialise things to NULL when there is no need.
---
 src/xenconfig/xen_xl.c | 66 +++---
 1 file changed, 41 insertions(+), 25 deletions(-)

diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 91cdff6..3d820cc 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -27,6 +27,7 @@
 
 #include "virconf.h"
 #include "virerror.h"
+#include "virlog.h"
 #include "domain_conf.h"
 #include "viralloc.h"
 #include "virstring.h"
@@ -35,6 +36,8 @@
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
+VIR_LOG_INIT("xen.xen_xl");
+
 /*
  * Xen provides a libxl utility library, with several useful functions,
  * specifically xlu_disk_parse for parsing xl disk config strings.
@@ -58,11 +61,46 @@ extern int xlu_disk_parse(XLU_Config *cfg,
   libxl_device_disk *disk);
 #endif
 
+static int xenParseCmdline(virConfPtr conf, char **r_cmdline)
+{
+char *cmdline;
+const char *root, *extra, *buf;
+
+if (xenConfigGetString(conf, "cmdline", , NULL) < 0)
+return -1;
+
+if (xenConfigGetString(conf, "root", , NULL) < 0)
+return -1;
+
+if (xenConfigGetString(conf, "extra", , NULL) < 0)
+return -1;
+
+if (buf) {
+if (VIR_STRDUP(cmdline, buf) < 0)
+return -1;
+if (root || extra)
+VIR_WARN("ignoring root= and extra= in favour of cmdline=");
+} else {
+if (root && extra) {
+if (virAsprintf(, "root=%s %s", root, extra) < 0)
+return -1;
+} else if (root) {
+if (virAsprintf(, "root=%s", root) < 0)
+return -1;
+} else if (extra) {
+if (VIR_STRDUP(cmdline, extra) < 0)
+return -1;
+}
+}
+
+*r_cmdline = cmdline;
+return 0;
+}
+
 static int
 xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
 {
 size_t i;
-const char *extra, *root;
 
 if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
 const char *boot;
@@ -84,19 +122,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, 
virCapsPtr caps)
 if (xenConfigCopyStringOpt(conf, "ramdisk", >os.initrd) < 0)
 return -1;
 
-if (xenConfigGetString(conf, "extra", , NULL) < 0)
-return -1;
-
-if (xenConfigGetString(conf, "root", , NULL) < 0)
+if (xenParseCmdline(conf, >os.cmdline) < 0)
 return -1;
-
-if (root) {
-if (virAsprintf(>os.cmdline, "root=%s %s", root, extra) < 0)
-return -1;
-} else {
-if (VIR_STRDUP(def->os.cmdline, extra) < 0)
-return -1;
-}
 #endif
 
 if (xenConfigGetString(conf, "boot", , "c") < 0)
@@ -132,19 +159,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, 
virCapsPtr caps)
 if (xenConfigCopyStringOpt(conf, "ramdisk", >os.initrd) < 0)
 return -1;
 
-if (xenConfigGetString(conf, "extra", , NULL) < 0)
-return -1;
-
-if (xenConfigGetString(conf, "root", , NULL) < 0)
+if (xenParseCmdline(conf, >os.cmdline) < 0)
 return -1;
-
-if (root) {
-if (virAsprintf(>os.cmdline, "root=%s %s", root, extra) < 0)
-return -1;
-} else {
-if (VIR_STRDUP(def->os.cmdline, extra) < 0)
-return -1;
-}
 }
 
 return 0;
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH LIBVIRT] libxl: Support cmdline= in xl config files

[Ian Campbell]

I went to ping this but noticed that I had sent it to "jimfehlig" (i.e. no
domain), so no wonder there was no reply!

To: line fixed here, let me know if you would prefer a resend.

Ian.

On Wed, 2015-12-16 at 12:09 +, Ian Campbell wrote:
> ... and consolidate the cmdline/extra/root parsing to facilitate doing
> so.
> 
> The logic is the same as xl's parse_cmdline from the current xen.git master
> branch (e6f0e099d2c17de47fd86e817b1998db903cab61), except I was unable
> to figure out how/where to route the warning about ignoring
> root+extra if cmdline was specified.
> 
> Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
> ---
>  src/xenconfig/xen_xl.c | 62 ++
> 
>  1 file changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index 91cdff6..ba8b938 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -58,11 +58,45 @@ extern int xlu_disk_parse(XLU_Config *cfg,
>    libxl_device_disk *disk);
>  #endif
>  
> +static int xenParseCmdline(virConfPtr conf, char **r_cmdline)
> +{
> +char *cmdline = NULL;
> +const char *root = NULL, *extra = NULL, *buf = NULL;
> +
> +if (xenConfigGetString(conf, "cmdline", , NULL) < 0)
> +return -1;
> +
> +if (xenConfigGetString(conf, "root", , NULL) < 0)
> +return -1;
> +
> +if (xenConfigGetString(conf, "extra", , NULL) < 0)
> +return -1;
> +
> +if (buf) {
> +if (VIR_STRDUP(cmdline, buf) < 0)
> +return -1;
> +/* root or extra are ignored in this case. */
> +} else {
> +if (root && extra) {
> +if (virAsprintf(, "root=%s %s", root, extra) < 0)
> +return -1;
> +} else if (root) {
> +if (virAsprintf(, "root=%s", root) < 0)
> +return -1;
> +} else if (extra) {
> +if (VIR_STRDUP(cmdline, extra) < 0)
> +return -1;
> +}
> +}
> +
> +*r_cmdline = cmdline;
> +return 0;
> +}
> +
>  static int
>  xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
>  {
>  size_t i;
> -const char *extra, *root;
>  
>  if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
>  const char *boot;
> @@ -84,19 +118,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def,
> virCapsPtr caps)
>  if (xenConfigCopyStringOpt(conf, "ramdisk", >os.initrd) <
> 0)
>  return -1;
>  
> -if (xenConfigGetString(conf, "extra", , NULL) < 0)
> -return -1;
> -
> -if (xenConfigGetString(conf, "root", , NULL) < 0)
> +if (xenParseCmdline(conf, >os.cmdline) < 0)
>  return -1;
> -
> -if (root) {
> -if (virAsprintf(>os.cmdline, "root=%s %s", root, extra)
> < 0)
> -return -1;
> -} else {
> -if (VIR_STRDUP(def->os.cmdline, extra) < 0)
> -return -1;
> -}
>  #endif
>  
>  if (xenConfigGetString(conf, "boot", , "c") < 0)
> @@ -132,19 +155,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def,
> virCapsPtr caps)
>  if (xenConfigCopyStringOpt(conf, "ramdisk", >os.initrd) <
> 0)
>  return -1;
>  
> -if (xenConfigGetString(conf, "extra", , NULL) < 0)
> -return -1;
> -
> -if (xenConfigGetString(conf, "root", , NULL) < 0)
> +if (xenParseCmdline(conf, >os.cmdline) < 0)
>  return -1;
> -
> -if (root) {
> -if (virAsprintf(>os.cmdline, "root=%s %s", root, extra)
> < 0)
> -return -1;
> -} else {
> -if (VIR_STRDUP(def->os.cmdline, extra) < 0)
> -return -1;
> -}
>  }
>  
>  return 0;

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2] Xen: support maxvcpus in xm and xl config

[Ian Campbell]

On Wed, 2015-12-16 at 10:11 +, Ian Campbell wrote:
> On Tue, 2015-12-15 at 15:20 -0700, Jim Fehlig wrote:
> > From: Ian Campbell <ian.campb...@citrix.com>
> > 
> > xend prior to 4.0 understands vcpus as maxvcpus and vcpu_avail
> > as a bit map of which cpus are online (default is all).
> > 
> > xend from 4.0 onwards understands maxvcpus as maxvcpus and
> > vcpus as the number which are online (from 0..N-1). The
> > upstream commit (68a94cf528e6 "xm: Add maxvcpus support")
> > claims that if maxvcpus is omitted then the old behaviour
> > (i.e. obeying vcpu_avail) is retained, but AFAICT it was not,
> > in this case vcpu==maxcpus==online cpus. This is good for us
> > because handling anything else would be fiddly.
> > 
> > This patch changes parsing of the virDomainDef maxvcpus and vcpus
> > entries to use the corresponding 'maxvcpus' and 'vcpus' settings
> > from xm and xl config. It also drops use of the old Xen 3.x
> > 'vcpu_avail' setting.
> > 
> > The change also removes the maxvcpus limit of MAX_VIRT_VCPUS (since
> > maxvcpus is simply a count, not a bit mask), which is particularly
> > crucial on ARM where MAX_VIRT_CPUS == 1 (since all guests are
> > expected to support vcpu placement, and therefore only the boot
> > vcpu's info lives in the shared info page).
> > 
> > Existing tests adjusted accordingly, and new tests added for the
> > 'maxvcpus' setting.
> > 
> > Signed-off-by: Jim Fehlig <jfeh...@suse.com>
> 
> Acked-by: Ian Campbell <ian.campb...@citrix.com>
> Tested-by: Ian Campbell <ian.campb...@citrix.com>
> (as far as "domxml-from-native xen-xl" goes, I seem to have another issue
> actually starting a domain on ARM, which I'll investigate...)

Turned out to be a mismatch between my build-time and run-time libxl
versions, fixed by a clean rebuild. So that's a full Tested-by.

I noticed that the newer cmdline= (inplace of root=+extra= etc) wasn't
supported. I'll knock something up.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/16] Xen: remove xend config version

[Ian Campbell]

On Tue, 2015-12-15 at 14:32 -0700, Jim Fehlig wrote:
> Hi All,
> 
> Ian Campbell recently attempted [1] to fix and issue around MAX_VIRT_VPUS
> on ARM that required adding a new XEND_CONFIG_VERSION. After some
> discussion [2] we decided to drop support for all of the old xend config
> versions and go with the version supported in Xen 4.0, since the xl syntax
> was originally based on (and intended to be compatible with) xm circa that
> point in time.
> 
> This series removes all traces of xend config version from the codebase,
> essentially removing support for Xen 3.x. Hopefully I succeeding in making
> the rather large series reviewable. The series is also available on the
> remove-xend-config-version branch of my libvirt github clone [2].

Wow, thanks for offering to take this over, I had no idea it would end up
with so much yakk hair everywhere!

Ian.

> 
> [1] https://www.redhat.com/archives/libvir-list/2015-November/msg01153.ht
> ml
> [2] https://www.redhat.com/archives/libvir-list/2015-December/msg00148.ht
> ml
> [3] https://github.com/jfehlig/libvirt/tree/remove-xend-config-version
> 
> Jim Fehlig (16):
>   Xen: tests: remove old xm config tests
>   Xen: tests: remove net-ioemu xm config test
>   Xen: tests: remove old sexpr2xml tests
>   Xen: tests: remove old xml2sexpr tests
>   Xen: tests: use latest XEND_CONFIG_VERSION in xm/xl tests
>   Xen: xenconfig: remove XEND_CONFIG_VERSION in common code
>   Xen: xenconfig: remove use of XEND_CONFIG_VERSION in xen_xm
>   Xen: xenconfig: remove xendConfigVersion from public functions
>   Xen: tests: use latest XEND_CONFIG_VERSION in sexpr2xml tests
>   Xen: xenconfig: remove disks from '(image)' sexpr
>   Xen: tests: use latest XEND_CONFIG_VERSION in xml2sexpr tests
>   Xen: xenconfig: remove use of XEND_CONFIG_VERSION in xen_sxpr
>   Xen: xen_driver: remove use of XEND_CONFIG_VERSION
>   Xen: xend: remove use of XEND_CONFIG_VERSION
>   Xen: xenconfig: remove xendConfigVersion from public sexpr functions
>   Xen: remove xendConfigVersion from driver private struct
> 
>  src/libxl/libxl_driver.c   |   9 +-
>  src/xen/xen_driver.c   | 296 ---
>  src/xen/xen_driver.h   |   2 -
>  src/xen/xend_internal.c| 224 ++-
>  src/xen/xm_internal.c  |   9 +-
>  src/xenconfig/xen_common.c | 211 ---
>  src/xenconfig/xen_common.h |   7 +-
>  src/xenconfig/xen_sxpr.c   | 411 ++-
> --
>  src/xenconfig/xen_sxpr.h   |  21 +-
>  src/xenconfig/xen_xl.c |   9 +-
>  src/xenconfig/xen_xl.h |   7 +-
>  src/xenconfig/xen_xm.c |  57 +--
>  src/xenconfig/xen_xm.h |   5 +-
>  src/xenconfig/xenxs_private.h  |   8 -
>  tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml  |   2 +-
>  .../sexpr2xmldata/sexpr2xml-fv-empty-kernel.sexpr  |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml  |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.sexpr  |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml|   4 +-
>  .../sexpr2xmldata/sexpr2xml-fv-force-nohpet.sexpr  |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml  |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml|   2 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-localtime.sexpr   |   3 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.sexpr   |   9 -
>  tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.xml |  48 ---
>  .../sexpr2xmldata/sexpr2xml-fv-net-netfront.sexpr  |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml  |   4 +-
>  .../sexpr2xmldata/sexpr2xml-fv-parallel-tcp.sexpr  |   3 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml  |   4 +-
>  .../sexpr2xml-fv-serial-dev-2-ports.sexpr  |   5 +-
>  .../sexpr2xml-fv-serial-dev-2-ports.xml|   4 +-
>  .../sexpr2xml-fv-serial-dev-2nd-port.sexpr |   4 +-
>  .../sexpr2xml-fv-serial-dev-2nd-port.xml   |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-serial-file.sexpr |   7 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml   |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-serial-null.sexpr |   3 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml   |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.sexpr |   7 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml   |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.sexpr  |   4 +-
>  tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml

Re: [libvirt] [Xen-devel] [PATCH LIBVIRT] libxl: Use libxentoollog in preference to libxenctrl if available.

[Ian Campbell]

On Tue, 2015-12-15 at 16:15 -0700, Jim Fehlig wrote:
> On 12/14/2015 04:37 AM, Ian Campbell wrote:
> > On Mon, 2015-12-14 at 11:15 +, Daniel P. Berrange wrote:
> > > On Thu, Dec 10, 2015 at 11:38:36AM +, Ian Campbell wrote:
> > > > Upstream Xen is in the process of splitting the (stable API) xtl_*
> > > > interfaces out from the (unstable API) libxenctrl library and into
> > > > a
> > > > new (stable API) libxentoollog.
> > > > 
> > > > In order to be compatible with Xen both before and after this
> > > > transition check for xtl_createlogger_stdiostream in a
> > > > libxentoollog
> > > > library and use it if present. If it is not present assume it is in
> > > > libxenctrl.
> > > Ok, so there's no API changes, just move stuf from one to the other.
> > Indeed, it should really have been a separate library all along and the
> > API
> > already setup that way.
> > 
> > I'm working on some other library splits, which will involve API
> > changes,
> > but AFAIK they are all isolated from libvirt via the use of libxl, so
> > there
> > should be no churn for you guys other than this one patch.
> > 
> > > > It might be nice to get this into 1.3.0 so that supports Xen 4.7
> > > > out
> > > > of the box? Not sure what the libvirt stable backport policy is but
> > > > it
> > > > might also be good to eventually consider it for that?
> > > We've missed 1.3.0 release, but I'd be ok with adding it to the
> > > stable branch if that's going to be useful.
> > I think it would, to allow things to build with Xen 4.7 (when it is
> > released).
> 
> I'm not sure it is necessary. libvirt is released monthly, so there will be
> several releases before Xen 4.7 is released.

AH, I didn't realise it was on such a fast cadence, that's ok then.

> > [...]
> > 
> > > Looks, fine from me but will let Jim push it.
> 
> I've pushed the patch to master, but not the 1.3.0 maint branch. It will be
> included in the 1.3.1 release planned for mid January. Ian, do you think that 
> is
> sufficient?

Easily, thanks.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2] Xen: support maxvcpus in xm and xl config

[Ian Campbell]

On Tue, 2015-12-15 at 15:20 -0700, Jim Fehlig wrote:
> From: Ian Campbell <ian.campb...@citrix.com>
> 
> xend prior to 4.0 understands vcpus as maxvcpus and vcpu_avail
> as a bit map of which cpus are online (default is all).
> 
> xend from 4.0 onwards understands maxvcpus as maxvcpus and
> vcpus as the number which are online (from 0..N-1). The
> upstream commit (68a94cf528e6 "xm: Add maxvcpus support")
> claims that if maxvcpus is omitted then the old behaviour
> (i.e. obeying vcpu_avail) is retained, but AFAICT it was not,
> in this case vcpu==maxcpus==online cpus. This is good for us
> because handling anything else would be fiddly.
> 
> This patch changes parsing of the virDomainDef maxvcpus and vcpus
> entries to use the corresponding 'maxvcpus' and 'vcpus' settings
> from xm and xl config. It also drops use of the old Xen 3.x
> 'vcpu_avail' setting.
> 
> The change also removes the maxvcpus limit of MAX_VIRT_VCPUS (since
> maxvcpus is simply a count, not a bit mask), which is particularly
> crucial on ARM where MAX_VIRT_CPUS == 1 (since all guests are
> expected to support vcpu placement, and therefore only the boot
> vcpu's info lives in the shared info page).
> 
> Existing tests adjusted accordingly, and new tests added for the
> 'maxvcpus' setting.
> 
> Signed-off-by: Jim Fehlig <jfeh...@suse.com>

Acked-by: Ian Campbell <ian.campb...@citrix.com>
Tested-by: Ian Campbell <ian.campb...@citrix.com>
(as far as "domxml-from-native xen-xl" goes, I seem to have another issue
actually starting a domain on ARM, which I'll investigate...)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/16] Xen: remove xend config version

[Ian Campbell]

On Wed, 2015-12-16 at 09:45 +, Ian Campbell wrote:
> On Tue, 2015-12-15 at 14:32 -0700, Jim Fehlig wrote:
> > Hi All,
> > 
> > Ian Campbell recently attempted [1] to fix and issue around
> > MAX_VIRT_VPUS
> > on ARM that required adding a new XEND_CONFIG_VERSION. After some
> > discussion [2] we decided to drop support for all of the old xend
> > config
> > versions and go with the version supported in Xen 4.0, since the xl
> > syntax
> > was originally based on (and intended to be compatible with) xm circa
> > that
> > point in time.
> > 
> > This series removes all traces of xend config version from the
> > codebase,
> > essentially removing support for Xen 3.x. Hopefully I succeeding in
> > making
> > the rather large series reviewable. The series is also available on the
> > remove-xend-config-version branch of my libvirt github clone [2].
> 
> Wow, thanks for offering to take this over, I had no idea it would end up
> with so much yakk hair everywhere!

I've looked through it and the transformations look good to me:

Acked-by: Ian Campbell <ian.campb...@citrix.com>

There were a couple of references to xendConfigVersion remaining in
comments on src/xen/xen_driver.c.

There was also po/* which I presume some sort of semiautomated update will
strip eventually.

I'm not sure what to make of the references under docs/api_extension/.

I tested this on ARM with "Xen: support maxvcpus in xm and xl config" on
top and it worked.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 5/7] virNetDevMacVLanTapSetup: Allow enabling of IFF_MULTI_QUEUE

[Ian Campbell]

Hello,

On Thu, 2015-12-10 at 08:38 +0100, Michal Privoznik wrote:
> Like we are doing for TUN/TAP devices, we should do the same for
> macvtaps. Although, it's not as critical as in that case, we
> should do it for the consistency.
> 
> Signed-off-by: Michal Privoznik 

This has triggered a build failure on amd64+i386+armhf within the Xen
automated test framework (which uses Debian Wheezy as the build
environment), I doubt it is in any way Xen specific though:

util/virnetdevmacvlan.c: In function 'virNetDevMacVLanTapSetup':
util/virnetdevmacvlan.c:338:26: error: 'IFF_MULTI_QUEUE' undeclared (first use 
in this function)
util/virnetdevmacvlan.c:338:26: note: each undeclared identifier is reported 
only once for each function it appears in

I'm not sure where that definition is supposed to come from, so I can't
tell if it is a missing #include in this code or an out of date header on
the Debian system.

Full logs are at
http://logs.test-lab.xenproject.org/osstest/logs/65756/
http://logs.test-lab.xenproject.org/osstest/logs/65756/build-amd64-libvirt/5.ts-libvirt-build.log
http://lists.xen.org/archives/html/xen-devel/2015-12/msg01470.html

But TBH there isn't much more of use than the above.

Cheers,
Ian.

> ---
>  src/util/virnetdevmacvlan.c | 40 ++-
> -
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index c4d0d53..76fd542 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -289,24 +289,26 @@ virNetDevMacVLanTapOpen(const char *ifname,
>   * @tapfd: array of file descriptors of the macvtap tap
>   * @tapfdSize: number of file descriptors in @tapfd
>   * @vnet_hdr: whether to enable or disable IFF_VNET_HDR
> + * @multiqueue: whether to enable or disable IFF_MULTI_QUEUE
> + *
> + * Turn the IFF_VNET_HDR flag, if requested and available, make sure
> it's
> + * off in the other cases. Similarly, IFF_MULTI_QUEUE is enabled if
> + * requested. However, if requested and failed to set, it is considered
> a
> + * fatal error (as opposed to @vnet_hdr).
>   *
> - * Turn the IFF_VNET_HDR flag, if requested and available, make sure
> - * it's off in the other cases.
>   * A fatal error is defined as the VNET_HDR flag being set but it cannot
>   * be turned off for some reason. This is reported with -1. Other fatal
>   * error is not being able to read the interface flags. In that case the
>   * macvtap device should not be used.
>   *
> - * Returns 0 on success, -1 in case of fatal error, error code
> otherwise.
> + * Returns 0 on success, -1 in case of fatal error.
>   */
>  static int
> -virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr)
> +virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr,
> bool multiqueue)
>  {
>  unsigned int features;
>  struct ifreq ifreq;
>  short new_flags = 0;
> -int rc_on_fail = 0;
> -const char *errmsg = NULL;
>  size_t i;
>  
>  for (i = 0; i < tapfdSize; i++) {
> @@ -320,27 +322,29 @@ virNetDevMacVLanTapSetup(int *tapfd, size_t
> tapfdSize, bool vnet_hdr)
>  
>  new_flags = ifreq.ifr_flags;
>  
> -if ((ifreq.ifr_flags & IFF_VNET_HDR) && !vnet_hdr) {
> -new_flags = ifreq.ifr_flags & ~IFF_VNET_HDR;
> -rc_on_fail = -1;
> -errmsg = _("cannot clean IFF_VNET_HDR flag on macvtap tap");
> -} else if ((ifreq.ifr_flags & IFF_VNET_HDR) == 0 && vnet_hdr) {
> +if (vnet_hdr) {
>  if (ioctl(tapfd[i], TUNGETFEATURES, ) < 0) {
>  virReportSystemError(errno, "%s",
>   _("cannot get feature flags on
> macvtap tap"));
>  return -1;
>  }
> -if ((features & IFF_VNET_HDR)) {
> -new_flags = ifreq.ifr_flags | IFF_VNET_HDR;
> -errmsg = _("cannot set IFF_VNET_HDR flag on macvtap
> tap");
> -}
> +if (features & IFF_VNET_HDR)
> +new_flags |= IFF_VNET_HDR;
> +} else {
> +new_flags &= ~IFF_VNET_HDR;
>  }
>  
> +if (multiqueue)
> +new_flags |= IFF_MULTI_QUEUE;
> +else
> +new_flags &= ~IFF_MULTI_QUEUE;
> +
>  if (new_flags != ifreq.ifr_flags) {
>  ifreq.ifr_flags = new_flags;
>  if (ioctl(tapfd[i], TUNSETIFF, ) < 0) {
> -virReportSystemError(errno, "%s", errmsg);
> -return rc_on_fail;
> +virReportSystemError(errno, "%s",
> + _("unable to set vnet or multiqueue
> flags on macvtap"));
> +return -1;
>  }
>  }
>  }
> @@ -852,7 +856,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char
> *tgifname,
>  if (virNetDevMacVLanTapOpen(cr_ifname, , 1, 10) < 0)
>  goto disassociate_exit;
>  
> -if 

Re: [libvirt] [PATCH LIBVIRT] libxl: Use libxentoollog in preference to libxenctrl if available.

[Ian Campbell]

On Mon, 2015-12-14 at 11:15 +, Daniel P. Berrange wrote:
> On Thu, Dec 10, 2015 at 11:38:36AM +0000, Ian Campbell wrote:
> > Upstream Xen is in the process of splitting the (stable API) xtl_*
> > interfaces out from the (unstable API) libxenctrl library and into a
> > new (stable API) libxentoollog.
> > 
> > In order to be compatible with Xen both before and after this
> > transition check for xtl_createlogger_stdiostream in a libxentoollog
> > library and use it if present. If it is not present assume it is in
> > libxenctrl.
> 
> Ok, so there's no API changes, just move stuf from one to the other.

Indeed, it should really have been a separate library all along and the API
already setup that way.

I'm working on some other library splits, which will involve API changes,
but AFAIK they are all isolated from libvirt via the use of libxl, so there
should be no churn for you guys other than this one patch.

> > It might be nice to get this into 1.3.0 so that supports Xen 4.7 out
> > of the box? Not sure what the libvirt stable backport policy is but it
> > might also be good to eventually consider it for that?
> 
> We've missed 1.3.0 release, but I'd be ok with adding it to the
> stable branch if that's going to be useful.

I think it would, to allow things to build with Xen 4.7 (when it is
released).

[...]

> Looks, fine from me but will let Jim push it.

Thanks.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 5/7] virNetDevMacVLanTapSetup: Allow enabling of IFF_MULTI_QUEUE

[Ian Campbell]

On Mon, 2015-12-14 at 12:35 +0100, Michal Privoznik wrote:
> On 14.12.2015 11:23, Ian Campbell wrote:
> > Hello,
> > 
> > On Thu, 2015-12-10 at 08:38 +0100, Michal Privoznik wrote:
> > > Like we are doing for TUN/TAP devices, we should do the same for
> > > macvtaps. Although, it's not as critical as in that case, we
> > > should do it for the consistency.
> > > 
> > > Signed-off-by: Michal Privoznik <mpriv...@redhat.com>
> > 
> > This has triggered a build failure on amd64+i386+armhf within the Xen
> > automated test framework (which uses Debian Wheezy as the build
> > environment), I doubt it is in any way Xen specific though:
> > 
> > util/virnetdevmacvlan.c: In function 'virNetDevMacVLanTapSetup':
> > util/virnetdevmacvlan.c:338:26: error: 'IFF_MULTI_QUEUE' undeclared
> > (first use in this function)
> > util/virnetdevmacvlan.c:338:26: note: each undeclared identifier is
> > reported only once for each function it appears in
> > 
> 
> this is supposed to be fixed by:

Ah, I somehow missed that commit in the logs, sorry.

The test run in question had picked up afe73ed46856 which was before the
fixup, the next one will pickup the newer version and be fine.

Thanks and sorry for the noise.

Ian.

> 
> 
> commit ec93cc25ecdad100a535cb52c08f7eaa3004b960
> Author: Michal Privoznik <mpriv...@redhat.com>
> AuthorDate: Sat Dec 12 08:05:17 2015 +0100
> Commit: Michal Privoznik <mpriv...@redhat.com>
> CommitDate: Sun Dec 13 08:35:46 2015 +0100
> 
> virNetDevMacVLanTapSetup: Work around older systems
> 
> Some older systems, e.g. RHEL-6 do not have IFF_MULTI_QUEUE flag
> which we use to enable multiqueue feature. Therefore one gets the
> following compile error there:
> 
>   CC util/libvirt_util_la-virnetdevmacvlan.lo
> util/virnetdevmacvlan.c: In function 'virNetDevMacVLanTapSetup':
> util/virnetdevmacvlan.c:338: error: 'IFF_MULTI_QUEUE' undeclared
> (first use in this function)
> util/virnetdevmacvlan.c:338: error: (Each undeclared identifier is
> reported only once
> util/virnetdevmacvlan.c:338: error: for each function it appears in.)
> make[3]: *** [util/libvirt_util_la-virnetdevmacvlan.lo] Error 1
> 
> So, whenever user wants us to enable the feature on such systems,
> we will just throw a runtime error instead.
> 
> Signed-off-by: Michal Privoznik <mpriv...@redhat.com>
> 
> 
> 
> Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH LIBVIRT] libxl: Use libxentoollog in preference to libxenctrl if available.

[Ian Campbell]

Upstream Xen is in the process of splitting the (stable API) xtl_*
interfaces out from the (unstable API) libxenctrl library and into a
new (stable API) libxentoollog.

In order to be compatible with Xen both before and after this
transition check for xtl_createlogger_stdiostream in a libxentoollog
library and use it if present. If it is not present assume it is in
libxenctrl.

Compile tested on Xen 4.6 and a development tree with the split in
place.

Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
---
I'm waiting on applying the upstream change until downstreams are
prepared for this. The latest upstream patch is
http://lists.xen.org/archives/html/xen-devel/2015-12/msg00454.html
which had to be reverted because I had somehow not properly checked if
libvirt used this interface
http://lists.xen.org/archives/html/xen-devel/2015-12/msg01153.html

It might be nice to get this into 1.3.0 so that supports Xen 4.7 out
of the box? Not sure what the libvirt stable backport policy is but it
might also be good to eventually consider it for that?
---
 configure.ac | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 98cf210..b641cc7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -883,7 +883,6 @@ if test "$with_libxl" != "no" ; then
 PKG_CHECK_MODULES([LIBXL], [xenlight], [
  LIBXL_FIRMWARE_DIR=`$PKG_CONFIG --variable xenfirmwaredir xenlight`
  LIBXL_EXECBIN_DIR=`$PKG_CONFIG --variable libexec_bin xenlight`
- LIBXL_LIBS="$LIBXL_LIBS -lxenctrl"
  with_libxl=yes
 ], [LIBXL_FOUND=no])
 if test "$LIBXL_FOUND" = "no"; then
@@ -896,7 +895,7 @@ if test "$with_libxl" != "no" ; then
 LIBS="$LIBS $LIBXL_LIBS"
 AC_CHECK_LIB([xenlight], [libxl_ctx_alloc], [
 with_libxl=yes
-LIBXL_LIBS="$LIBXL_LIBS -lxenlight -lxenctrl"
+LIBXL_LIBS="$LIBXL_LIBS -lxenlight"
 ],[
 if test "$with_libxl" = "yes"; then
 fail=1
@@ -924,6 +923,14 @@ if test "$with_libxl" = "yes"; then
 if test "x$LIBXL_EXECBIN_DIR" != "x"; then
 AC_DEFINE_UNQUOTED([LIBXL_EXECBIN_DIR], ["$LIBXL_EXECBIN_DIR"], 
[directory containing Xen libexec binaries])
 fi
+dnl Check if the xtl_* infrastructure is in libxentoollog
+dnl (since Xen 4.7) if not then assume it is in libxenctrl
+dnl (as it was for 4.6 and earler)
+AC_CHECK_LIB([xentoollog], [xtl_createlogger_stdiostream], [
+LIBXL_LIBS="$LIBXL_LIBS -lxentoollog"
+],[
+LIBXL_LIBS="$LIBXL_LIBS -lxenctrl"
+])
 fi
 AM_CONDITIONAL([WITH_LIBXL], [test "$with_libxl" = "yes"])
 
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH LIBVIRT v1 1/2] libxl: Correct value for xendConfigVersion to xen{Parse, Format}ConfigCommon

[Ian Campbell]

On Fri, 2015-12-04 at 10:01 +, Daniel P. Berrange wrote:
> On Thu, Dec 03, 2015 at 11:13:06PM -0700, Jim Fehlig wrote:
> > On 11/26/2015 09:59 AM, Ian Campbell wrote:
> > > libxlConnectDomainXMLFromNative calls both xenParseXM and xenParseXL
> > > with cfg->verInfo->xen_version_major, however AFAICT they both
> > > (either
> > > inherently, or through there use of xenParseConfigCommon expect a
> > > value from xenConfigVersionEnum (which does not correspond to
> > > xen_version_major).
> > 
> > I recall being a little apprehensive about using xen_version_major when
> > writing
> > the code, and apparently that was justified. But now that we are
> > revisiting
> > this, I wonder if we care about these old xend config versions at all.
> > Is anyone
> > even running Xen 3.0.x, or 3.1.x, particularly with a newish libvirt?
> > IMO we
> > could drop all of this xend config nonsense and go with the code
> > associated with
> > the last xend config version, i.e. XEND_CONFIG_VERSION_3_1_0.
> > 
> > And while mentioning dropping support for these old xend config
> > versions, do I
> > dare mention dropping the xend driver altogether? :-) It will certainly
> > need to
> > be done some day. Question is whether that's a bit premature now.
> 
> We just decided to drop QEMU driver code supporting for RHEL-5 vintage
> distros, requiring RHEL-6 as minimum. So I think it is entirely reasonable
> to drop Xen driver code supporting similar vintage XenD.  That would
> certainly simplify or even eliminate the current crazy xend_config_version
> code we have

RHEL 6.0 looks[0] to have been release on 2010-11-09, which was in the
middle of Xen 4.0 and 4.1[1]. 4.0 seems like a decent enough cut off point
(and is what is in Debian oldstable AKA Wheezy, FWIW) plus it is after the
currently latest XEND_CONFIG_VERSION, so all that code could be removed.

Shall I respin this series with that as a precursor?

Ian.

[0] https://access.redhat.com/articles/3078
[1] http://wiki.xen.org/wiki/Xen_Release_Features

> I think we need to continue suppoorting XenD driver for a while, but at
> least you can simplify the code shared with libxl.
> 
> Regards,
> Daniel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] network: selectively disable -Wcast-align in virNetDevParseDadStatus

[Ian Campbell]

Commit 0f7436ca54c9 "network: wait for DAD to finish for bridge IPv6 addresses"
results in:

 CC util/libvirt_util_la-virnetdevmacvlan.lo
util/virnetdev.c: In function 'virNetDevParseDadStatus':
util/virnetdev.c:1319:188: error: cast increases required alignment of target 
type [-Werror=cast-align]
util/virnetdev.c:1332:41: error: cast increases required alignment of target 
type [-Werror=cast-align]
util/virnetdev.c:1334:92: error: cast increases required alignment of target 
type [-Werror=cast-align]
cc1: all warnings being treated as errors

on at least ARM platforms.

The three macros involved (NLMSG_NEXT, IFA_RTA and RTA_NEXT) all appear to
correctly take care of alignment, therefore suppress Wcast-align around their
uses.

Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
Cc: Maxim Perevedentsev <mperevedent...@virtuozzo.com>
Cc: Laine Stump <la...@laine.org>
Cc: Dario Faggioli <dario.faggi...@citrix.com>
Cc: Jim Fehlig <jfeh...@suse.com>
---
 src/util/virnetdev.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index ade9afa..0bc809e 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1316,7 +1316,10 @@ virNetDevParseDadStatus(struct nlmsghdr *nlh, int len,
 struct rtattr *rtattr_ptr;
 size_t i;
 struct in6_addr *addr;
+
+VIR_WARNINGS_NO_CAST_ALIGN
 for (; NLMSG_OK(nlh, len); nlh = NLMSG_NEXT(nlh, len)) {
+VIR_WARNINGS_RESET
 if (NLMSG_PAYLOAD(nlh, 0) < sizeof(struct ifaddrmsg)) {
 /* Message without payload is the last one. */
 break;
@@ -1329,9 +1332,11 @@ virNetDevParseDadStatus(struct nlmsghdr *nlh, int len,
 }
 
 ifaddrmsg_len = IFA_PAYLOAD(nlh);
+VIR_WARNINGS_NO_CAST_ALIGN
 rtattr_ptr = (struct rtattr *) IFA_RTA(ifaddrmsg_ptr);
 for (; RTA_OK(rtattr_ptr, ifaddrmsg_len);
 rtattr_ptr = RTA_NEXT(rtattr_ptr, ifaddrmsg_len)) {
+VIR_WARNINGS_RESET
 if (RTA_PAYLOAD(rtattr_ptr) != sizeof(struct in6_addr)) {
 /* No address: ignore. */
 continue;
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH LIBVIRT v1 0/2] Support maxvcpus (AKA >1 vcpu on Xen/ARM)

[Ian Campbell]

libvirt currently clamps the maximum number of vcpus to MAX_VIRT_CPUS
== XEN_LEGACY_MAX_VCPUS, which on ARM is 1 (because all guests are expected
to support vcpu info placement).

Even on x86 this limitation is a hold over from an older xm interface where
the maximum number of vcpus was expressed as a bitmap and so limited to the
number of bits in an unsigned long (for which XEN_LEGACY_MAX_VCPUS is a
convenient proxy), which doesn't apply to xl on x86 which uses a bitmap
data type to express larger bitmaps.

To do this it was necessary to add a new value to xenConfigVersionEnum
corresponding to this new version.

I've tested with tests/x?configtest (including new test case) on x86 and on
ARM with xlconfigtest and via domxml-from-native.

As you might infer from some of the changlogs I feel a bit like I am
abusing xendConfigVersion somewhat here (given that xl != xend, and that
real xend never went passed xend_config_version) but its use is quite
widespread in the common xl/xm support code in libvirt so a great deal of
refactoring/renaming would otherwise seem to be required. I'm open to other
ideas or suggestions though.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH LIBVIRT v1 1/2] libxl: Correct value for xendConfigVersion to xen{Parse, Format}ConfigCommon

[Ian Campbell]

libxlConnectDomainXMLFromNative calls both xenParseXM and xenParseXL
with cfg->verInfo->xen_version_major, however AFAICT they both (either
inherently, or through there use of xenParseConfigCommon expect a
value from xenConfigVersionEnum (which does not correspond to
xen_version_major).

The actual xend backend passes priv->xendConfigVersion, which seems to
have been gotten from xend and I assume conforms to that enum, via the
node/xend_config_format value in the xend sxp.

Add a new value to that enum, XEND_CONFIG_VERSION_4_0_0, on the basis
that the xl syntax was originally based on (and intended to be
compatible with) xm circa that point in time and that I will shortly
want to add handling of a cfg file difference which occured in xend in
4.0.0 (maxvcpus instead of vpu_avail bitmask).

Pass this from every caller of xen{Parse,Format}X? in the libxl
driver.

Update xlconfigtest to pass this new value, and regenerate the output
files with that (all of the changes are expected AFAICT).

The enum and the parameters are already slightly misnamed now (since
they kind-of apply to xl too), but I didn't go through and rename
them. In the future we may want to add new values to the enum to
reflect evolution of the xl cfg syntax (xend was essentially static
from 4.0 until it was removed), at that point renaming should probably
be considered.

Note also that xend's xend_config_format remained at 4 until it's
removal in Xen 4.5, so there will be no actual change in behaviour for
xm/xend here.

Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
---
 src/libxl/libxl_driver.c|  8 
 src/xenconfig/xen_sxpr.h|  1 +
 tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg |  3 ++-
 tests/xlconfigdata/test-fullvirt-direct-kernel-boot.xml |  2 +-
 tests/xlconfigdata/test-fullvirt-multiusb.cfg   |  3 ++-
 tests/xlconfigdata/test-fullvirt-multiusb.xml   |  2 +-
 tests/xlconfigdata/test-new-disk.cfg|  3 ++-
 tests/xlconfigdata/test-new-disk.xml|  2 +-
 tests/xlconfigdata/test-spice-features.cfg  |  3 ++-
 tests/xlconfigdata/test-spice-features.xml  |  2 +-
 tests/xlconfigdata/test-spice.cfg   |  3 ++-
 tests/xlconfigdata/test-spice.xml   |  2 +-
 tests/xlconfigtest.c| 10 +-
 13 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 35d7fae..892ae44 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2587,14 +2587,14 @@ libxlConnectDomainXMLFromNative(virConnectPtr conn,
 goto cleanup;
 if (!(def = xenParseXL(conf,
cfg->caps,
-   cfg->verInfo->xen_version_major)))
+   XEND_CONFIG_VERSION_4_0_0)))
 goto cleanup;
 } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) {
 if (!(conf = virConfReadMem(nativeConfig, strlen(nativeConfig), 0)))
 goto cleanup;
 
 if (!(def = xenParseXM(conf,
-   cfg->verInfo->xen_version_major,
+   XEND_CONFIG_VERSION_4_0_0,
cfg->caps)))
 goto cleanup;
 } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_SEXPR)) {
@@ -2647,10 +2647,10 @@ libxlConnectDomainXMLToNative(virConnectPtr conn, const 
char * nativeFormat,
 goto cleanup;
 
 if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XL)) {
-if (!(conf = xenFormatXL(def, conn, cfg->verInfo->xen_version_major)))
+if (!(conf = xenFormatXL(def, conn, XEND_CONFIG_VERSION_4_0_0)))
 goto cleanup;
 } else if (STREQ(nativeFormat, LIBXL_CONFIG_FORMAT_XM)) {
-if (!(conf = xenFormatXM(conn, def, cfg->verInfo->xen_version_major)))
+if (!(conf = xenFormatXM(conn, def, XEND_CONFIG_VERSION_4_0_0)))
 goto cleanup;
 } else {
 
diff --git a/src/xenconfig/xen_sxpr.h b/src/xenconfig/xen_sxpr.h
index f354a50..bb9ed3c 100644
--- a/src/xenconfig/xen_sxpr.h
+++ b/src/xenconfig/xen_sxpr.h
@@ -37,6 +37,7 @@ typedef enum {
 XEND_CONFIG_VERSION_3_0_3 = 2,
 XEND_CONFIG_VERSION_3_0_4 = 3,
 XEND_CONFIG_VERSION_3_1_0 = 4,
+XEND_CONFIG_VERSION_4_0_0 = 5,
 } xenConfigVersionEnum;
 
 /* helper functions to get the dom id from a sexpr */
diff --git a/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg 
b/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg
index 1fac3a5..f452af6 100644
--- a/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg
+++ b/tests/xlconfigdata/test-fullvirt-direct-kernel-boot.cfg
@@ -8,6 +8,7 @@ acpi = 1
 apic = 1
 hap = 0
 viridian = 0
+rtc_timeoffset = 0
 localtime = 0
 on_poweroff = "destroy"
 on_reboot = "restart"
@@ -18,7 +1

[libvirt] [PATCH LIBVIRT v1 2/2] xen: Handle maxcpus in xl configutation files

[Ian Campbell]

This new cfg field was addded in 4.0 by 68a94cf528e6 "xm: Add maxvcpus
support" and, more crucially these day, it is what xl supports.

This removes the MAX_VIRT_CPUS limitation for such versions of Xen
(since maxvcpus is simply a count, not a bit mask) which is
particularly crucial on ARM where MAX_VIRT_CPUS == 1 (since all guests
are expected to support vcpu placement, and therefore only the boot
vcpu's info lives in the shared info page).

Add a new test case to both XM and XL config tests covering this case.

Note that although xm gained support for maxvcpus in Xen 4.0 the
xend_config_format was never bumped beyond 4 and the internal handling
remained in terms of vcpu_avail. Therefore the support for
xend >= XEND_CONFIG_VERSION_4_0_0 is somewhat theoretical.

Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
---
 src/xenconfig/xen_common.c| 72 ---
 src/xenconfig/xen_xm.c|  7 +++
 tests/xlconfigdata/test-paravirt-maxvcpus.cfg | 13 +
 tests/xlconfigdata/test-paravirt-maxvcpus.xml | 28 +++
 tests/xlconfigtest.c  |  1 +
 tests/xmconfigdata/test-paravirt-maxvcpus.cfg | 13 +
 tests/xmconfigdata/test-paravirt-maxvcpus.xml | 30 +++
 tests/xmconfigtest.c  |  1 +
 8 files changed, 146 insertions(+), 19 deletions(-)
 create mode 100644 tests/xlconfigdata/test-paravirt-maxvcpus.cfg
 create mode 100644 tests/xlconfigdata/test-paravirt-maxvcpus.xml
 create mode 100644 tests/xmconfigdata/test-paravirt-maxvcpus.cfg
 create mode 100644 tests/xmconfigdata/test-paravirt-maxvcpus.xml

diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 0890c73..6f2afcc 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -492,24 +492,49 @@ xenParsePCI(virConfPtr conf, virDomainDefPtr def)
 
 
 static int
-xenParseCPUFeatures(virConfPtr conf, virDomainDefPtr def)
+xenParseCPUFeatures(virConfPtr conf, virDomainDefPtr def,
+int xendConfigVersion)
 {
 unsigned long count = 0;
 const char *str = NULL;
 int val = 0;
 
-if (xenConfigGetULong(conf, "vcpus", , 1) < 0 ||
-MAX_VIRT_CPUS < count)
-return -1;
+/*
+ * xend prior to 4.0 understands vcpus as maxvcpus and vcpu_avail
+ * as a bit map of which cpus are online (default is all).
+ *
+ * xend from 4.0 onwards understands maxvcpus as maxvcpus and
+ * vcpus as the number which are online (from 0..N-1). The
+ * upstream commit (68a94cf528e6 "xm: Add maxvcpus support")
+ * claims that if maxvcpus is omitted then the old behaviour
+ * (i.e. obeying vcpu_avail) is retained, but AFAICT it was not,
+ * in this case vcpu==maxcpus==online cpus. This is good for us
+ * because handling anything else would be fiddly.
+ *
+ * xl understands vcpus + maxvcpus, but not vcpu_avail
+ * (ever).
+ */
+if (xendConfigVersion < XEND_CONFIG_VERSION_4_0_0) {
+if (xenConfigGetULong(conf, "vcpus", , 1) < 0 ||
+MAX_VIRT_CPUS < count)
+return -1;
+def->maxvcpus = count;
 
-def->maxvcpus = count;
-if (xenConfigGetULong(conf, "vcpu_avail", , -1) < 0)
-return -1;
+if (xenConfigGetULong(conf, "vcpu_avail", , -1) < 0)
+return -1;
+def->vcpus = MIN(count_one_bits_l(count), def->maxvcpus);
+} else {
+if (xenConfigGetULong(conf, "vcpus", , 1) < 0)
+return -1;
+def->vcpus = count;
+
+if (xenConfigGetULong(conf, "maxvcpus", , def->vcpus) < 0)
+return -1;
+def->maxvcpus = count;
+}
 
-def->vcpus = MIN(count_one_bits_l(count), def->maxvcpus);
 if (xenConfigGetString(conf, "cpus", , NULL) < 0)
 return -1;
-
 if (str && (virBitmapParse(str, 0, >cpumask, 4096) < 0))
 return -1;
 
@@ -1032,7 +1057,7 @@ xenParseConfigCommon(virConfPtr conf,
 if (xenParseEventsActions(conf, def) < 0)
 return -1;
 
-if (xenParseCPUFeatures(conf, def) < 0)
+if (xenParseCPUFeatures(conf, def, xendConfigVersion) < 0)
 return -1;
 
 if (xenParseTimeOffset(conf, def, xendConfigVersion) < 0)
@@ -1519,19 +1544,28 @@ xenFormatCharDev(virConfPtr conf, virDomainDefPtr def)
 
 
 static int
-xenFormatCPUAllocation(virConfPtr conf, virDomainDefPtr def)
+xenFormatCPUAllocation(virConfPtr conf, virDomainDefPtr def,
+   int xendConfigVersion)
 {
 int ret = -1;
 char *cpus = NULL;
 
-if (xenConfigSetInt(conf, "vcpus", def->maxvcpus) < 0)
-goto cleanup;
+if (xendConfigVersion >= XEND_CONFIG_VERSION_4_0_0) {
+if (def->vcpus < def->maxvcpus &&
+xenConfigSetInt(conf, "maxvcpus", de

Re: [libvirt] build issue due to '-Werror=cast-align' on ARM (armhf) [was: Re: [Xen-devel] [libvirt test] 63528: regressions - FAIL]

[Ian Campbell]

Hi Maxim,

On Fri, 2015-11-13 at 20:26 -0700, Jim Fehlig wrote:
> On 11/13/2015 10:00 AM, Dario Faggioli wrote:
> > Hello,
> > 
> > The Xen Project's automated test suite is failing at running its
> > libvirt tests for a few time, like this:
> > 
> > On Wed, 2015-11-04 at 09:04 +, osstest service owner wrote:
> > > flight 63528 libvirt real [real]
> > > http://logs.test-lab.xenproject.org/osstest/logs/63528/
> > > 
> > > Regressions :-(
> > > 
> > > Tests which did not succeed and are blocking,
> > > including tests which could not be run:
> > >  build-armhf-libvirt   5 libvirt-build fail REGR.
> > > vs. 63340
> > > 
> > More specifically (from a more recent run):
> > 
> >   CC util/libvirt_util_la-virnetdevmacvlan.lo
> > util/virnetdev.c: In function 'virNetDevParseDadStatus':
> > util/virnetdev.c:1319:188: error: cast increases required alignment of 
> > target type [-Werror=cast-align]
> > util/virnetdev.c:1332:41: error: cast increases required alignment of 
> > target type [-Werror=cast-align]
> > util/virnetdev.c:1334:92: error: cast increases required alignment of 
> > target type [-Werror=cast-align]
> > cc1: all warnings being treated as errors
> > make[3]: *** [util/libvirt_util_la-virnetdev.lo] Error 1
> > make[3]: *** Waiting for unfinished jobs
> > make[3]: Leaving directory 
> > `/home/osstest/build.64130.build-armhf-libvirt/libvirt/src'
> > make[2]: *** [all] Error 2
> > make[2]: Leaving directory 
> > `/home/osstest/build.64130.build-armhf-libvirt/libvirt/src'
> > make[1]: *** [all-recursive] Error 1
> > make[1]: Leaving directory 
> > `/home/osstest/build.64130.build-armhf-libvirt/libvirt'
> > make: *** [all] Error 2


According to http://logs.test-lab.xenproject.org/osstest/logs/65035/build-a
rmhf-libvirt/info.html this build failure is still occurring with the same
trace.

Is there a fix in the works?

Ian.

> 
> Yep, noticed this several days ago but haven't yet found time to
> investigate.
> 
> > The changeset being tested last, as of now, and still failing, is:
> > db92aee2b477355759351889e37a365379b43bf6
> 
> I did notice the bisector ran and flagged commit 0f7436ca
> 
> http://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03385.html
> 
> I've cc'd author of that commit.
> 
> Regards,
> Jim
> 
> > 
> > Full logs available at the following links:
> > http://logs.test-lab.xenproject.org/osstest/logs/64130/build-armhf-libv
> > irt/info.html
> > http://logs.test-lab.xenproject.org/osstest/logs/64130/build-armhf-libv
> > irt/5.ts-libvirt-build.log
> > 
> > Right now, I don't have an ARM cross build environment handy to try to
> > reproduce and attempt a fix, so I figured I at least was reporting it
> > here.
> > 
> > Regards,
> > Dario
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH RFC] libxl: use libxl_event_wait to process libxl events

[Ian Campbell]

On Fri, 2015-11-13 at 14:36 -0700, Jim Fehlig wrote:
> Prior to this patch, libxl events were delivered to libvirt via
> the libxlDomainEventHandler callback registered with libxl.
> Documenation in $xensrc/tools/libxl/libxl_event.h states that the
> callback "may occur on any thread in which the application calls
> libxl". This can result in deadlock since many of the libvirt
> callees of libxl hold a lock on the virDomainObj they are working
> on. When the callback is invoked, it attempts to find a virDomainObj
> corresponding to the domain ID provided by libxl. Searching the
> domain obj list results in locking each obj before checking if it is
> active, and its ID equals the requested ID. Deadlock is possible
> when attempting to lock an obj that is already locked further up
> the call stack. Indeed, Max Ustermann recently reported an instance
> of this deadlock
> 
> https://www.redhat.com/archives/libvir-list/2015-November/msg00130.html
> 
> This patch moves processing of libxl events to a thread, where
> libxl_event_wait() is used to collect events. This allows processing
> libxl events asynchronously in libvirt, avoiding the deadlock.
> 
> Reported-by: max ustermann 
> Signed-off-by: Jim Fehlig 
> ---
> 
> The only reservations I have about this patch come from comments
> about libxl_event_wait in libxl_event.h
> 
>   Like libxl_event_check but blocks if no suitable events are
>   available, until some are.  Uses libxl_osevent_beforepoll/
>   _afterpoll so may be inefficient if very many domains are being
>   handled by a single program.
> 
> libvirt does handle "very many domains" :-). But thus far, I haven't
> noticed any problems in my testing. The reporter expressed interest
> in testing the patch. Positive results from that testing would improve
> my confidence, as would an ACK from Ian Jackson.

Would it be possible/allowable to have your cake and eat it by using the
callbacks but having them simply enqueue the events on the libvirt side and
kick the dedicated thread for further processing?

Or we could consider whether libxl should gain such functionality.


> 
>  src/libxl/libxl_domain.c | 130 ++---
> --
>  src/libxl/libxl_domain.h |  16 +-
>  src/libxl/libxl_driver.c |  13 ++---
>  3 files changed, 66 insertions(+), 93 deletions(-)
> 
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 40dcea1..0b8c292 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -380,27 +380,14 @@ virDomainDefParserConfig libxlDomainDefParserConfig
> = {
>  .domainPostParseCallback = libxlDomainDefPostParse,
>  };
>  
> -
> -struct libxlShutdownThreadInfo
> -{
> -libxlDriverPrivatePtr driver;
> -virDomainObjPtr vm;
> -libxl_event *event;
> -};
> -
> -
>  static void
> -libxlDomainShutdownThread(void *opaque)
> +libxlDomainShutdownEventHandler(libxlDriverPrivatePtr driver,
> +virDomainObjPtr vm,
> +libxl_event *ev)
>  {
> -struct libxlShutdownThreadInfo *shutdown_info = opaque;
> -virDomainObjPtr vm = shutdown_info->vm;
> -libxl_event *ev = shutdown_info->event;
> -libxlDriverPrivatePtr driver = shutdown_info->driver;
>  virObjectEventPtr dom_event = NULL;
>  libxl_shutdown_reason xl_reason = ev-
> >u.domain_shutdown.shutdown_reason;
> -libxlDriverConfigPtr cfg;
> -
> -cfg = libxlDriverConfigGet(driver);
> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>  
>  if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
>  goto cleanup;
> @@ -501,74 +488,77 @@ libxlDomainShutdownThread(void *opaque)
>  virObjectUnlock(vm);
>  if (dom_event)
>  libxlDomainEventQueue(driver, dom_event);
> -libxl_event_free(cfg->ctx, ev);
> -VIR_FREE(shutdown_info);
>  virObjectUnref(cfg);
>  }
>  
> +static int
> +libxlDomainXEventsPredicate(const libxl_event *event,
> +   ATTRIBUTE_UNUSED void *data)
> +{
> +/*
> + * Currently we only handle shutdown event
> + */
> +if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN)
> +return 1;
> +
> +return 0;
> +}
> +
>  /*
> - * Handle previously registered domain event notification from
> libxenlight.
> + * Process events from libxl using libxl_event_wait.
>   */
> -void
> -libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST libxl_event
> *event)
> +static void
> +libxlDomainXEventsProcess(void *opaque)
>  {
> -libxlDriverPrivatePtr driver = data;
> +libxlDriverPrivatePtr driver = opaque;
> +libxl_event *event;
>  virDomainObjPtr vm = NULL;
> -libxl_shutdown_reason xl_reason = event-
> >u.domain_shutdown.shutdown_reason;
> -struct libxlShutdownThreadInfo *shutdown_info = NULL;
> -virThread thread;
> -libxlDriverConfigPtr cfg;
> +libxl_shutdown_reason xl_reason;
> +libxlDriverConfigPtr cfg = 

Re: [libvirt] [Xen-devel] [PATCH] libxl: open libxl log stream with libvirtd log_level

[Ian Campbell]

On Tue, 2015-09-15 at 09:26 -0600, Jim Fehlig wrote:
> Instead of a hardcoded DEBUG log level, use the overall
> daemon log level specified in libvirtd.conf when opening
> a log stream with libxl. libxl is very verbose when DEBUG
> log level is set, resulting in huge log files that can
> potentially fill a disk. Control of libxl verbosity should
> be placed in the administrator's hands.
> 
> Signed-off-by: Jim Fehlig <jfeh...@suse.com>

FWIW this seems like a good idea to me:
Acked-by: Ian Campbell <ian.campb...@citrix.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH LIBVIRT] libxl: don't end job for ephemeal domain on start failure

[Ian Campbell]

commit 4b53d0d4ac9c "libxl: don't remove persistent domain on start
failure" cleans up the vm object and sets it to NULL if the vm is not
persistent, however at end job vm (now NULL) is dereferenced via the call to
libxlDomainObjEndJob. Avoid this by skipping "endjob" and going
straight to "cleanup" in this case.

Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
---
 src/libxl/libxl_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 5f69b49..e2797d5 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -992,6 +992,7 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
 if (!vm->persistent) {
 virDomainObjListRemove(driver->domains, vm);
 vm = NULL;
+goto cleanup;
 }
 goto endjob;
 }
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] libxl: report correct errno from virNetSocketNewConnectTCP on migration

[Ian Campbell]

On Thu, 2015-09-03 at 10:26 -0600, Jim Fehlig wrote:
> From a30c493bd9e20c9a7a423789a202c444a5eba344 Mon Sep 17 00:00:00 2001
> From: Jim Fehlig <jfeh...@suse.com>
> Date: Thu, 3 Sep 2015 10:14:20 -0600
> Subject: [PATCH] libxl: don't overwrite error from 
> virNetSocketNewConnectTCP()
> 
> Remove redundant error reporting libxlDomainMigrationPerform().
> virNetSocketNewConnectTCP() is perfectly capable of reporting
> sensible errors.
> 
> Signed-off-by: Jim Fehlig <jfeh...@suse.com>

FWIW: Acked-by: Ian Campbell <ian.campb...@citrix.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] libxl: report correct errno from virNetSocketNewConnectTCP on migration

[Ian Campbell]

saved_errno is never written to in this function after it is
initialised and it is only used to log the failure from
virNetSocketNewConnectTCP masking the real errno from that function.

Drop saved_errno and use errno itself.

Signed-off-by: Ian Campbell <ian.campb...@citrix.com>
---
 src/libxl/libxl_migration.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 39e4a65..e291d71 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -480,7 +480,6 @@ libxlDomainMigrationPerform(libxlDriverPrivatePtr driver,
 virURIPtr uri = NULL;
 virNetSocketPtr sock;
 int sockfd = -1;
-int saved_errno = EINVAL;
 int ret = -1;
 
 /* parse dst host:port from uri */
@@ -496,7 +495,7 @@ libxlDomainMigrationPerform(libxlDriverPrivatePtr driver,
 if (virNetSocketNewConnectTCP(hostname, portstr,
   AF_UNSPEC,
   ) < 0) {
-virReportSystemError(saved_errno,
+virReportSystemError(errno,
  _("unable to connect to '%s:%s'"),
  hostname, portstr);
 goto cleanup;
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH libvirt] libxl: avoid freeing an uninitialised bitmap

[Ian Campbell]

If vm-def-cputune.nvcpupin is 0 in libxlDomainSetVcpuAffinities (as
seems to be the case on arm) then the VIR_FREE after cleanup: would be
operating on an uninitialised pointer in map.map.

Fix this by using libxl_bitmap_init and libxl_bitmap_dispose in the
appropriate places (like VIR_FREE libxl_bitmap_dispose is also
idempotent, so there is no double free on exit from the loop).

libxl_bitmap_dispose is slightly preferable since it also sets
map.size back to 0, avoiding a potential source of confusion.

This fixes the crashes we've been seeing in the Xen automated tests on
ARM.

I had a glance at the handful of other users of libxl_bitmap and none
of them looked to have a similar issue.

Signed-off-by: Ian Campbell ian.campb...@citrix.com
---
 src/libxl/libxl_domain.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 0652270..0904b78 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -789,6 +789,8 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm)
 size_t i;
 int ret = -1;
 
+libxl_bitmap_init(map);
+
 for (i = 0; i  vm-def-cputune.nvcpupin; ++i) {
 pin = vm-def-cputune.vcpupin[i];
 cpumask = pin-cpumask;
@@ -802,13 +804,13 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr 
driver, virDomainObjPtr vm)
 goto cleanup;
 }
 
-VIR_FREE(map.map);
+libxl_bitmap_dispose(map); /* Also returns to freshly-init'd state */
 }
 
 ret = 0;
 
  cleanup:
-VIR_FREE(map.map);
+libxl_bitmap_dispose(map);
 virObjectUnref(cfg);
 return ret;
 }
-- 
1.7.10.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

On Wed, 2015-05-06 at 10:24 +0200, Olaf Hering wrote:
 On Fri, May 01, Ian Campbell wrote:
 
  Olaf, please can you use gdb to capture the stack trace so we can fix
  this (and the other issue) properly in libxl instead of just hacking
  around it in libvirt (which might also be appropriate for compat with
  old libxl but shouldn't be done without also fixing libxl IMHO).
 
 The code flow was essentially like this:
 
 libxl_device_vfb_init(libxl);
 switch(libvirt-type) {
   case SDL:
 libxl_defbool_set(libxl-sdl.enable, 1);
 break;
   case VNC:
 libxl_defbool_set(libxl-vnc.enable, 1);
 break;
 }
 
 if (libvirt-os.type == HVM) {
   if (libxl_defbool_val(libxl-vnc.enable)) {
 /* do VNC things */
   } else if (libxl_defbool_val(libxl-sdl.enable)) {
 /* do SDL things */
 if (libxl_defbool_val(libxl-opengl.enable))
   /* do openGL things */
   }
 }
 
 
 The first crash was because I had SDL enabled, and the SDL case did not
 initialize the defbool for VNC. Once it did the next crash was the
 openGL part which was not initialized either.
 
 I see nothing wrong with libxl in such usage.

I've explained this repeatedly now, it is a bug in libxl if the above
ends up crashing in libxl.

It is always a *libxl bug* for a libxl code path to lead a use of
libxl_defbool_val on a value which has not had the appropriate
setdefault called on it *by libxl*. It is not required for the user of
libxl to initialise any defbool (other than via the libxl_TYPE_init
function, which should set it to the explicit default value).

It is therefore a bug if libxl reaches this code without having ensured
that libxl_defbool_setdefault has been called *by libxl* on
libxl-opengl.enable (perhaps on if libxl-sdl.enable is true).

I see no code in libxl which matches what you have above, the only call
to libxl_device_vfb_init has no switch statement or if  == HVM anywhere
near it.

Please provide the actual stack trace as requested so we can see which
code path in libxl is failing to properly initialise the defbools.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

On Wed, 2015-05-06 at 10:08 +0100, Ian Campbell wrote:
 On Wed, 2015-05-06 at 10:24 +0200, Olaf Hering wrote:
  On Fri, May 01, Ian Campbell wrote:
  
   Olaf, please can you use gdb to capture the stack trace so we can fix
   this (and the other issue) properly in libxl instead of just hacking
   around it in libvirt (which might also be appropriate for compat with
   old libxl but shouldn't be done without also fixing libxl IMHO).
  
  The code flow was essentially like this:
  
  libxl_device_vfb_init(libxl);
  switch(libvirt-type) {
case SDL:
  libxl_defbool_set(libxl-sdl.enable, 1);
  break;
case VNC:
  libxl_defbool_set(libxl-vnc.enable, 1);
  break;
  }
  
  if (libvirt-os.type == HVM) {
if (libxl_defbool_val(libxl-vnc.enable)) {
  /* do VNC things */
} else if (libxl_defbool_val(libxl-sdl.enable)) {
  /* do SDL things */
  if (libxl_defbool_val(libxl-opengl.enable))
/* do openGL things */
}
  }
  
  
  The first crash was because I had SDL enabled, and the SDL case did not
  initialize the defbool for VNC. Once it did the next crash was the
  openGL part which was not initialized either.
  
  I see nothing wrong with libxl in such usage.

 Please provide the actual stack trace as requested so we can see which
 code path in libxl is failing to properly initialise the defbools.

Ah, one moment, are you saying that all this code is within libvirt and
not in libxl and that the libxl- object here hasn't yet been passed
to a libxl function?

If so then yes it is a libvirt bug to use
libxl_defbool_val(libxl-opengl.enable) without having called set on it
first. If you passed such a thing to libxl and it crashed then that
would be a libxl bug, but it seems you aren't getting that far.

I was confused by the initial message because the assert says libxl.c,
but that's just because it is out of line, which lead to a confusing
error message. Sorry for leading the conversation down the wrong alley
way.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

On Fri, 2015-05-01 at 09:10 -0600, Jim Fehlig wrote:
 Ian Campbell wrote:
  On Fri, 2015-04-17 at 13:57 -0600, Jim Fehlig wrote:

  On 04/17/2015 11:59 AM, Olaf Hering wrote:
  
  On Fri, Apr 17, Olaf Hering wrote:
 

  If the domU configu has sdl enabled libvirtd crashes:
  libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion 
  `!libxl_defbool_is_default(db)' failed.
 
  Initialize the relevant defbool variables in libxl_device_vfb.
  
  Fix one crash, find another:
 
  Does libvirt have a representation for this one?
 
 libxl_defbool_val(vfb.sdl.opengl));

  I'm not aware of any way to specify OpenGL in libvirt domXML.
 
  
  If not, it should be initialized to false in libxlMakeVfb.

  As before, seems like the init function should handle this.
  
 
  As before, _not_ the init function, the setdefault within libxl should
  ensure that this is set to some value _before_ it is used. These changes
  are just hiding libxl bugs.

 
 Yes, I agree.
 
  Olaf, please can you use gdb to capture the stack trace so we can fix
  this (and the other issue) properly in libxl instead of just hacking
  around it in libvirt (which might also be appropriate for compat with
  old libxl but shouldn't be done without also fixing libxl IMHO).

 
 I was torn on committing Olaf's changes to the libvirt libxl driver, but
 in the end did so for the compatibility you noted.

I just fear that the underlying issue will now not be diagnosed.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

On Fri, 2015-04-17 at 13:57 -0600, Jim Fehlig wrote:
 On 04/17/2015 11:59 AM, Olaf Hering wrote:
  On Fri, Apr 17, Olaf Hering wrote:
 
  If the domU configu has sdl enabled libvirtd crashes:
  libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion 
  `!libxl_defbool_is_default(db)' failed.
 
  Initialize the relevant defbool variables in libxl_device_vfb.
  Fix one crash, find another:
 
  Does libvirt have a representation for this one?
 
 libxl_defbool_val(vfb.sdl.opengl));
 
 I'm not aware of any way to specify OpenGL in libvirt domXML.
 
  If not, it should be initialized to false in libxlMakeVfb.
 
 As before, seems like the init function should handle this.

As before, _not_ the init function, the setdefault within libxl should
ensure that this is set to some value _before_ it is used. These changes
are just hiding libxl bugs.

Olaf, please can you use gdb to capture the stack trace so we can fix
this (and the other issue) properly in libxl instead of just hacking
around it in libvirt (which might also be appropriate for compat with
old libxl but shouldn't be done without also fixing libxl IMHO).

Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

On Fri, 2015-04-17 at 13:40 -0600, Jim Fehlig wrote:
 On 04/17/2015 11:19 AM, Olaf Hering wrote:
  If the domU configu has sdl enabled libvirtd crashes:
  libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion 
  `!libxl_defbool_is_default(db)' failed.
 
 The assertion seems harsh considering the offense...

libxl should be setting a default for this value if the application
hasn't, by calling libxl__device_vfb_setdefault at some correct point.

Not to say the application shouldn't also set it if it has a different
default preference, but there is a libxl bug here somewhere too.

Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

On Mon, 2015-04-20 at 12:32 +0200, Olaf Hering wrote:
 On Mon, Apr 20, Ian Campbell wrote:
 
  It makes no sense to do that at init time, the whole purpose of a
  defbool is to allow the calling application to choose a value or to
  explicitly leave it as a request to for the default (which might vary
  depending on other selections).
 
 Yes, and thats why my patch does?

This is a libvirt patch. It might be correct in its own right, if
libvirt has different ideas about the defaults to libxl, but it is also
masking an underlying libxl bug.

Ian.

 
  libxl_device_vfb_init(x_vfb);
 +libxl_defbool_set(x_vfb-sdl.opengl, 0);
  if (libxl_defbool_val(vfb.sdl.opengl))
;
 
 Olaf


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

On Mon, 2015-04-20 at 12:20 +0200, Olaf Hering wrote:
 On Mon, Apr 20, Ian Campbell wrote:
 
  If what you said were true then an assert would be a rather harsh
  overreaction to an application coding error.
 
 Currently both libxl and libvirt are coded that way. Since the sdl code
 path in libvirt was never executed the crash in libxlMakeVfbList was not
 noticed.
 
 So you are saying that an external user of libxl_device_vfb_init has to
 call yet another function to initialize all defbool to give them either
 true of false? Or should libxl_device_vfb_init call
 libxl__device_vfb_setdefault directly?
 

Neither.

Any code within libxl which consumes a libxl_device_vfb must, at some
point before touching the defbools therein, have arranged to call the
appropriate setdefaults function.

It makes no sense to do that at init time, the whole purpose of a
defbool is to allow the calling application to choose a value or to
explicitly leave it as a request to for the default (which might vary
depending on other selections).

It is up to *libxl* to convert such requests for default behaviour into
a specific value before trying to use it, that is the purpose of the
internal setdefaults functions and for the assert when reading a
defbool.

Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: initialize vfb defbools in libxlMakeVfb

[Ian Campbell]

On Mon, 2015-04-20 at 11:32 +0200, Olaf Hering wrote:
 On Mon, Apr 20, Ian Campbell wrote:
 
  On Fri, 2015-04-17 at 13:40 -0600, Jim Fehlig wrote:
   On 04/17/2015 11:19 AM, Olaf Hering wrote:
If the domU configu has sdl enabled libvirtd crashes:
libvirtd[5158]: libvirtd: libxl.c:343: libxl_defbool_val: Assertion 
`!libxl_defbool_is_default(db)' failed.
   
   The assertion seems harsh considering the offense...
  
  libxl should be setting a default for this value if the application
  hasn't, by calling libxl__device_vfb_setdefault at some correct point.
 
 This is an internal function. Its up to the caller of
 libxl_device_vfb_init to initialize the defbool members.

No, it isn't. It's up to libxl to call libxl__device_vfb_setdefault
somewhere on the code path between entering the library and actually
using this defbool. (Possibly indirectly via the containing struct in
this case)

We normally do this as close to entry into the library as we can, e.g.
in the libxl_device_FOO_add or e.g. in libxl_domain_create.

It is always a libxl bug if a defbool gets used without having had a
defaulted value applied by the library first, hence the assert.

If what you said were true then an assert would be a rather harsh
overreaction to an application coding error.

Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [libvirt test] 50401: regressions - FAIL

[Ian Campbell]

On Wed, 2015-04-15 at 14:16 +0100, Daniel P. Berrange wrote:
 Yeah, there is nothing Xen specific about the problem - it is entirely
 down to the build toolchain  compiler options.

FYI our bisector has now tripped over another related problem, 
http://lists.xen.org/archives/html/xen-devel/2015-04/msg01900.html
http://lists.xen.org/archives/html/xen-devel/2015-04/msg01968.html

in particular:

http://logs.test-lab.xenproject.org/osstest/logs/50460/build-armhf-libvirt/5.ts-libvirt-build.log:

qemu/qemu_domain.c: In function 'qemuDomainSupportsBlockJobs':
qemu/qemu_domain.c:3067:11: error: declaration of 'sync' shadows a 
global 
declaration [-Werror=shadow]
cc1: all warnings being treated as errors

Essentially another instance of the same class of issue, in this case
arising from:

  commit 1eccac1d2da7bbe97e1df25fd0ddac6e71b0794a
  Author: Peter Krempa pkre...@redhat.com
  Date:   Tue Mar 31 17:29:35 2015 +0200
  
  qemu: domain: Add helper to check block job support

Given the code in autoconf to enable essentially all warnings I'm a bit
surprised -Wshadow isn't being enabled for everyone -- I thought it was
quite a long standing warning.

I'm also not sure why we are having a sudden rash of these now.

But perhaps it is worth adding Wshadow to the set of excluded options
until either it is more widely supported or the libvirt code base has
been audited? I can send such a patch if you think that's desirable.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/2] Re: libvirtd live-locking on CTX_LOCK when doing 'virsh domid save /tmp/blah' with guest corrupting memory (on purpose).

[Ian Campbell]

On Thu, 2015-04-16 at 18:18 +0100, Ian Jackson wrote:
 Jim Fehlig writes (Re: [PATCH 0/2] Re: libvirtd live-locking on CTX_LOCK 
 when doing 'virsh domid save /tmp/blah' with guest corrupting memory (on 
 purpose).):
  On 04/14/2015 11:31 AM, Ian Jackson wrote:
   I have produced what I think are two patches that will fix this.  I
   have compiled them but I haven't tested them.  Konrad, are you able to
   check whether they fix your bug ?
  
  I too saw this bug just before Konrad's report, but the patches don't seem 
  to 
  help.  Running a script that continually saves and restores domains will 
  eventually lock libvirtd with essentially the same traces reported by Konrad
 
 I'm a total idiot.  I do the recheck but I don't pay any attention to
 the result.

Your second patch was updating revents which was used in the next if,
what have I missed?

 I will send an updated approach which does this centrally.

Ack, that's probably best anyhow.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [libvirt test] 50401: regressions - FAIL

[Ian Campbell]

On Tue, 2015-04-14 at 10:37 +0100, Daniel P. Berrange wrote:
 On Tue, Apr 14, 2015 at 10:33:45AM +0100, Ian Campbell wrote:
  On Tue, 2015-04-14 at 02:27 +, osstest service user wrote:
   flight 50401 libvirt real [real]
   http://logs.test-lab.xenproject.org/osstest/logs/50401/
   
   Regressions :-(
   
   Tests which did not succeed and are blocking,
   including tests which could not be run:
build-armhf-libvirt   5 libvirt-build fail REGR. vs. 
   50368
  [...]
  Per
  http://logs.test-lab.xenproject.org/osstest/logs/50401/build-armhf-libvirt/5.ts-libvirt-build.log
   this is:
  
  qemu/qemu_driver.c: In function 'qemuDomainAddCgroupForThread':
  qemu/qemu_driver.c:4641:34: error: declaration of 'index' shadows a global 
  declaration [-Werror=shadow]
  qemu/qemu_driver.c: In function 'qemuDomainHotplugAddPin':
  qemu/qemu_driver.c:4674:29: error: declaration of 'index' shadows a global 
  declaration [-Werror=shadow]
  qemu/qemu_driver.c: In function 'qemuDomainHotplugPinThread':
  qemu/qemu_driver.c:4702:32: error: declaration of 'index' shadows a global 
  declaration [-Werror=shadow]
  qemu/qemu_driver.c: In function 'qemuDomainDelCgroupForThread':
  qemu/qemu_driver.c:4733:34: error: declaration of 'index' shadows a global 
  declaration [-Werror=shadow]
  cc1: all warnings being treated as errors
  
  
  This seems to be a general issue unrelated to Xen.
  
   version targeted for testing:
libvirt  b487bb810ec95df862e7e80468c8e861ed80b0cb
   baseline version:
libvirt  225aa80246d5e4a9e3a16ebd4c482525045da3db
  
  After a quick glance I don't see a fix post-b487bb810ec9 either in
  master or on the libvirt list.
  
  Looking at the range under test it looks like one or more of John's
  changes is adding parameters called index, shadowing index(3) from
  strings.h.
 
 Yeah, we've had this problem several times before - we usually just
 do a s/index/idx/ or similar to address it.

I see this is now fixed in libvirt.git#master, thanks.

However, I would just comment that contrary to the commit message, I
don't think there is anything the Xen build has done which caused this,
I think it's down to the LIBVIRT_COMPILE_WARNINGS macro which ends up
enabling Wshadow if it was available on the system which ran autogen and
on the compiling system.

Our builds run on Debian Wheezy, which IIRC uses gcc 4.4 which isn't
unusual...

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [libvirt test] 50401: regressions - FAIL

[Ian Campbell]

On Tue, 2015-04-14 at 02:27 +, osstest service user wrote:
 flight 50401 libvirt real [real]
 http://logs.test-lab.xenproject.org/osstest/logs/50401/
 
 Regressions :-(
 
 Tests which did not succeed and are blocking,
 including tests which could not be run:
  build-armhf-libvirt   5 libvirt-build fail REGR. vs. 
 50368
[...]
Per
http://logs.test-lab.xenproject.org/osstest/logs/50401/build-armhf-libvirt/5.ts-libvirt-build.log
 this is:

qemu/qemu_driver.c: In function 'qemuDomainAddCgroupForThread':
qemu/qemu_driver.c:4641:34: error: declaration of 'index' shadows a global 
declaration [-Werror=shadow]
qemu/qemu_driver.c: In function 'qemuDomainHotplugAddPin':
qemu/qemu_driver.c:4674:29: error: declaration of 'index' shadows a global 
declaration [-Werror=shadow]
qemu/qemu_driver.c: In function 'qemuDomainHotplugPinThread':
qemu/qemu_driver.c:4702:32: error: declaration of 'index' shadows a global 
declaration [-Werror=shadow]
qemu/qemu_driver.c: In function 'qemuDomainDelCgroupForThread':
qemu/qemu_driver.c:4733:34: error: declaration of 'index' shadows a global 
declaration [-Werror=shadow]
cc1: all warnings being treated as errors


This seems to be a general issue unrelated to Xen.

 version targeted for testing:
  libvirt  b487bb810ec95df862e7e80468c8e861ed80b0cb
 baseline version:
  libvirt  225aa80246d5e4a9e3a16ebd4c482525045da3db

After a quick glance I don't see a fix post-b487bb810ec9 either in
master or on the libvirt list.

Looking at the range under test it looks like one or more of John's
changes is adding parameters called index, shadowing index(3) from
strings.h.

Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH 3/3] libxl: drop virDomainObj lock when destroying a domain

[Ian Campbell]

On Wed, 2015-03-25 at 14:08 -0600, Jim Fehlig wrote:
 A destroy operation can take considerable time on large memory
 domains due to scrubbing the domain' memory.  The operation is
 running in the context of a job, so unlocking the domain and
 allowing query operations is safe.
 
 Signed-off-by: Jim Fehlig jfeh...@suse.com

I don't really know enough about the libvirt job/obj locking to comment
on the previous patches or that aspect of this one, but in general the
idea of dropping the lock before calling libxl_destroy seems reasonable
to me.

In principal you could use the async stuff (the final NULL to the call),
but you'd still be wanting to drop the lock for that period, so it's not
clear it's any better from your PoV.

Ian.

 ---
  src/libxl/libxl_domain.c | 4 
  src/libxl/libxl_driver.c | 5 -
  2 files changed, 8 insertions(+), 1 deletion(-)
 
 diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
 index e240eae..aef0157 100644
 --- a/src/libxl/libxl_domain.c
 +++ b/src/libxl/libxl_domain.c
 @@ -435,7 +435,9 @@ libxlDomainShutdownThread(void *opaque)
  libxlDomainEventQueue(driver, dom_event);
  dom_event = NULL;
  }
 +virObjectUnlock(vm);
  libxl_domain_destroy(cfg-ctx, vm-def-id, NULL);
 +virObjectLock(vm);
  libxlDomainCleanup(driver, vm, reason);
  if (!vm-persistent)
  virDomainObjListRemove(driver-domains, vm);
 @@ -447,7 +449,9 @@ libxlDomainShutdownThread(void *opaque)
  libxlDomainEventQueue(driver, dom_event);
  dom_event = NULL;
  }
 +virObjectUnlock(vm);
  libxl_domain_destroy(cfg-ctx, vm-def-id, NULL);
 +virObjectLock(vm);
  libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
  if (libxlDomainStart(driver, vm, false, -1)  0) {
  virErrorPtr err = virGetLastError();
 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index a1977c2..21e20bb 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -1250,7 +1250,10 @@ libxlDomainDestroyFlags(virDomainPtr dom,
  event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
   VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
  
 -if (libxl_domain_destroy(cfg-ctx, vm-def-id, NULL)  0) {
 +virObjectUnlock(vm);
 +ret = libxl_domain_destroy(cfg-ctx, vm-def-id, NULL);
 +virObjectLock(vm);
 +if (ret  0) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _(Failed to destroy domain '%d'), vm-def-id);
  goto endjob;


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 8/9] libxl: pass cmdline to HVM guests

[Ian Campbell]

On Fri, 2015-03-20 at 22:13 +0100, Marek Marczykowski-Górecki wrote:
 I'll definitely do. But above raises a question - how can I set extra
 arguments for qemu? In case of qemu in dom0, it's not a problem because
 I can create some wrapper script. But in case of qemu in stubdom, the
 only way is to set b_info-extra_hvm. Some additional attributes for
 emulator tag?

There are several components, please can you be precise about which you
want to pass something to.

1. qemu process command line in dom0 (used e.g. for pv backends)
2. command line of the stubdom pv kernel itself
3. qemu process command line in stubdom
4. kernel command line in dom0

It's possible that libxl doesn't fully expose all 4 of those. I'm not
sure if #2 is actually useful but it seems like we would certainly want
to be able to expose the other 3 if possible.

b_info-cmdline should be #4, I'm pretty sure.

We also have b_info-extra, -extra_pv and -extra_hvm, but I'm not
entirely sure how these map onto #1 and #3.

I'm reasonably sure that -extra maps to _both_ #1 and #3. It would seem
logical enough if -extra_hvm mapped to #3 and -extra_pv mapped to #1,
but I don't know if it actually works that way. It would probably be
quickest to confirm empirically.

With that I'll leave the mapping into livirt xml to others ;-)

Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: fix dom0 balloon logic

[Ian Campbell]

(just ccing the other tools maintainers, in particular Stefano who knows
what this stuff is supposed to do...)

On Fri, 2015-03-20 at 17:10 -0600, Jim Fehlig wrote:
 Recent testing on large memory systems revealed a bug in the Xen xl
 tool's freemem() function.  When autoballooning is enabled, freemem()
 is used to ensure enough memory is available to start a domain,
 ballooning dom0 if necessary.  When ballooning large amounts of memory
 from dom0, freemem() would exceed its self-imposed wait time and
 return an error.  Meanwhile, dom0 continued to balloon.  Starting the
 domain later, after sufficient memory was ballooned from dom0, would
 succeed.  The libvirt implementation in libxlDomainFreeMem() suffers
 the same bug since it is modeled after freemem().
 
 In the end, the best place to fix the bug on the Xen side was to
 slightly change the behavior of libxl_wait_for_memory_target().
 Instead of failing after caller-provided wait_sec, the function now
 blocks as long as dom0 memory ballooning is progressing.  It will return
 failure only when more memory is needed to reach the target and wait_sec
 have expired with no progress being made.  See xen.git commit fd3aa246.
 There was a dicussion on how this would affect other libxl apps like
 libvirt
 
 http://lists.xen.org/archives/html/xen-devel/2015-03/msg00739.html
 
 If libvirt containing this patch was build against a Xen containing
 the old libxl_wait_for_memory_target() behavior, libxlDomainFreeMem()
 will fail after 30 sec and domain creation will be terminated.
 Without this patch and with old libxl_wait_for_memory_target() behavior,
 libxlDomainFreeMem() does not succeed after 30 sec, but returns success
 anyway.  Domain creation continues resulting in all sorts of fun stuff
 like cpu soft lockups in the guest OS.  It was decided to properly fix
 libxl_wait_for_memory_target(), and if anything improve the default
 behavior of apps using the freemem reference impl in xl.
 
 xl was patched to accommodate the change in libxl_wait_for_memory_target()
 with xen.git commit 883b30a0.  This patch does the same in the libxl
 driver.  While at it, I changed the logic to essentially match
 freemem() in $xensrc/tools/libxl/xl_cmdimpl.c.  It was a bit cleaner
 IMO and will make it easier to spot future, potentially interesting
 divergences.
 
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 ---
  src/libxl/libxl_domain.c | 57 
 
  1 file changed, 28 insertions(+), 29 deletions(-)
 
 diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
 index 407a9bd..ff78133 100644
 --- a/src/libxl/libxl_domain.c
 +++ b/src/libxl/libxl_domain.c
 @@ -1121,38 +1121,41 @@ libxlDomainFreeMem(libxlDomainObjPrivatePtr priv, 
 libxl_domain_config *d_config)
  {
  uint32_t needed_mem;
  uint32_t free_mem;
 -size_t i;
 -int ret = -1;
 +int ret;
  int tries = 3;
  int wait_secs = 10;
  
 -if ((ret = libxl_domain_need_memory(priv-ctx, d_config-b_info,
 -needed_mem)) = 0) {
 -for (i = 0; i  tries; ++i) {
 -if ((ret = libxl_get_free_memory(priv-ctx, free_mem))  0)
 -break;
 +ret = libxl_domain_need_memory(priv-ctx, d_config-b_info,
 +   needed_mem);
 +if (ret  0)
 +goto error;
  
 -if (free_mem = needed_mem) {
 -ret = 0;
 -break;
 -}
 +do {
 +ret = libxl_get_free_memory(priv-ctx, free_mem);
 +if (ret  0)
 +goto error;
  
 -if ((ret = libxl_set_memory_target(priv-ctx, 0,
 -   free_mem - needed_mem,
 -   /* relative */ 1, 0))  0)
 -break;
 +if (free_mem = needed_mem)
 +return 0;
  
 -ret = libxl_wait_for_free_memory(priv-ctx, 0, needed_mem,
 - wait_secs);
 -if (ret == 0 || ret != ERROR_NOMEM)
 -break;
 +ret = libxl_set_memory_target(priv-ctx, 0,
 +  free_mem - needed_mem,
 +  /* relative */ 1, 0);
 +if (ret  0)
 +goto error;
  
 -if ((ret = libxl_wait_for_memory_target(priv-ctx, 0, 1))  0)
 -break;
 -}
 -}
 +ret = libxl_wait_for_free_memory(priv-ctx, 0, needed_mem,
 + wait_secs);
 +if (ret  0)
 +goto error;
  
 -return ret;
 +tries--;
 +} while (tries  0);
 +
 + error:
 +virReportSystemError(ret, %s,
 + _(Failed to balloon domain0 memory));
 +return -1;
  }
  
  static void
 @@ -1271,12 +1274,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
 virDomainObjPtr vm,
 priv-ctx, d_config)  0)
  

Re: [libvirt] [PATCH 8/9] libxl: pass cmdline to HVM guests

[Ian Campbell]

On Fri, 2015-03-20 at 11:18 -0600, Jim Fehlig wrote:
 Marek Marczykowski-Górecki wrote:
  Signed-off-by: Marek Marczykowski-Górecki marma...@invisiblethingslab.com
  ---
   src/libxl/libxl_conf.c | 8 
   1 file changed, 8 insertions(+)
 
  diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
  index 8ec3c75..d78d2b2 100644
  --- a/src/libxl/libxl_conf.c
  +++ b/src/libxl/libxl_conf.c
  @@ -735,6 +735,14 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
   libxl_defbool_set(b_info-device_model_stubdomain,
 def-stubdomain == VIR_TRISTATE_BOOL_YES);
   
  +if (def-os.cmdline  def-os.cmdline[0]) {
  +b_info-extra_hvm = virStringSplit(def-os.cmdline,  , 0);

 
 What is the difference between b_info-extra_hvm and b_info-cmdline? 

According to the comment in libxl_types.idl (i.e. I didn't check the
code) extra_hvm is passed to the qemu (for qemu's own consumption),
cmdline is passed to the guest (used by pv and I suppose by hvm with
direct kernel boot).

Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [xen-unstable test] 35257: regressions - FAIL

[Ian Campbell]

On Fri, 2015-02-27 at 11:51 -0700, Jim Fehlig wrote:
  2015-02-23 20:13:15.845+: 2133: error :
  virFirewallValidateBackend:193 : direct firewall backend requested,
  but /sbin/ebtables is not available: No such file or directory
 
 Odd, since ebtables was found when building
 
 checking for ebtables... /sbin/ebtables
 
 But AFAICT, that wont prevent libvirtd from starting.

The build host and the runtime host will likely be different (or at
least reinstalled).

The base set of packages should be the same, but the build one will
install a bunch of libfoo-dev while the runtime host will only get
libfoo. Perhaps some libfoo-dev is pulling in ebtables somehow while
just libfoo is not. I'll have a look next week. I think its probably
non-critical to the error here.

  I think these are just spurious.
 
  2015-02-23 20:13:15.845+: 2133: error : virFirewallApply:936 : 
  out of memory
  
 
  2015-02-23 20:13:16.092+: 2133: error : virExec:491 : Cannot 
  find 'pm-is-supported' in path: No such file or directory
  2015-02-23 20:13:16.092+: 2133: warning : virQEMUCapsInit:999 : 
  Failed to get host power management capabilities
  
  As are these two.
  
  2015-02-23 20:13:16.400+: 2133: error : virFirewallApply:936 : 
  out of memory
 
  Has these OOM messages resulted in libvirtd exiting?
 
 No, I don't think so.  The related code is
 
 int
 virFirewallApply(virFirewallPtr firewall)
 {
 size_t i, j;
 int ret = -1;
 
 virMutexLock(ruleLock);
 
 if (!firewall || firewall-err == ENOMEM) {
 virReportOOMError();
 goto cleanup;
 ...
 }
 
 I suspect 'firewall' is null, so OOM error is reported and the function
 returns -1.  But I also don't see this preventing libvirtd from
 starting.  I've cc'd the libvirt list for verification that these errors
 won't prevent libvirtd from starting.

I'm pretty sure libvirtd did successfully start, since we have
successfully done a guest start and stop.

The failing step is a second guest start, so it seems like libvirtd has
either crashed or exited.

I suppose these messages are from start of day and therefore
red-herrings wrt the reason libvirtd went away.

   I don't see any
  evidence of a crash elsewhere in the logs (i.e. no process segfaulted
  in dmesg, no OOM killing going on etc).
 
  We don't seem to collect dom0 freemem info, but that most likely
  wouldn't help given the libvirtd process has exited.
 

  Any ideas where to look next?
 
 Can you access the test environment and try starting libvirtd in the
 foreground?  Or enable debug log level in /etc/libvirt/libvirtd.conf?

The test env will have been recycled, I could try and replicate it
manually, but I think to start with I should arrange for the test env to
have more logging enabled, in the hopes that if it happens again we get
more information. I had some question around this in my reply Wei in
this thread at 1425042785.14641.188.ca...@citrix.com.

Cheers,
Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] libvirt/libxl implemetation of get_online_cpu / virNodeGetCPUMap?

[Ian Campbell]

On Wed, 2015-02-25 at 15:03 +, Daniel P. Berrange wrote:
 FWIW, this code in openstack was only added for benefit of s390
 architecture where apparently it is common to have hosts with
 CPUs offlined. Presumably you have to pay IBM for each extra
 CPU you turn online :)

Presumably :-)

OOI, why does the code care which CPUs are online rather than just the
total number (IOW why a bitmap)?

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] libvirt/libxl implemetation of get_online_cpu / virNodeGetCPUMap?

[Ian Campbell]

On Wed, 2015-02-25 at 10:24 +0100, Dario Faggioli wrote:
 On Tue, 2015-02-24 at 13:10 +, Ian Campbell wrote:
  On Tue, 2015-02-24 at 12:41 +, Anthony PERARD wrote:
 
   What libxl API those provide this information, if it exist?
   
   I found libxl_get_online_cpus() but that not enough. They want a
   bitmap.
  
  I think that is all which currently exists, at least at the libxl level,
  you may need to add a new interface.
  
  It'd be worth looking into the various host numa interfaces -- perhaps
  one of them indirectly exposes what you want?
  
 Given Daniel's latest emails, I'm not sure this is useful but
 libxl_get_cpu_topology() should put LIBXL_CPUTOPOLOGY_INVALID_ENTRY in
 all the fields of the i-eth element of the array it returns, if the
 i-eth pcpu is offline (see the implementation of XEN_SYSCTL_topologyinfo
 in xen/common/sysctl.c).
 
 So, scanning that array and constructing the bitmap according to whether
 or not we find that marker on the various elements would be the way to
 go, I would say.

It could work, yes, although if there were other reasons for INVALID
entry it would fall down.

Thinking about it, it might be a better idea long term to expose a some
specific interfaces for managing or interrogating host CPU status rather
than inferring it through other means, it's not like the hypercall would
be very hard to setup and plumb through.

But as you say, Daniel's response might have made this all moot anyway,
or at least deferrable.

Ian.

 I've actually never tested this, i.e., I've never tried offlining a pcpu
 on the host. I'll give it a go as soon as I find 5 minutes, and let know
 if it works.
 
 Regards,
 Dario


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] libvirt/libxl implemetation of get_online_cpu / virNodeGetCPUMap?

[Ian Campbell]

On Tue, 2015-02-24 at 12:41 +, Anthony PERARD wrote:
 Hi,
 
 A recent OpenStack nova commit make use of virNodeGetCPUMap to get the list
 of online cpu of a host. But this API is not implemented for the libvirt
 xen driver.
 
 The commit:
   Add handling for offlined CPUs to the nova libvirt driver.
 https://review.openstack.org/gitweb?p=openstack/nova.git;a=commitdiff;h=0696a5cd5f0fdc08951a074961bb8ce0c3310086
 
 Is there a need to use this under Xen? (Is it possible to have offline
 CPU?).

Yes, I think so. No idea how you use it though.

 What libxl API those provide this information, if it exist?
 
 I found libxl_get_online_cpus() but that not enough. They want a
 bitmap.

I think that is all which currently exists, at least at the libxl level,
you may need to add a new interface.

It'd be worth looking into the various host numa interfaces -- perhaps
one of them indirectly exposes what you want?

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [libvirt bisection] complete build-armhf-libvirt

[Ian Campbell]

On Mon, 2015-01-19 at 09:37 +, Ian Campbell wrote:
 On Mon, 2015-01-19 at 04:48 +, xen.org wrote:
  branch xen-unstable
  xen branch xen-unstable
  job build-armhf-libvirt
  test libvirt-build
  
  Tree: libvirt git://libvirt.org/libvirt.git
  Tree: qemuu git://xenbits.xen.org/staging/qemu-upstream-unstable.git
  Tree: xen git://xenbits.xen.org/xen.git
  
  *** Found and reproduced problem changeset ***
  
Bug is in tree:  libvirt git://libvirt.org/libvirt.git
Bug introduced:  2c78051a14acfb7aba078d569b1632dfe0ca0853
Bug not present: 7ad117b2e33039737126ce9df7a267a6f939988d
  
  
commit 2c78051a14acfb7aba078d569b1632dfe0ca0853
Author: Kiarie Kahurani davidkiar...@gmail.com
Date:   Thu Sep 11 07:10:33 2014 +0300

src/xenconfig: Xen-xl parser
 
 This has since been reverted and done again differently, so nothing to
 worry about I think.

That said the latest failure is 
http://www.chiark.greenend.org.uk/~xensrcts/logs/33553/build-amd64-libvirt/5.ts-libvirt-build.log
  CC virdbustest-testutils.o
  CC virsystemdtest-virsystemdtest.o
  CC virsystemdtest-testutils.o
  CC virdrivermoduletest.o
  CCLD   xlconfigtest
  CC qemuxml2argvtest.o
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../x86_64-linux-gnu/crt1.o: In 
function `_start':
(.text+0x20): undefined reference to `main'
collect2: error: ld returned 1 exit status
(from http://www.chiark.greenend.org.uk/~xensrcts/logs/33553/ )

The bisector is working on it, partial result at:
http://www.chiark.greenend.org.uk/~xensrcts/results/bisect.libvirt.build-amd64-libvirt.libvirt-build.html

I'll mail the result when it arrives.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 1/3] Add virXMLValidateAgainstSchema helper method

[Ian Campbell]

Hello,

On Tue, 2015-01-13 at 17:00 +, Daniel P. Berrange wrote:
 +#  define VIR_WARNINGS_NO_PRINTF \
 +_Pragma (GCC diagnostic push) \
 +_Pragma (GCC diagnostic ignored \-Wsuggest-attribute=format\)

Xen automated tests are failing to build on all architectures with:

util/virxml.c: In function 'catchRNGError':
util/virxml.c:1094:9: error: unknown option after '#pragma GCC 
diagnostic' kind [-Werror=pragmas]

which I think must be down to one of these additions.

(helpful of gcc not to print the unknown option in question!)

test overview:
http://www.chiark.greenend.org.uk/~xensrcts/logs/33443/
specific failure log:
http://www.chiark.greenend.org.uk/~xensrcts/logs/33443/build-amd64-libvirt/5.ts-libvirt-build.log

We use Debian Wheezy's gcc, which is 4.6.3 AFAIK.

Cheers,
Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 1/3] Add virXMLValidateAgainstSchema helper method

[Ian Campbell]

On Fri, 2015-01-16 at 14:19 +, Daniel P. Berrange wrote:
 On Fri, Jan 16, 2015 at 01:58:27PM +, Ian Campbell wrote:
  Hello,
  
  On Tue, 2015-01-13 at 17:00 +, Daniel P. Berrange wrote:
   +#  define VIR_WARNINGS_NO_PRINTF \
   +_Pragma (GCC diagnostic push) \
   +_Pragma (GCC diagnostic ignored \-Wsuggest-attribute=format\)
  
  Xen automated tests are failing to build on all architectures with:
  
  util/virxml.c: In function 'catchRNGError':
  util/virxml.c:1094:9: error: unknown option after '#pragma GCC 
  diagnostic' kind [-Werror=pragmas]
  
  which I think must be down to one of these additions.
  
  (helpful of gcc not to print the unknown option in question!)
  
  test overview:
  http://www.chiark.greenend.org.uk/~xensrcts/logs/33443/
  specific failure log:
  http://www.chiark.greenend.org.uk/~xensrcts/logs/33443/build-amd64-libvirt/5.ts-libvirt-build.log
  
  We use Debian Wheezy's gcc, which is 4.6.3 AFAIK.
 
 The configure logs show
 
 checking whether C compiler handles -Wsuggest-attribute=const... yes
 checking whether C compiler handles -Wsuggest-attribute=format... no
 checking whether C compiler handles -Wsuggest-attribute=noreturn... yes
 checking whether C compiler handles -Wsuggest-attribute=pure... yes
 
 So, can someone with a Debian machine check if it helps to modify
 
 _Pragma (GCC diagnostic ignored \-Wsuggest-attribute=format\)
 
 To be just
 
 _Pragma (GCC diagnostic ignored \-Wsuggest-attribute\)

I'm afraid it doesn't seem to. Specifically:
diff --git a/src/internal.h b/src/internal.h
index 9855c49..508f8b5 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -236,7 +236,7 @@
 _Pragma (GCC diagnostic ignored \-Wcast-align\)
 #  define VIR_WARNINGS_NO_PRINTF \
 _Pragma (GCC diagnostic push) \
-_Pragma (GCC diagnostic ignored \-Wsuggest-attribute=format\)
+_Pragma (GCC diagnostic ignored \-Wsuggest-attribute\)
 
 #  define VIR_WARNINGS_RESET \
 _Pragma (GCC diagnostic pop)

Didn't help.

According to
https://gcc.gnu.org/onlinedocs/gcc-4.6.3/gcc/Warning-Options.html#Warning-Options
 the valid -Wsuggest-attributes=FOO in that version are pure const and noreturn.

=format seems to have arrived in 4.8, FWIW.

Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Conditionalize use of -Wno-suggest-attribute=format pragma

[Ian Campbell]

On Fri, 2015-01-16 at 14:55 +, Daniel P. Berrange wrote:
 Many GCC versions don't understand -Wno-suggest-attribute=format
 so the pragma must only be used when supported

That seems to have done the trick:
Tested-by: Ian Campbell ian.campb...@citrix.com


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH 00/12] Replace Xen xl parsing/formatting impl

[Ian Campbell]

On Fri, 2015-01-09 at 22:03 -0700, Jim Fehlig wrote:
 The first attempt to implement support for parsing/formatting Xen's
 xl disk config format copied Xen's flex-based parser into libvirt, which
 has proved to be challenging in the context of autotools.  But as it turns
 out, Xen provides an interface to the parser via libxlutil.
 
 This series reverts the first attempt, along with subsequent attempts to
 fix it, and replaces it with an implementation based on libxlutil.  The
 first nine patches revert the original implementation and subsequent fixes.
 Patch 10 provides an implemenation based on libxlutil.  Patches 11 and
 12 are basically unchanged from patches 3 and 4 in the first attempt.
 
 One upshot of using libxlutil instead of copying the flex source is
 removing the potential for source divergence.

Thanks for doing this, looks good to me, FWIW.

Is the presence/absence of xen-xl support exposed via virsh anywhere? If
so then I can arrange for my Xen osstest patches for libvirt testing to
use xen-xl when available but still fallback to xen-xm. I've had a look
in virsh capabilities and virsh help domxml-from-native but not
seeing xen-xm, so assuming xen-xl won't magically appear in any of those
places either.

(TBH, this may become moot since I suspect your patches will be well
established by the time my osstest patches hit osstest...)

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH 00/12] Replace Xen xl parsing/formatting impl

[Ian Campbell]

On Mon, 2015-01-12 at 09:23 -0700, Jim Fehlig wrote:
 Ian Campbell wrote:
  On Fri, 2015-01-09 at 22:03 -0700, Jim Fehlig wrote:

  The first attempt to implement support for parsing/formatting Xen's
  xl disk config format copied Xen's flex-based parser into libvirt, which
  has proved to be challenging in the context of autotools.  But as it turns
  out, Xen provides an interface to the parser via libxlutil.
 
  This series reverts the first attempt, along with subsequent attempts to
  fix it, and replaces it with an implementation based on libxlutil.  The
  first nine patches revert the original implementation and subsequent fixes.
  Patch 10 provides an implemenation based on libxlutil.  Patches 11 and
  12 are basically unchanged from patches 3 and 4 in the first attempt.
 
  One upshot of using libxlutil instead of copying the flex source is
  removing the potential for source divergence.
  
 
  Thanks for doing this, looks good to me, FWIW.
 
  Is the presence/absence of xen-xl support exposed via virsh anywhere? If
  so then I can arrange for my Xen osstest patches for libvirt testing to
  use xen-xl when available but still fallback to xen-xm. I've had a look
  in virsh capabilities and virsh help domxml-from-native but not
  seeing xen-xm, so assuming xen-xl won't magically appear in any of those
  places either.

 
 AFAIK, the only place the supported native formats are listed is in the
 virsh man page.

Not to worry, I think I'll just use xen-xl everywhere then, osstest's
handling of test failures and regression detection will do the right
thing with versions of libvirt which don't have this applied.

   But thanks for the question, else I would have missed
 adding xen-xl to the man page in 12/12.

No problem ;-)

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 1/2] src/Makefile: move the new xen_xl_disk parser code at the correct place

[Ian Campbell]

On Thu, 2015-01-08 at 16:43 -0700, Jim Fehlig wrote:
 Eric Blake wrote:
  On 01/08/2015 06:20 AM, Pavel Hrdina wrote:

  Signed-off-by: Pavel Hrdina phrd...@redhat.com
  ---
   src/Makefile.am | 34 +-
   1 file changed, 17 insertions(+), 17 deletions(-)
  
 

   
   if WITH_XENCONFIG
  +AM_LFLAGS = -Pxl_disk_ --header-file=../$*.h
  
 
  Uggh.  Not your fault (this patch was just code motion), but it
  highlights a portability problem: RHEL 5 ships with flex 2.5.4, which
  lacks --header-file, and which spells --outfile only as -o.  The build
  is failing with:
 
  flex  -Pxl_disk_
  --header-file=/home/dummy/libvirt/src/xenconfig/xen_xl_disk.h
  --outfile=/home/dummy/libvirt/src/xenconfig/xen_xl_disk.c
  xenconfig/xen_xl_disk.l
  flex: unknown flag '-'.  For usage, try
  flex --help

 
 Sigh...  Sorry for all the grief this has caused.  I wasted a huge
 amount of time trying to get all of this working right, including some
 of the approaches in the follow-up patches, but always hit some sort of
 problem.  I thought I finally had everything worked out with the V3
 patches that Michal ACKed.  But given all the problems I encountered
 along the way, I suppose I shouldn't be surprised by the problems folks
 are seeing.  So now I'm causing others to waste time :-(.  Again, I'm
 sorry for that.
 
 And of course this has rippled to the Xen push gate tests too.

NB only our libvirt push gate, which isn't really gating anything, the
result just becomes the baseline for next time and becomes what is used
for testing !libvirt.

So it's not currently impacting our testing of anything other than new
versions of libvirt. Testing of xen-unstable and stable will keep using
the old/working version of libvirt until it is fixed.

 I suppose it is possible to include some minimal header in libvirt (or
 an extern declaration?) for the xlu_disk_parse() function, and actually
 link against the libxlutil.so which is installed.  I can work on such an
 approach if folks think it is worthwhile.

FWIW MHO is that sticking the hack in an #ifndef HAVE_LIBXLUTIL_H (from
AC_CHECK_HEADER) isn't so terrible
(http://lists.xen.org/archives/html/xen-devel/2015-01/msg00715.html)

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Ian Campbell]

On Tue, 2015-01-06 at 17:20 +, Wei Liu wrote:
 On Tue, Jan 06, 2015 at 05:03:59PM +, Ian Campbell wrote:
  On Tue, 2015-01-06 at 16:12 +, 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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Ian Campbell]

On Tue, 2015-01-06 at 16:12 +, 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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH V2] libxl: Set path to console on domain startup.

[Ian Campbell]

On Mon, 2014-12-15 at 17:07 +, Anthony PERARD wrote:
 On Thu, Dec 11, 2014 at 04:23:15PM +, Ian Campbell wrote:
  On Thu, 2014-12-11 at 16:16 +, Anthony PERARD wrote:
   On Tue, Dec 09, 2014 at 03:56:02PM +, Ian Campbell wrote:
On Tue, 2014-12-09 at 15:39 +, Anthony PERARD wrote:
 The path to the pty of a Xen PV console is set only in
 virDomainOpenConsole. But this is done too late. A call to
 virDomainGetXMLDesc done before OpenConsole will not have the path to
 the pty, but a call after OpenConsole will.
 
 e.g. of the current issue.
 Starting a domain with 'console type=pty/'
 Then:
 virDomainGetXMLDesc():
   devices
 console type='pty'
   target type='xen' port='0'/
 /console
   /devices
 virDomainOpenConsole()
 virDomainGetXMLDesc():
   devices
 console type='pty' tty='/dev/pts/30'
   source path='/dev/pts/30'/
   target type='xen' port='0'/
 /console
   /devices
 
 The patch intend to have the TTY path on the first call of GetXMLDesc.
 This is done by setting up the path at domain start up instead of in
 OpenConsole.
 
 https://bugzilla.redhat.com/show_bug.cgi?id=1170743
 
 Signed-off-by: Anthony PERARD anthony.per...@citrix.com
 
 ---
 Change in V2:
   Adding bug report link.
   Reword the last part of the patch description.
   Cleanup the code.
   Use VIR_FREE before VIR_STRDUP.
   Remove the code from OpenConsole as it is now a duplicate.
 ---
  src/libxl/libxl_domain.c | 20 
  src/libxl/libxl_driver.c | 15 ---
  2 files changed, 20 insertions(+), 15 deletions(-)
 
 diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
 index 9c62291..325de79 100644
 --- a/src/libxl/libxl_domain.c
 +++ b/src/libxl/libxl_domain.c
 @@ -1290,6 +1290,26 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
 virDomainObjPtr vm,
  if (libxlDomainSetVcpuAffinities(driver, vm)  0)
  goto cleanup_dom;
  
 +if (vm-def-nconsoles) {
 +virDomainChrDefPtr chr = vm-def-consoles[0];

Given vm-def-nconsoles should we loop and do them all?
   
   Maybe. libvirt it self will not be able to access any console that is
   not the first one (virDomainOpenConsole only provide access to console
   0), but a consumer of libvirt could.
   
Also, and I really should know the answer to this (and sorry for not
thinking of it earlier), but:

 +ret = libxl_console_get_tty(priv-ctx, vm-def-id,
 +chr-target.port, 
 console_type,
 +console);

Might this race against xenconsoled writing the node to xenstore and
therefore be prone to failing when starting multiple guests all at once?
   
   I've look through out libxl, xenconsoled and libvirt, and I could not
   find any synchronisation point. So I guest it's possible.
   
Is there a hook which is called on virsh dumpxml which could update
things on the fly (i.e. lookup the console iff it isn't already set)?
   
   I guest we could modify libxlDomainGetXMLDesc and libxlDomainOpenConsole
   to do the check, or having a xenstore watch on the path (if that is
   possible).
  
  The aop_console_how option to libxl_domain_create_new and
  libxl_domain_create_restore is documented as:
  
/* A progress report will be made via ao_console_how, of type
 * domain_create_console_available, when the domain's primary
 * console is available and can be connected to.
 */
  
  Which sounds like exactly what is needed?
 
 It looks like the progress is reported before the main function finish,
 from the description of the type libxl_asyncprogress_how (in libxl.h).

Correct.

 Also, I tryied to use it, it works if xenconsoled is running. But if
 xenconsoled is not running, then the callback is also called and
 libxl_console_get_tty() return an empty string.

I'm not sure xenconsoled not running is a configuration we need to worry
about, or at least it could be expected not to get a console in that
case.

Unless by not running you meant bottlenecked or not keeping up
perhaps.

 Or, maybe my test case is not relevant, so another question: Will
 libxl wait for xenconsoled to process the new domain before calling the
 callback?

I don't see an explicit wait in the code, but I don't know if it has
arranged the sequencing some other way.

 Or, should I set the callback to NULL and have the
 domain_create_console_available event sent through
 the callback set by libxl_event_register_callbacks()?

That would make sense, except I don't see libxl_evdisable_foo for these
events, so I'm not sure it is possible.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH V2] libxl: Set path to console on domain startup.

[Ian Campbell]

On Tue, 2014-12-16 at 12:36 +, Ian Jackson wrote:
 Anthony PERARD writes (Re: [Xen-devel] [PATCH V2] libxl: Set path to console 
 on domain startup.):
  On Tue, Dec 16, 2014 at 09:30:28AM +, Ian Campbell wrote:
   Unless by not running you meant bottlenecked or not keeping up
   perhaps.
  
  Well, I did meant no xenconsoled process. But after, I also tried `kill
  -STOP`, but the same things is happening.
 
 I think this is a bug.  I imagine that you can cause `xl create -c' to
 malfunction too.

Presumably the libxl side fix is to register an ev_xswatch (and an
ev_timeout) on the console path (which differs by guest type, see guts
of libxl_console_get_tty) and call the callback out of that instead of
directly at the end of domcreate_

Depending on how racy this is (not very, I suspect) libvirt could ignore
this possibility and assume the event works (and we'll independently fix
it so it always does), or it could be a bit belt and braces and in
addition to handling the callback also check for NULL and-recheck on the
libvirt internal accessors.

Anthony, you should also note that the callback can happen more than
once, e.g. if you are using pygrub you get it once for the bootloader
pty and then again for the actual guests pty.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] xenconfig: fix boot device parsing

[Ian Campbell]

On Sun, 2014-12-14 at 14:58 +, Wei Liu wrote:
 The original code always checked *boot which was in effect boot[0]. It
 should use boot[i].
 
 Signed-off-by: Wei Liu wei.l...@citrix.com

Acked-by: Ian Campbell ian.campb...@citrix.com


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH V2] libxl: Set path to console on domain startup.

[Ian Campbell]

On Thu, 2014-12-11 at 16:16 +, Anthony PERARD wrote:
 On Tue, Dec 09, 2014 at 03:56:02PM +, Ian Campbell wrote:
  On Tue, 2014-12-09 at 15:39 +, Anthony PERARD wrote:
   The path to the pty of a Xen PV console is set only in
   virDomainOpenConsole. But this is done too late. A call to
   virDomainGetXMLDesc done before OpenConsole will not have the path to
   the pty, but a call after OpenConsole will.
   
   e.g. of the current issue.
   Starting a domain with 'console type=pty/'
   Then:
   virDomainGetXMLDesc():
 devices
   console type='pty'
 target type='xen' port='0'/
   /console
 /devices
   virDomainOpenConsole()
   virDomainGetXMLDesc():
 devices
   console type='pty' tty='/dev/pts/30'
 source path='/dev/pts/30'/
 target type='xen' port='0'/
   /console
 /devices
   
   The patch intend to have the TTY path on the first call of GetXMLDesc.
   This is done by setting up the path at domain start up instead of in
   OpenConsole.
   
   https://bugzilla.redhat.com/show_bug.cgi?id=1170743
   
   Signed-off-by: Anthony PERARD anthony.per...@citrix.com
   
   ---
   Change in V2:
 Adding bug report link.
 Reword the last part of the patch description.
 Cleanup the code.
 Use VIR_FREE before VIR_STRDUP.
 Remove the code from OpenConsole as it is now a duplicate.
   ---
src/libxl/libxl_domain.c | 20 
src/libxl/libxl_driver.c | 15 ---
2 files changed, 20 insertions(+), 15 deletions(-)
   
   diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
   index 9c62291..325de79 100644
   --- a/src/libxl/libxl_domain.c
   +++ b/src/libxl/libxl_domain.c
   @@ -1290,6 +1290,26 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
   virDomainObjPtr vm,
if (libxlDomainSetVcpuAffinities(driver, vm)  0)
goto cleanup_dom;

   +if (vm-def-nconsoles) {
   +virDomainChrDefPtr chr = vm-def-consoles[0];
  
  Given vm-def-nconsoles should we loop and do them all?
 
 Maybe. libvirt it self will not be able to access any console that is
 not the first one (virDomainOpenConsole only provide access to console
 0), but a consumer of libvirt could.
 
  Also, and I really should know the answer to this (and sorry for not
  thinking of it earlier), but:
  
   +ret = libxl_console_get_tty(priv-ctx, vm-def-id,
   +chr-target.port, console_type,
   +console);
  
  Might this race against xenconsoled writing the node to xenstore and
  therefore be prone to failing when starting multiple guests all at once?
 
 I've look through out libxl, xenconsoled and libvirt, and I could not
 find any synchronisation point. So I guest it's possible.
 
  Is there a hook which is called on virsh dumpxml which could update
  things on the fly (i.e. lookup the console iff it isn't already set)?
 
 I guest we could modify libxlDomainGetXMLDesc and libxlDomainOpenConsole
 to do the check, or having a xenstore watch on the path (if that is
 possible).

The aop_console_how option to libxl_domain_create_new and
libxl_domain_create_restore is documented as:

  /* A progress report will be made via ao_console_how, of type
   * domain_create_console_available, when the domain's primary
   * console is available and can be connected to.
   */

Which sounds like exactly what is needed?

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH V2] libxl: Set path to console on domain startup.

[Ian Campbell]

On Tue, 2014-12-09 at 15:39 +, Anthony PERARD wrote:
 The path to the pty of a Xen PV console is set only in
 virDomainOpenConsole. But this is done too late. A call to
 virDomainGetXMLDesc done before OpenConsole will not have the path to
 the pty, but a call after OpenConsole will.
 
 e.g. of the current issue.
 Starting a domain with 'console type=pty/'
 Then:
 virDomainGetXMLDesc():
   devices
 console type='pty'
   target type='xen' port='0'/
 /console
   /devices
 virDomainOpenConsole()
 virDomainGetXMLDesc():
   devices
 console type='pty' tty='/dev/pts/30'
   source path='/dev/pts/30'/
   target type='xen' port='0'/
 /console
   /devices
 
 The patch intend to have the TTY path on the first call of GetXMLDesc.
 This is done by setting up the path at domain start up instead of in
 OpenConsole.
 
 https://bugzilla.redhat.com/show_bug.cgi?id=1170743
 
 Signed-off-by: Anthony PERARD anthony.per...@citrix.com
 
 ---
 Change in V2:
   Adding bug report link.
   Reword the last part of the patch description.
   Cleanup the code.
   Use VIR_FREE before VIR_STRDUP.
   Remove the code from OpenConsole as it is now a duplicate.
 ---
  src/libxl/libxl_domain.c | 20 
  src/libxl/libxl_driver.c | 15 ---
  2 files changed, 20 insertions(+), 15 deletions(-)
 
 diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
 index 9c62291..325de79 100644
 --- a/src/libxl/libxl_domain.c
 +++ b/src/libxl/libxl_domain.c
 @@ -1290,6 +1290,26 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
 virDomainObjPtr vm,
  if (libxlDomainSetVcpuAffinities(driver, vm)  0)
  goto cleanup_dom;
  
 +if (vm-def-nconsoles) {
 +virDomainChrDefPtr chr = vm-def-consoles[0];

Given vm-def-nconsoles should we loop and do them all?

Also, and I really should know the answer to this (and sorry for not
thinking of it earlier), but:

 +ret = libxl_console_get_tty(priv-ctx, vm-def-id,
 +chr-target.port, console_type,
 +console);

Might this race against xenconsoled writing the node to xenstore and
therefore be prone to failing when starting multiple guests all at once?

Is there a hook which is called on virsh dumpxml which could update
things on the fly (i.e. lookup the console iff it isn't already set)?

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: Set path to console on domain startup.

[Ian Campbell]

On Fri, 2014-12-05 at 16:30 +, Anthony PERARD wrote:

Jim Fehlig maintains the libxl driver in libvirt, so you should CC him
(I've done so here...)

 The path to the pty of a Xen PV console is set only in
 virDomainOpenConsole. But this is done too late. A call to
 virDomainGetXMLDesc done before OpenConsole will not have the path to
 the pty, but a call after OpenConsole will.
 
 e.g. of the current issue.
 Starting a domain with 'console type=pty/'
 Then:
 virDomainGetXMLDesc():
   devices
 console type='pty'
   target type='xen' port='0'/
 /console
   /devices
 virDomainOpenConsole()
 virDomainGetXMLDesc():
   devices
 console type='pty' tty='/dev/pts/30'
   source path='/dev/pts/30'/
   target type='xen' port='0'/
 /console
   /devices
 
 The patch intend to get the tty path on the first call of GetXMLDesc.

Doesn't it actually do it on domain start (which makes more sense to me
anyway).

 
 Signed-off-by: Anthony PERARD anthony.per...@citrix.com
 ---
  src/libxl/libxl_domain.c | 17 +
  1 file changed, 17 insertions(+)
 
 diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
 index 9c62291..de56054 100644
 --- a/src/libxl/libxl_domain.c
 +++ b/src/libxl/libxl_domain.c
 @@ -1290,6 +1290,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
 virDomainObjPtr vm,
  if (libxlDomainSetVcpuAffinities(driver, vm)  0)
  goto cleanup_dom;
  
 +if (vm-def-nconsoles) {
 +virDomainChrDefPtr chr = NULL;
 +chr = vm-def-consoles[0];
 +if (chr  chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY) {
 +libxl_console_type console_type;
 +char *console = NULL;
 +console_type =
 +(chr-targetType == 
 VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ?
 + LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV);
 +ret = libxl_console_get_tty(priv-ctx, vm-def-id, 
 chr-target.port,
 +console_type, console);
 +if (!ret)
 +ignore_value(VIR_STRDUP(chr-source.data.file.path, 
 console));

libxlDomainOpenConsole will strdup another (well, probably the same)
value here, causing a leak I think, so you'll need some check there too
I expect.

 +VIR_FREE(console);
 +}
 +}
 +
  if (!start_paused) {
  libxl_domain_unpause(priv-ctx, domid);
  virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, 
 VIR_DOMAIN_RUNNING_BOOTED);


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: Set path to console on domain startup.

[Ian Campbell]

On Mon, 2014-12-08 at 15:11 +, Anthony PERARD wrote:
   The patch intend to get the tty path on the first call of GetXMLDesc.
  
  Doesn't it actually do it on domain start (which makes more sense to me
  anyway).
 
 Just a wording issue. I meant: Have GetXMLDesc always return the path to
 the tty.
 

Ah, yes, I get what you meant now.

   
   Signed-off-by: Anthony PERARD anthony.per...@citrix.com
   ---
src/libxl/libxl_domain.c | 17 +
1 file changed, 17 insertions(+)
   
   diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
   index 9c62291..de56054 100644
   --- a/src/libxl/libxl_domain.c
   +++ b/src/libxl/libxl_domain.c
   @@ -1290,6 +1290,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
   virDomainObjPtr vm,
if (libxlDomainSetVcpuAffinities(driver, vm)  0)
goto cleanup_dom;

   +if (vm-def-nconsoles) {
   +virDomainChrDefPtr chr = NULL;
   +chr = vm-def-consoles[0];
   +if (chr  chr-source.type == VIR_DOMAIN_CHR_TYPE_PTY) {
   +libxl_console_type console_type;
   +char *console = NULL;
   +console_type =
   +(chr-targetType == 
   VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ?
   + LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV);
   +ret = libxl_console_get_tty(priv-ctx, vm-def-id, 
   chr-target.port,
   +console_type, console);
   +if (!ret)
   +ignore_value(VIR_STRDUP(chr-source.data.file.path, 
   console));
  
  libxlDomainOpenConsole will strdup another (well, probably the same)
  value here, causing a leak I think, so you'll need some check there too
  I expect.
 
 So, if OpenConsole is call twice, there will also be a leak? Or maybe it
 can not be called twice.

I'm not sure, there may be an existing issue here I suppose.

 Anyway, I though from the use of VIR_STRDUP there that it was safe to
 call VIR_STRDUP several times and it well free the destination. I might
 be wrong.

I wondered about that, but AFAICS VIR_STRDUP is a wrapper around
virStrdup and virStrdup doesn't free any existing destination.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] docs: Correct invalid hyperlinks

[Ian Campbell]

On Tue, 2014-12-02 at 07:50 +0100, Martin Kletzander wrote:
 That said, I found out that that XML-XPath is only needed when
 creating the tarball, but (probably) not when building an RPM with it.

It's needed when building from the git tree to, Xen automated testing
picked up on this overnight:
http://www.chiark.greenend.org.uk/~xensrcts/logs/32083/build-amd64-libvirt/5.ts-libvirt-build.log

I'm not sure if things which are only needed to build the tarball are
supposed to be checked for by configure or not.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] docs: Create html documentation even if XHTML1 DTD is not available to validate

[Ian Campbell]

On a Debian system lacking the w3c-dtd-xhtml package the build fails
with:

$ make -C docs/ formatcaps.html
make: Entering directory '/local/scratch/ianc/devel/libvirt.git/docs'
Generating formatcaps.html.tmp
I/O error : Attempt to load network entity 
http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd
formatcaps.html.in:2: warning: failed to load external entity 
http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd;
C//DTD XHTML 1.0 Strict//EN 
http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd;

   ^
I/O error : Attempt to load network entity 
http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd
../docs/sitemap.html.in:2: warning: failed to load external entity 
http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd;
C//DTD XHTML 1.0 Strict//EN 
http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd;

   ^
missing XHTML1 DTD
rm formatcaps.html.tmp
make: Leaving directory '/local/scratch/ianc/devel/libvirt.git/docs'
$ ls docs/formatcaps*
docs/formatcaps.html.in

https://www.redhat.com/archives/libvir-list/2009-November/msg00413.html
suggests that the XHTML1 DTD should not be a hard requirement and the
docs should be generated but not validated if it is not available.

Therefore when the DTD is not available arrange for the .html.tmp file
to be propagated to the .html output.

Signed-off-by: Ian Campbell ian.campb...@citrix.com
Cc: Daniel Veillard veill...@redhat.com
---
 docs/Makefile.am | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/Makefile.am b/docs/Makefile.am
index 5485ee9..c5ba688 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -232,7 +232,7 @@ internals/%.html.tmp: internals/%.html.in subsite.xsl 
page.xsl sitemap.html.in
  SGML_CATALOG_FILES='$(XML_CATALOG_FILE)' \
  $(XMLLINT) --catalogs --nonet --format --valid $  $(srcdir)/$@ \
  || { rm $(srcdir)/$@  exit 1; }; \
- else echo missing XHTML1 DTD ; fi ; fi
+ else echo missing XHTML1 DTD; cat $  $(srcdir)/$@ ; fi ; fi
 
 %.php.tmp: %.php.in site.xsl page.xsl sitemap.html.in
@if [ -x $(XSLTPROC) ] ; then \
@@ -258,7 +258,7 @@ html/index.html: libvirt-api.xml newapi.xsl page.xsl 
sitemap.html.in
 /dev/null ; then \
  SGML_CATALOG_FILES='$(XML_CATALOG_FILE)' \
  $(XMLLINT) --catalogs --nonet --valid --noout $(srcdir)/html/*.html ; 
\
- else echo missing XHTML1 DTD ; fi ; fi
+ else echo missing XHTML1 DTD; cat $  $(srcdir)/$@ ; fi ; fi
 
 $(addprefix $(srcdir)/,$(devhelphtml)): $(srcdir)/libvirt-api.xml $(devhelpxsl)
$(AM_V_GEN)if [ -x $(XSLTPROC) ] ; then \
-- 
2.1.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] docs: Create html documentation even if XHTML1 DTD is not available to validate

[Ian Campbell]

On Fri, 2014-11-28 at 14:36 +, Ian Campbell wrote:
 On a Debian system lacking the w3c-dtd-xhtml package the build fails

On a somewhat related note on a system without xsltproc configure
succeeds but the build does not:
$ make -C docs csharp.html
make: Entering directory '/local/scratch/ianc/devel/libvirt.git/docs'
missing XHTML1 DTD
/bin/cat: csharp.html.tmp: No such file or directory
Makefile:2353: recipe for target 'csharp.html' failed
make: *** [csharp.html] Error 1
make: Leaving directory '/local/scratch/ianc/devel/libvirt.git/docs'

This is because the %.html.tmp silently does nothing if xsltproc isn't
available. (aside: the use of @ all over docs/Makefile.am makes it quite
hard to diagnose this sort of issue...)

Is it valid to just cp %.html.in in this case or is the xsltproc stuff
doing something essential?

If copying isn't valid then is the preference to fail at configure time
if xsltproc cannot be found or to not build the docs?

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH 1/2] virdbus: don't force users to pass int for bool values

[Ian Campbell]

On Mon, 2014-11-24 at 09:21 -0700, Eric Blake wrote:
 On 11/24/2014 02:43 AM, Ian Campbell wrote:
 
 
  I think this change breaks build on FreeBSD:
 
CC   util/libvirt_util_la-virdbus.lo
  util/virdbus.c:956:13: error: cast from 'bool *' to 'dbus_bool_t *' (aka 
  'unsigned int *') increases required alignment from 1 to 4 
  [-Werror,-Wcast-align]
  GET_NEXT_VAL(dbus_bool_t, bool_val, bool, %d);
  ^~~
  util/virdbus.c:858:17: note: expanded from macro 'GET_NEXT_VAL'
  x = (dbustype *)(*xptrptr + (*narrayptr - 1));  \
  ^ 1 error 
  generated.
 
  Valid complaint, so I'll have to figure something out to avoid it.  :(
 
  I'm, guessing that this is the same underlying issue as:
  util/virdbus.c: In function 'virDBusMessageIterDecode':
  util/virdbus.c:956:346: error: cast increases required alignment of 
  target type [-Werror=cast-align]
 
 Yes.
 
  
  which we are seeing in the Xen automated tests [0, 1] (on armhf only,
  probably compiler dependent?).
 
 So, do I have an ACK on my proposed fix yet? :)
 https://www.redhat.com/archives/libvir-list/2014-November/msg00838.html

Well, FWIW it looks good to me:

Acked-by: Ian Campbell ian.campb...@citrix.com

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] virdbus: don't force users to pass int for bool values

[Ian Campbell]

On Fri, 2014-11-21 at 17:16 -0700, Eric Blake wrote:
 On 11/20/2014 04:17 PM, Eric Blake wrote:
  On 11/20/2014 08:12 AM, Conrad Meyer wrote:
  Hi Eric,
 
  I think this change breaks build on FreeBSD:
 
CC   util/libvirt_util_la-virdbus.lo
  util/virdbus.c:956:13: error: cast from 'bool *' to 'dbus_bool_t *' (aka 
  'unsigned int *') increases required alignment from 1 to 4 
  [-Werror,-Wcast-align]
  GET_NEXT_VAL(dbus_bool_t, bool_val, bool, %d);
  ^~~
  util/virdbus.c:858:17: note: expanded from macro 'GET_NEXT_VAL'
  x = (dbustype *)(*xptrptr + (*narrayptr - 1));  \
  ^ 1 error 
  generated.
  
  Valid complaint, so I'll have to figure something out to avoid it.  :(
  Thanks for the heads up.
 
 Thanks again for flagging this!
 
 We had a pre-existing bug.  Even something like an was broken on
 encoding, because even though type 'n' (int16_t) promotes to a full
 'int' when parsed via varargs, passing an array of shorts and then
 dereferencing it via (int*) will read beyond array bounds.  We had a
 hole in our testsuite for never testing arrayref with sub-int types,
 even before my commit added 'bool *' to the list of sub-int types that
 we now need to support.

I'm, guessing that this is the same underlying issue as:
util/virdbus.c: In function 'virDBusMessageIterDecode':
util/virdbus.c:956:346: error: cast increases required alignment of 
target type [-Werror=cast-align]

which we are seeing in the Xen automated tests [0, 1] (on armhf only,
probably compiler dependent?).

Ian.

[0] 
http://www.chiark.greenend.org.uk/~xensrcts/logs/31787/build-armhf-libvirt/5.ts-libvirt-build.log
[1] 
http://www.chiark.greenend.org.uk/~xensrcts/logs/31787/build-armhf-libvirt/info.html


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] dom0 kenrel crashes for openstack + libvirt + libxl

[Ian Campbell]

On Fri, 2014-11-21 at 09:28 +, Ian Campbell wrote:
 I think libvirt is wrong to specify an absolute path here, IMHO by
 default it should just specify pygrub and let libxl figure out the
 correct path. Jim, what do you think?

e.g. something like the following untested (but pretty obvious[0])
patch.

I'm currently rebuilding Debian's libvirt package with this included so
I can give it a go.

Ian.

[0] famous last words, I know!

8---

From 4edbcbdc7e28896121832d8e226e7aeccf30633c Mon Sep 17 00:00:00 2001
From: Ian Campbell ian.campb...@citrix.com
Date: Fri, 21 Nov 2014 14:00:38 +
Subject: [PATCH] libxl: Allow libxl to find pygrub binary.

Specifying an explicit path to pygrub (e.g. BINDIR /pygrub) only works if
Xen and libvirt happen to be installed to the same prefix. A more flexible
approach is to simply specify pygrub which will cause libxl to use the
correct path which it knows (since it is built with the same prefix as pygrub).

This is particular problematic in the Debian packaging, since the Debian Xen
package relocates pygrub into a libexec dir, however I think this change makes
sense upstream.

Signed-off-by: Ian Campbell ian.campb...@citrix.com
---
 src/libxl/libxl_conf.h |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 25f77ea..3669e68 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -53,7 +53,7 @@
 # define LIBXL_LIB_DIR LOCALSTATEDIR /lib/libvirt/libxl
 # define LIBXL_SAVE_DIR LIBXL_LIB_DIR /save
 # define LIBXL_DUMP_DIR LIBXL_LIB_DIR /dump
-# define LIBXL_BOOTLOADER_PATH BINDIR /pygrub
+# define pygrub
 
 /* libxl interface for setting VCPU affinity changed in 4.5. In fact, a new
  * parameter has been added, representative of 'VCPU soft affinity'. If one
-- 
1.7.10.4



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] dom0 kenrel crashes for openstack + libvirt + libxl

[Ian Campbell]

On Fri, 2014-11-21 at 14:18 +, David Vrabel wrote:
 On 21/11/14 14:05, Ian Campbell wrote:
  On Fri, 2014-11-21 at 09:28 +, Ian Campbell wrote:
  I think libvirt is wrong to specify an absolute path here, IMHO by
  default it should just specify pygrub and let libxl figure out the
  correct path. Jim, what do you think?
  
  e.g. something like the following untested (but pretty obvious[0])
  patch.
 [...]
  +# define pygrub
 
 I'm going to go with pretty obviously broken.  You're missing the macro
 name.

Doh! Indeed...

Lets try that again...

8--

From 9f2d8da8264b426f54b92378e9e00973694193d4 Mon Sep 17 00:00:00 2001
From: Ian Campbell ian.campb...@citrix.com
Date: Fri, 21 Nov 2014 14:00:38 +
Subject: [PATCH] libxl: Allow libxl to find pygrub binary.

Specifying an explicit path to pygrub (e.g. BINDIR /pygrub) only works if
Xen and libvirt happen to be installed to the same prefix. A more flexible
approach is to simply specify pygrub which will cause libxl to use the
correct path which it knows (since it is built with the same prefix as pygrub).

This is particular problematic in the Debian packaging, since the Debian Xen
package relocates pygrub into a libexec dir, however I think this change makes
sense upstream.

Signed-off-by: Ian Campbell ian.campb...@citrix.com
---
 .gnulib|2 +-
 src/libxl/libxl_conf.h |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/.gnulib b/.gnulib
index 9565c3b..2d28074 16
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit 9565c3be73eb6d76b7b42a21d68d2e00a62abb6d
+Subproject commit 2d280742a9e30088aa169f53353765d5daafe4c0
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 25f77ea..d7a3971 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -53,7 +53,7 @@
 # define LIBXL_LIB_DIR LOCALSTATEDIR /lib/libvirt/libxl
 # define LIBXL_SAVE_DIR LIBXL_LIB_DIR /save
 # define LIBXL_DUMP_DIR LIBXL_LIB_DIR /dump
-# define LIBXL_BOOTLOADER_PATH BINDIR /pygrub
+# define LIBXL_BOOTLOADER_PATH pygrub
 
 /* libxl interface for setting VCPU affinity changed in 4.5. In fact, a new
  * parameter has been added, representative of 'VCPU soft affinity'. If one
-- 
1.7.10.4



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] dom0 kenrel crashes for openstack + libvirt + libxl

[Ian Campbell]

On Fri, 2014-11-21 at 14:32 +, Ian Campbell wrote:

I've now built and tested this with the 1.2.9 Debian package, works like
a charm...

 From 9f2d8da8264b426f54b92378e9e00973694193d4 Mon Sep 17 00:00:00 2001
 From: Ian Campbell ian.campb...@citrix.com
 Date: Fri, 21 Nov 2014 14:00:38 +
 Subject: [PATCH] libxl: Allow libxl to find pygrub binary.
 
 Specifying an explicit path to pygrub (e.g. BINDIR /pygrub) only works if
 Xen and libvirt happen to be installed to the same prefix. A more flexible
 approach is to simply specify pygrub which will cause libxl to use the
 correct path which it knows (since it is built with the same prefix as 
 pygrub).
 
 This is particular problematic in the Debian packaging, since the Debian Xen
 package relocates pygrub into a libexec dir, however I think this change makes
 sense upstream.
 
 Signed-off-by: Ian Campbell ian.campb...@citrix.com
 ---
  .gnulib|2 +-
  src/libxl/libxl_conf.h |2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/.gnulib b/.gnulib
 index 9565c3b..2d28074 16
 --- a/.gnulib
 +++ b/.gnulib
 @@ -1 +1 @@
 -Subproject commit 9565c3be73eb6d76b7b42a21d68d2e00a62abb6d
 +Subproject commit 2d280742a9e30088aa169f53353765d5daafe4c0
 diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
 index 25f77ea..d7a3971 100644
 --- a/src/libxl/libxl_conf.h
 +++ b/src/libxl/libxl_conf.h
 @@ -53,7 +53,7 @@
  # define LIBXL_LIB_DIR LOCALSTATEDIR /lib/libvirt/libxl
  # define LIBXL_SAVE_DIR LIBXL_LIB_DIR /save
  # define LIBXL_DUMP_DIR LIBXL_LIB_DIR /dump
 -# define LIBXL_BOOTLOADER_PATH BINDIR /pygrub
 +# define LIBXL_BOOTLOADER_PATH pygrub
  
  /* libxl interface for setting VCPU affinity changed in 4.5. In fact, a new
   * parameter has been added, representative of 'VCPU soft affinity'. If one


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] libvirt build failure on armhf (Re: [Xen-devel] [libvirt bisection] complete build-armhf-libvirt)

[Ian Campbell]

On Tue, 2014-10-21 at 18:02 -0600, Jim Fehlig wrote:
 Ian Campbell wrote:
  Hi,
 
  On Fri, 2014-10-17 at 00:42 +0100, xen.org wrote:

  *** Found and reproduced problem changeset ***
 
Bug is in tree:  libvirt git://libvirt.org/libvirt.git
Bug introduced:  24c160376275b7d31f71fbde83af8183cbf744a7
Bug not present: 69f7b67d55316ab7b28fb904b346943497b856a1
  
 
  The Xen automated build tests have discovered a built error on armhf. I
  don't think it is Xen specific. Our bisector has fingered the changeset
  below. I've had a skim of the git log from there to master and of the
  outstanding patches on libvir list and I don't see anything which is
  obviously a related fix, so I hope this is still useful/relevant.
 
  An instance of the failure can be seen at
  http://www.chiark.greenend.org.uk/~xensrcts/logs/30765/build-armhf-libvirt/info.html
 
  The logs say:
  util/virsocketaddr.c: In function 'virSocketAddrIsNumericLocalhost':
  util/virsocketaddr.c:904:17: error: cast increases required alignment of 
  target type [-Werror=cast-align]
  util/virsocketaddr.c:909:17: error: cast increases required alignment of 
  target type [-Werror=cast-align]

 
 Hi Ian,
 
 While catching up on libvirt mail today, I not only saw your report, but
 also noticed a fix from Roman
 
 https://www.redhat.com/archives/libvir-list/2014-October/msg00559.html

Great, glad to see it's being worked on.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] libvirt build failure on armhf (Re: [Xen-devel] [libvirt bisection] complete build-armhf-libvirt)

[Ian Campbell]

Hi,

On Fri, 2014-10-17 at 00:42 +0100, xen.org wrote:
 *** Found and reproduced problem changeset ***
 
   Bug is in tree:  libvirt git://libvirt.org/libvirt.git
   Bug introduced:  24c160376275b7d31f71fbde83af8183cbf744a7
   Bug not present: 69f7b67d55316ab7b28fb904b346943497b856a1

The Xen automated build tests have discovered a built error on armhf. I
don't think it is Xen specific. Our bisector has fingered the changeset
below. I've had a skim of the git log from there to master and of the
outstanding patches on libvir list and I don't see anything which is
obviously a related fix, so I hope this is still useful/relevant.

An instance of the failure can be seen at
http://www.chiark.greenend.org.uk/~xensrcts/logs/30765/build-armhf-libvirt/info.html

The logs say:
util/virsocketaddr.c: In function 'virSocketAddrIsNumericLocalhost':
util/virsocketaddr.c:904:17: error: cast increases required alignment of target 
type [-Werror=cast-align]
util/virsocketaddr.c:909:17: error: cast increases required alignment of target 
type [-Werror=cast-align]

Ian.

   commit 24c160376275b7d31f71fbde83af8183cbf744a7
   Author: Chen Fan chen.fan.f...@cn.fujitsu.com
   Date:   Tue Oct 7 12:07:31 2014 +0800
   
   conf: add check if migration_host is a localhost address
   
Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
   
   Signed-off-by: Ján Tomko jto...@redhat.com
 
 
 For bisection revision-tuple graph see:

 http://www.chiark.greenend.org.uk/~xensrcts/results/bisect.libvirt.build-armhf-libvirt.libvirt-build.html
 Revision IDs in each graph node refer, respectively, to the Trees above.
 
 
 Searching for failure / basis pass:
  30765 fail [host=marilith-n4] / 30748 ok.
 Failure / basis pass flights: 30765 / 30748
 Tree: gnulib_libvirt 
 git://drall.uk.xensource.com:9419/git://git.sv.gnu.org/gnulib.git%20[fetch=try]
 Tree: libvirt git://libvirt.org/libvirt.git
 Tree: qemuu git://xenbits.xen.org/staging/qemu-upstream-unstable.git
 Tree: xen git://xenbits.xen.org/xen.git
 Latest b155b0649814b20e635a2db305696710fa1037ce 
 e9a1c4384c4f83a8e0d5d98c80369ecfd5b3f2e0 
 c9d8f8b755e8960edf7725e05f3e6ac743a5e12e 
 4d57153b52a36183d58e8de6ba613929f906386a
 Basis pass b155b0649814b20e635a2db305696710fa1037ce 
 4d1852c48541a29e3c47caf0f2b801dfcb6579db 
 c9d8f8b755e8960edf7725e05f3e6ac743a5e12e 
 7d96cc5c4b2670a4220a50746fa17a0e8a4da1c2
 Generating revisions with ./adhoc-revtuple-generator  
 git://drall.uk.xensource.com:9419/git://git.sv.gnu.org/gnulib.git%20[fetch=try]#b155b0649814b20e635a2db305696710fa1037ce-b155b0649814b20e635a2db305696710fa1037ce
  
 git://libvirt.org/libvirt.git#4d1852c48541a29e3c47caf0f2b801dfcb6579db-e9a1c4384c4f83a8e0d5d98c80369ecfd5b3f2e0
  
 git://xenbits.xen.org/staging/qemu-upstream-unstable.git#c9d8f8b755e8960edf7725e05f3e6ac743a5e12e-c9d8f8b755e8960edf7725e05f3e6ac743a5e12e
  
 git://xenbits.xen.org/xen.git#7d96cc5c4b2670a4220a50746fa17a0e8a4da1c2-4d57153b52a36183d58e8de6ba613929f906386a
 + exec
 + sh -xe
 + cd /export/home/osstest/repos/libvirt
 + git remote set-url origin 
 git://drall.uk.xensource.com:9419/git://libvirt.org/libvirt.git
 + git fetch -p origin +refs/heads/*:refs/remotes/origin/*
 + exec
 + sh -xe
 + cd /export/home/osstest/repos/xen
 + git remote set-url origin 
 git://drall.uk.xensource.com:9419/git://xenbits.xen.org/xen.git
 + git fetch -p origin +refs/heads/*:refs/remotes/origin/*
 + exec
 + sh -xe
 + cd /export/home/osstest/repos/libvirt
 + git remote set-url origin 
 git://drall.uk.xensource.com:9419/git://libvirt.org/libvirt.git
 + git fetch -p origin +refs/heads/*:refs/remotes/origin/*
 + exec
 + sh -xe
 + cd /export/home/osstest/repos/xen
 + git remote set-url origin 
 git://drall.uk.xensource.com:9419/git://xenbits.xen.org/xen.git
 + git fetch -p origin +refs/heads/*:refs/remotes/origin/*
 Loaded 3959 nodes in revision graph
 Searching for test results:
  30748 pass b155b0649814b20e635a2db305696710fa1037ce 
 4d1852c48541a29e3c47caf0f2b801dfcb6579db 
 c9d8f8b755e8960edf7725e05f3e6ac743a5e12e 
 7d96cc5c4b2670a4220a50746fa17a0e8a4da1c2
  30765 fail b155b0649814b20e635a2db305696710fa1037ce 
 e9a1c4384c4f83a8e0d5d98c80369ecfd5b3f2e0 
 c9d8f8b755e8960edf7725e05f3e6ac743a5e12e 
 4d57153b52a36183d58e8de6ba613929f906386a
  30776 pass b155b0649814b20e635a2db305696710fa1037ce 
 4d1852c48541a29e3c47caf0f2b801dfcb6579db 
 c9d8f8b755e8960edf7725e05f3e6ac743a5e12e 
 7d96cc5c4b2670a4220a50746fa17a0e8a4da1c2
  30777 fail b155b0649814b20e635a2db305696710fa1037ce 
 e9a1c4384c4f83a8e0d5d98c80369ecfd5b3f2e0 
 c9d8f8b755e8960edf7725e05f3e6ac743a5e12e 
 4d57153b52a36183d58e8de6ba613929f906386a
  30779 fail b155b0649814b20e635a2db305696710fa1037ce 
 3d6c07f7f804be936df800eed749d01873da5a45 
 c9d8f8b755e8960edf7725e05f3e6ac743a5e12e 
 4d57153b52a36183d58e8de6ba613929f906386a
  30780 fail b155b0649814b20e635a2db305696710fa1037ce 
 24c160376275b7d31f71fbde83af8183cbf744a7 
 c9d8f8b755e8960edf7725e05f3e6ac743a5e12e 
 

Re: [libvirt] [PATCH] virprocess: Introduce our own setns() wrapper

[Ian Campbell]

On Mon, 2014-09-15 at 10:21 +0100, Daniel P. Berrange wrote:
 On Sun, Sep 14, 2014 at 06:35:19PM +0100, Ian Campbell wrote:
  On Wed, 2014-09-10 at 12:20 +0200, Michal Privoznik wrote:

   +/*
   + * Workaround older glibc. While kernel may support the setns
   + * syscall, the glibc wrapper might not exist. If that's the
   + * case, use our own.
   + */
   +#ifndef __NR_setns
   +# if defined(__x86_64__)
   +#  define __NR_setns 308
   +# elif defined(__i386__)
   +#  define __NR_setns 346
   +# else
   +#  error __NR_setns is not defined
   +# endif
   +#endif
  
  Xen's automated build tests of libvirt have just failed with this when
  building for armhf:
  util/virprocess.c:75:4: error: #error __NR_setns is not
  defined
  
  is this another aspect of the out of date glibc issue?
 
 IIUC, the syscalls numbers come from the kernel headers, while the syscall
 API comes from glibc.

Makes sense.

  Either way, we should not have left this as a fatal
 #error. I'll dig up the required constants for all other architectures we
 care about.

Thanks!

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virprocess: Introduce our own setns() wrapper

[Ian Campbell]

On Wed, 2014-09-10 at 12:20 +0200, Michal Privoznik wrote:
  
 +/*
 + * Workaround older glibc. While kernel may support the setns
 + * syscall, the glibc wrapper might not exist. If that's the
 + * case, use our own.
 + */
 +#ifndef __NR_setns
 +# if defined(__x86_64__)
 +#  define __NR_setns 308
 +# elif defined(__i386__)
 +#  define __NR_setns 346
 +# else
 +#  error __NR_setns is not defined
 +# endif
 +#endif

Xen's automated build tests of libvirt have just failed with this when
building for armhf:
util/virprocess.c:75:4: error: #error __NR_setns is not
defined

is this another aspect of the out of date glibc issue?

(build log at
http://www.chiark.greenend.org.uk/~xensrcts/logs/30238/build-armhf-libvirt/5.ts-libvirt-build.log
 and full test run logs linked from 
http://lists.xen.org/archives/html/xen-devel/2014-09/msg02146.html  

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [libvirt bisection] complete build-armhf-libvirt

[Ian Campbell]

Xen's automated testing of libvirt against newer Xen's has found a build
issue which it has bisected down to blockcopy: expose new API in
virsh.

An instance of the failure can be found in flight 30154:
http://lists.xen.org/archives/html/xen-devel/2014-09/msg01063.html
links to the logs =
http://www.chiark.greenend.org.uk/~xensrcts/logs/30154/
click the header of a failing column =
http://www.chiark.greenend.org.uk/~xensrcts/logs/30154/build-armhf-libvirt/info.html
 
click the failing step =
http://www.chiark.greenend.org.uk/~xensrcts/logs/30154/build-armhf-libvirt/5.ts-libvirt-build.log

virsh-domain.c: In function 'cmdBlockCopy':
virsh-domain.c:2003:17: error: comparison is always false due to 
limited range of data type [-Werror=type-limits]
cc1: all warnings being treated as errors

It seems to be failing similarly on i386 and I suppose most 32-bit
arches.

Cheers,
Ian.

On Mon, 2014-09-08 at 19:30 +0100, xen.org wrote:
 branch xen-unstable
 xen branch xen-unstable
 job build-armhf-libvirt
 test libvirt-build
 
 Tree: gnulib_libvirt 
 git://drall.uk.xensource.com:9419/git://git.sv.gnu.org/gnulib.git%20[fetch=try]
 Tree: libvirt git://libvirt.org/libvirt.git
 Tree: qemuu git://xenbits.xen.org/staging/qemu-upstream-unstable.git
 Tree: xen git://xenbits.xen.org/xen.git
 
 *** Found and reproduced problem changeset ***
 
   Bug is in tree:  libvirt git://libvirt.org/libvirt.git
   Bug introduced:  c1d75deea228e7f4a7462aef143e18c456b2a82c
   Bug not present: 0e8bed817705664764db408c93ba54277ef85157
 
 
   commit c1d75deea228e7f4a7462aef143e18c456b2a82c
   Author: Eric Blake ebl...@redhat.com
   Date:   Fri Aug 29 15:47:28 2014 -0600
   
   blockcopy: expose new API in virsh
   
   Expose the new power of virDomainBlockCopy through virsh (well,
   all but the finer-grained bandwidth, as that is its own can of
   worms for a later patch).  Continue to use the older API where
   possible, for maximum compatibility.
   
   The command now requires either --dest (with optional --format
   and --blockdev), to directly describe the file destination, or
   --xml, to name a file that contains an XML description such as:
   
   disk type='network'
 driver type='raw'/
 source protocol='gluster' name='vol1/img'
   host name='red'/
 /source
   /disk
   
   [well, it may be a while before the qemu driver is actually patched
   to act on that particular xml beyond just parsing it, but the virsh
   interface won't need changing at that time]
   
   Non-zero option parameters are converted into virTypedParameters,
   and if anything requires the new API, the command can synthesize
   appropriate XML even if the --dest option was used instead of --xml.
   
   The existing --raw flag remains for back-compat, but the preferred
   spelling is now --format=raw, since the new API now allows us
   to specify all formats rather than just a boolean raw to suppress
   probing.
   
   I hope I did justice in describing the effects of granularity and
   buf-size on how they get passed through to qemu.
   
   * tools/virsh-domain.c (cmdBlockCopy): Add new options --xml,
   --granularity, --buf-size, --format. Make --raw an alias for
   --format=raw. Call new API if new parameters are in use.
   * tools/virsh.pod (blockcopy): Document new options.
   
   Signed-off-by: Eric Blake ebl...@redhat.com
 
 
 For bisection revision-tuple graph see:

 http://www.chiark.greenend.org.uk/~xensrcts/results/bisect.libvirt.build-armhf-libvirt.libvirt-build.html
 Revision IDs in each graph node refer, respectively, to the Trees above.
 
 
 Searching for failure / basis pass:
  30154 fail [host=marilith-n4] / 30147 ok.
 Failure / basis pass flights: 30154 / 30147
 Tree: gnulib_libvirt 
 git://drall.uk.xensource.com:9419/git://git.sv.gnu.org/gnulib.git%20[fetch=try]
 Tree: libvirt git://libvirt.org/libvirt.git
 Tree: qemuu git://xenbits.xen.org/staging/qemu-upstream-unstable.git
 Tree: xen git://xenbits.xen.org/xen.git
 Latest 2bf7326e10fae4abef536486aa9819331596c379 
 a48362cdfeb5c948218a2e4bf7cc9354082fc1b6 
 ea772ca487e219e5d5b82d50da460c4145238038 
 93d5f192d4903953baa8c2115534d23140236176
 Basis pass 9565c3be73eb6d76b7b42a21d68d2e00a62abb6d 
 0eaad0a39c67bdddc56d608ff28fcb490c12b8b3 
 ea772ca487e219e5d5b82d50da460c4145238038 
 c33fe5459732fc85c2c279c5e8ed316b8601c58c
 Generating revisions with ./adhoc-revtuple-generator  
 git://drall.uk.xensource.com:9419/git://git.sv.gnu.org/gnulib.git%20[fetch=try]#9565c3be73eb6d76b7b42a21d68d2e00a62abb6d-2bf7326e10fae4abef536486aa9819331596c379
  
 git://libvirt.org/libvirt.git#0eaad0a39c67bdddc56d608ff28fcb490c12b8b3-a48362cdfeb5c948218a2e4bf7cc9354082fc1b6
  
 

Re: [libvirt] [Xen-devel] [libvirt bisection] complete build-armhf-libvirt

[Ian Campbell]

On Tue, 2014-09-09 at 09:54 +0100, Daniel P. Berrange wrote:
 On Tue, Sep 09, 2014 at 09:49:47AM +0100, Ian Campbell wrote:
  Xen's automated testing of libvirt against newer Xen's has found a build
  issue which it has bisected down to blockcopy: expose new API in
  virsh.
  
  An instance of the failure can be found in flight 30154:
  http://lists.xen.org/archives/html/xen-devel/2014-09/msg01063.html
  links to the logs =
  http://www.chiark.greenend.org.uk/~xensrcts/logs/30154/
  click the header of a failing column =
  http://www.chiark.greenend.org.uk/~xensrcts/logs/30154/build-armhf-libvirt/info.html
   
  click the failing step =
  http://www.chiark.greenend.org.uk/~xensrcts/logs/30154/build-armhf-libvirt/5.ts-libvirt-build.log
  
  virsh-domain.c: In function 'cmdBlockCopy':
  virsh-domain.c:2003:17: error: comparison is always false due to 
  limited range of data type [-Werror=type-limits]
  cc1: all warnings being treated as errors
  
  It seems to be failing similarly on i386 and I suppose most 32-bit
  arches.
 
 Thanks, we've just had a fix for that pushed

Thanks, I grepped libvir-list but must have missed it.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: support domainReset

[Ian Campbell]

On Tue, 2014-08-05 at 20:30 +0100, Wei Liu wrote:
 On Tue, Aug 05, 2014 at 07:45:48PM +0100, Ian Campbell wrote:
 [...]
 Sure.  I think having an API that emulates a power reset button would 
 be
 a nice addition to libxl's domain operations.  The destroy/start
 approach incurs a small bit of overhead, which would be avoided with
 such an API.  Clients (perhaps incorrectly) implementing their own
 notion of reset  would also be avoided.

I think this ought to become pretty easy once Wei's patches to record
the guest cfg in libxl are completed. Wei -- what do you think?

   
   I don't think this reset API will need to record any state, i.e. this
   feature looks unrelated to my work. What do I miss?
  
  It's a forced reboot, so the API would need to destroy and then recreate
  the domain. Recreate would need to use the state your patches arrange
  for libxl to store.
  
 
 Oh you were talking about pesisting state across hard reset, that's of
 course achievable. I think hard reset is more or less the same as
 reboot.
 
 That's still somewhat orthogonal to my work though. Not having the
 capability to presist state across in libxl doesn't prevent us from
 introducing reset. I think this is the status quo of reboot API,
 isn't it?

There is no reboot API in the sense we are talking about.
libxl_domain_reboot() asks the guest to reboot itself. The resulting
actual reboot is handled by the toolstack receiving
LIBXL_EVENT_TYPE_DOMAIN_DEATH and using libxl_domain_destroy
+libxl_domain_create to recreate the domain, prior to your changes only
the toolstack app could do this because only xl/libvirt knew the actual
domain cfg. With your changes a new libxl_domain_hard_reboot could, I
think, be written which does the reboot.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: support domainReset

[Ian Campbell]

On Mon, 2014-08-04 at 20:09 -0600, Jim Fehlig wrote:
 Currently, libxl_send_trigger() does not implement the LIBXL_TRIGGER_RESET
 option,

There's a case in the switch within libxl_send_trigger which at least
purports to do so:

int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid,
   libxl_trigger trigger, uint32_t vcpuid)
{
[...]
case LIBXL_TRIGGER_RESET:
rc = xc_domain_send_trigger(ctx-xch, domid,
XEN_DOMCTL_SENDTRIGGER_RESET, vcpu
break;

Do you mean to say that it is broken or perhaps that it doesn't match
the required semantics of domainReset?

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: support domainReset

[Ian Campbell]

On Tue, 2014-08-05 at 10:41 +0200, Olaf Hering wrote:
 On Tue, Aug 05, Ian Campbell wrote:
 
  case LIBXL_TRIGGER_RESET:
  rc = xc_domain_send_trigger(ctx-xch, domid,
  XEN_DOMCTL_SENDTRIGGER_RESET, vcpu
  break;
  
  Do you mean to say that it is broken or perhaps that it doesn't match
  the required semantics of domainReset?
 
 This op is not implemented.

You mean in libxc/Xen?

  And would it work for PV and HVM anyway?

Aren't these triggers HVM only? They turn into ACPI events and things,
don't they?

I'm not sure what the intended behaviour of libvirt domainReset is, but
perhaps libxl_domain_{reboot,shutdown} is closer to what is needed?

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: support domainReset

[Ian Campbell]

On Tue, 2014-08-05 at 10:55 +0200, Olaf Hering wrote:
 On Tue, Aug 05, Ian Campbell wrote:
 
  On Tue, 2014-08-05 at 10:41 +0200, Olaf Hering wrote:
   On Tue, Aug 05, Ian Campbell wrote:
   
case LIBXL_TRIGGER_RESET:
rc = xc_domain_send_trigger(ctx-xch, domid,
XEN_DOMCTL_SENDTRIGGER_RESET, vcpu
break;

Do you mean to say that it is broken or perhaps that it doesn't match
the required semantics of domainReset?
   
   This op is not implemented.
  
  You mean in libxc/Xen?
 
 In Xen itself.
 
And would it work for PV and HVM anyway?
  
  Aren't these triggers HVM only? They turn into ACPI events and things,
  don't they?
 
 Most likely yes.
 
  I'm not sure what the intended behaviour of libvirt domainReset is, but
  perhaps libxl_domain_{reboot,shutdown} is closer to what is needed?
 
 The original report was that 'Reset' does not work from GUI, like
 virt-manager or virsh. I think the expected outcome is like pushing the
 reset button on a physical board. Xen doesnt do it that way, no idea
 about others.

Sounds like you want libxl_domain_reboot then, perhaps with a fallback
on ERROR_NOPARAVIRT for an HVM guest to sending a trigger.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: support domainReset

[Ian Campbell]

On Tue, 2014-08-05 at 08:06 -0600, Jim Fehlig wrote:
 Ian Campbell wrote:
  On Tue, 2014-08-05 at 10:55 +0200, Olaf Hering wrote:

  The original report was that 'Reset' does not work from GUI, like
  virt-manager or virsh. I think the expected outcome is like pushing the
  reset button on a physical board. Xen doesnt do it that way, no idea
  about others.
  
 
  Sounds like you want libxl_domain_reboot then, perhaps with a fallback
  on ERROR_NOPARAVIRT for an HVM guest to sending a trigger.

 
 Hrm, I don't think that's right .  It should be a hard reset
 
 http://libvirt.org/html/libvirt-libvirt.html#virDomainReset
 
 destroy/start seems the correct way to implement this.

Yes, given that requirement it is. Sorry for the noise.

Would some sort of hard reset API be useful in libxl?

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: support domainReset

[Ian Campbell]

On Tue, 2014-08-05 at 09:10 -0600, Jim Fehlig wrote:
 Ian Campbell wrote:
  On Tue, 2014-08-05 at 08:06 -0600, Jim Fehlig wrote:

  Ian Campbell wrote:
  
  On Tue, 2014-08-05 at 10:55 +0200, Olaf Hering wrote:


  The original report was that 'Reset' does not work from GUI, like
  virt-manager or virsh. I think the expected outcome is like pushing the
  reset button on a physical board. Xen doesnt do it that way, no idea
  about others.
  
  
  Sounds like you want libxl_domain_reboot then, perhaps with a fallback
  on ERROR_NOPARAVIRT for an HVM guest to sending a trigger.


  Hrm, I don't think that's right .  It should be a hard reset
 
  http://libvirt.org/html/libvirt-libvirt.html#virDomainReset
 
  destroy/start seems the correct way to implement this.
  
 
  Yes, given that requirement it is. Sorry for the noise.
 
  Would some sort of hard reset API be useful in libxl?

 
 Sure.  I think having an API that emulates a power reset button would be
 a nice addition to libxl's domain operations.  The destroy/start
 approach incurs a small bit of overhead, which would be avoided with
 such an API.  Clients (perhaps incorrectly) implementing their own
 notion of reset  would also be avoided.

I think this ought to become pretty easy once Wei's patches to record
the guest cfg in libxl are completed. Wei -- what do you think?

 In the absence of libxl_domain_reset(), do folks think the destroy/start
 approach is acceptable?

FWIW I believe it is.

   As Olaf mentioned, it allows Force Reset to
 work via virt-manager.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: support domainReset

[Ian Campbell]

On Tue, 2014-08-05 at 17:12 +0100, Wei Liu wrote:
 On Tue, Aug 05, 2014 at 04:30:47PM +0100, Ian Campbell wrote:
  On Tue, 2014-08-05 at 09:10 -0600, Jim Fehlig wrote:
   Ian Campbell wrote:
On Tue, 2014-08-05 at 08:06 -0600, Jim Fehlig wrote:
  
Ian Campbell wrote:

On Tue, 2014-08-05 at 10:55 +0200, Olaf Hering wrote:
  
  
The original report was that 'Reset' does not work from GUI, like
virt-manager or virsh. I think the expected outcome is like pushing 
the
reset button on a physical board. Xen doesnt do it that way, no idea
about others.


Sounds like you want libxl_domain_reboot then, perhaps with a fallback
on ERROR_NOPARAVIRT for an HVM guest to sending a trigger.
  
  
Hrm, I don't think that's right .  It should be a hard reset
   
http://libvirt.org/html/libvirt-libvirt.html#virDomainReset
   
destroy/start seems the correct way to implement this.

   
Yes, given that requirement it is. Sorry for the noise.
   
Would some sort of hard reset API be useful in libxl?
  
   
   Sure.  I think having an API that emulates a power reset button would be
   a nice addition to libxl's domain operations.  The destroy/start
   approach incurs a small bit of overhead, which would be avoided with
   such an API.  Clients (perhaps incorrectly) implementing their own
   notion of reset  would also be avoided.
  
  I think this ought to become pretty easy once Wei's patches to record
  the guest cfg in libxl are completed. Wei -- what do you think?
  
 
 I don't think this reset API will need to record any state, i.e. this
 feature looks unrelated to my work. What do I miss?

It's a forced reboot, so the API would need to destroy and then recreate
the domain. Recreate would need to use the state your patches arrange
for libxl to store.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] libxl: don't break the build on Xen=4.5 because of libxl_vcpu_setaffinity()

[Ian Campbell]

On Tue, 2014-07-01 at 11:00 -0600, Jim Fehlig wrote:
 Dario Faggioli wrote:
  Just thinking out loud here but, if we did not get the push failure,
  when would have we discovered this?

 
 Good question.  I could have been days (or even weeks) before I stumbled
 across it.  ATM, I don't have any automated builds of libvirt.git master
 against xen.git master :-/.  Adding it to my list...

FWIW I think this effectively already exists in the osstest flights.

[xen-unstable test] is testing new Xen against gated libvirt
[libvirt test] is testing new libvirt against gated Xen-unstable

(where gated X means the last successfully tested version of X from
the other flight)

What's not tested is newest Xen against newest libvirt, but I think any
issue should show up in one or the other of the existing flights.

 Thanks for your work on the push gate Ian!

No problem, glad it's actually flagging useful stuff already!

Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] libxl: don't break the build on Xen=4.5 because of libxl_vcpu_setaffinity()

[Ian Campbell]

On Tue, 2014-07-01 at 08:52 +0200, Dario Faggioli wrote:
 On lun, 2014-06-30 at 15:32 -0600, Jim Fehlig wrote:
  Eric Blake wrote:
 
   Thanks, looks good.  I was about to push, but wanted to check with other
   libvirt devs first since we are in 1.2.6 freeze.  Would it be fine to
   push this?  It fixes a libxl driver build failure against xen-unstable.
   
  
   Yes, fixing a build failure is an acceptable fix during hard freeze.  Go
   ahead and push.
 
  
  Thanks Eric, pushed now.
  
 Cool! Thanks Everyone,

And thanks Dario for the quick fix.

WRT Xen's automated testing the new libvirt should get tested today and
the result be picked up for a xen-unstable run shortly after, so staging
ought to be unblocked by ~tomorrow, barring any other failures.

I've added does libvirt need a peemptive update? to my mental
checklist for things involved LIBXL_API_VERSION changes.

Cheers,
Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH V2] libxl: detect support for save and restore

[Ian Campbell]

On Wed, 2014-06-25 at 18:09 -0600, Jim Fehlig wrote:
 libxl does not support save, restore, or migrate on all architectures,
 notably ARM.  Detect whether libxl supports these operations using
 LIBXL_HAVE_NO_SUSPEND_RESUME.  If not supported, drop advertisement of
 migration_features.
 
 Found by Ian Campbell while improving Xen's OSSTEST infrastructure
 
 http://lists.xen.org/archives/html/xen-devel/2014-06/msg02171.html
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 ---
 
 Another option for
 
 https://www.redhat.com/archives/libvir-list/2014-June/msg01276.html
 
 With this one, we even avoid the distasteful double negative :).
 
 Compile-tested on x86 only at this point.  The ARM build is still
 slowly grinding away...

Build and runtime tested on ARM and x86 here, works fine. Thanks!

Ian.

 
  src/libxl/libxl_conf.c   |  4 
  src/libxl/libxl_driver.c | 35 +++
  2 files changed, 39 insertions(+)
 
 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
 index 4b6b5c0..8eeaf82 100644
 --- a/src/libxl/libxl_conf.c
 +++ b/src/libxl/libxl_conf.c
 @@ -1340,7 +1340,11 @@ libxlMakeCapabilities(libxl_ctx *ctx)
  {
  virCapsPtr caps;
  
 +#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 +if ((caps = virCapabilitiesNew(virArchFromHost(), 0, 0)) == NULL)
 +#else
  if ((caps = virCapabilitiesNew(virArchFromHost(), 1, 1)) == NULL)
 +#endif
  return NULL;
  
  if (libxlCapsInitHost(ctx, caps)  0)
 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index 1ea99e2..646c9b9 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -1379,6 +1379,11 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, 
 const char *dxml,
  int ret = -1;
  bool remove_dom = false;
  
 +#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 +virReportUnsupportedError();
 +return -1;
 +#endif
 +
  virCheckFlags(0, -1);
  if (dxml) {
  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
 @@ -1440,6 +1445,11 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char 
 *from,
  int fd = -1;
  int ret = -1;
  
 +#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 +virReportUnsupportedError();
 +return -1;
 +#endif
 +
  virCheckFlags(VIR_DOMAIN_SAVE_PAUSED, -1);
  if (dxml) {
  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
 @@ -4351,6 +4361,11 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
  const char *xmlin = NULL;
  virDomainObjPtr vm = NULL;
  
 +#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 +virReportUnsupportedError();
 +return NULL;
 +#endif
 +
  virCheckFlags(LIBXL_MIGRATION_FLAGS, NULL);
  if (virTypedParamsValidate(params, nparams, LIBXL_MIGRATION_PARAMETERS) 
  0)
  return NULL;
 @@ -4395,6 +4410,11 @@ libxlDomainMigratePrepare3Params(virConnectPtr dconn,
  const char *dname = NULL;
  const char *uri_in = NULL;
  
 +#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 +virReportUnsupportedError();
 +return -1;
 +#endif
 +
  virCheckFlags(LIBXL_MIGRATION_FLAGS, -1);
  if (virTypedParamsValidate(params, nparams, LIBXL_MIGRATION_PARAMETERS) 
  0)
  goto error;
 @@ -4445,6 +4465,11 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
  const char *uri = NULL;
  int ret = -1;
  
 +#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 +virReportUnsupportedError();
 +return -1;
 +#endif
 +
  virCheckFlags(LIBXL_MIGRATION_FLAGS, -1);
  if (virTypedParamsValidate(params, nparams, LIBXL_MIGRATION_PARAMETERS) 
  0)
  goto cleanup;
 @@ -4497,6 +4522,11 @@ libxlDomainMigrateFinish3Params(virConnectPtr dconn,
  virDomainObjPtr vm = NULL;
  const char *dname = NULL;
  
 +#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 +virReportUnsupportedError();
 +return NULL;
 +#endif
 +
  virCheckFlags(LIBXL_MIGRATION_FLAGS, NULL);
  if (virTypedParamsValidate(params, nparams, LIBXL_MIGRATION_PARAMETERS) 
  0)
  return NULL;
 @@ -4545,6 +4575,11 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
  libxlDriverPrivatePtr driver = domain-conn-privateData;
  virDomainObjPtr vm = NULL;
  
 +#ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
 +virReportUnsupportedError();
 +return -1;
 +#endif
 +
  virCheckFlags(LIBXL_MIGRATION_FLAGS, -1);
  if (virTypedParamsValidate(params, nparams, LIBXL_MIGRATION_PARAMETERS) 
  0)
  return -1;


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: prefer qdisk for driver name='file'

[Ian Campbell]

On Tue, 2014-06-24 at 16:24 -0600, Jim Fehlig wrote:
 Ian Campbell wrote:
  On Fri, 2014-06-20 at 15:07 -0600, Jim Fehlig wrote:

  The libxl driver currently sets the disk backend to
  LIBXL_DISK_BACKEND_TAP when driver name='file' is specified
  in the disk config.  qdisk should be prefered with this
  configuration, otherwise existing configuration such as the
  following, which worked with the old Xen driver, will not work
  with the libxl driver
  
 
  OOI why not let libxl pick the most appropriate backend for (most of)
  these alternatives?

 
 I think the libvirt approach is to be a bit more explicit, instead of
 relying on hypervisor defaults that might change over time.  I'm quite
 sure this is the approach danpb has always advocated.

I thought that policy was more for guest visible things whereas the disk
backend is transparent to the guest.

But either way it's up to libvirt.

Thanks,
Ian.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH RFC OSSTEST 7/9] Toolstack: Abstract away migration support check.

[Ian Campbell]

On Wed, 2014-06-25 at 09:25 -0600, Jim Fehlig wrote:
 Ian Campbell wrote:
  On Tue, 2014-06-24 at 21:18 -0600, Jim Fehlig wrote:
 

  Something like the attached patch (compile-tested only).  You'll need an
  updated libvirt.git master to apply cleanly.
  
 
  Thanks. Building on ARM I get:
 
  libxl/libxl_driver.c:4346:1: error: 'libxlDomainMigrateBegin3Params' 
  defined but not used [-Werror=unused-function]
  libxl/libxl_driver.c:4384:1: error: 'libxlDomainMigratePrepare3Params' 
  defined but not used [-Werror=unused-function]
  libxl/libxl_driver.c:4433:1: error: 'libxlDomainMigratePerform3Params' 
  defined but not used [-Werror=unused-function]
  libxl/libxl_driver.c:4488:1: error: 'libxlDomainMigrateFinish3Params' 
  defined but not used [-Werror=unused-function]
  libxl/libxl_driver.c:4539:1: error: 'libxlDomainMigrateConfirm3Params' 
  defined but not used [-Werror=unused-function]

 
 Yeah, compile-tested on x86 only as it turned out.  I was building
 packages in the build service, where I had the libxl driver disabled for
 aarch64 :-/.

Whoops!

 With the fixup, does this work for you?  Is migration_features omitted
 from the capabilities?

Yes, it is omitted. On ARM:

# virsh capabilities
capabilities

  host
cpu
  archarmv7l/arch
/cpu
power_management/
topology
  cells num='1'
cell id='0'
  memory unit='KiB'4186112/memory
  cpus num='4'
cpu id='0' socket_id='0' core_id='0' siblings='0-3'/
cpu id='1' socket_id='0' core_id='0' siblings='0-3'/
cpu id='2' socket_id='0' core_id='0' siblings='0-3'/
cpu id='3' socket_id='0' core_id='0' siblings='0-3'/
  /cpus
/cell
  /cells
/topology
  /host

  guest
os_typexen/os_type
arch name='armv7l'
  wordsize32/wordsize
  emulator/usr/lib/xen/bin/qemu-dm/emulator
  machinexenpv/machine
  domain type='xen'
  /domain
/arch
  /guest

/capabilities

Thanks,
Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] libxl: detect support for save and restore

[Ian Campbell]

On Wed, 2014-06-25 at 13:10 -0600, Eric Blake wrote:
 On 06/25/2014 12:13 PM, Jim Fehlig wrote:
  libxl does not support save, restore, or migrate on all architectures,
  notably ARM.  Detect whether libxl supports these operations using
  LIBXL_HAVE_NO_SUSPEND_RESUME.  If not supported, drop advertisement of
  migration_features.
  
  Found by Ian Campbell while improving Xen's OSSTEST infrastructure
  
  http://lists.xen.org/archives/html/xen-devel/2014-06/msg02171.html
  Signed-off-by: Jim Fehlig jfeh...@suse.com
  ---
  
  Derived from a test patch I sent to Ian Campbell
  
  http://lists.xen.org/archives/html/xen-devel/2014-06/msg03150.html
  
  Includes fixups Ian provided later in the thread.

I think it looks identical to that combination, in which case you can
add my Tested-by: Ian Campbell ian.campb...@citrix.com if you want.

  
   src/libxl/libxl_conf.c   | 4 
   src/libxl/libxl_driver.c | 8 
   2 files changed, 12 insertions(+)
 
   
  +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME
 
 Double negative logic is hard to read. Oh well.

libxl didn't initially supply a #define (because it only supported x86
which always did migration) and when ARM came along we could only add
something to new versions since obviously we can't change already
released stuff, so it had to be this way, sadly.

   static virDriver libxlDriver = {
  @@ -4594,10 +4598,12 @@ static virDriver libxlDriver = {
   .domainSetMemoryFlags = libxlDomainSetMemoryFlags, /* 0.9.0 */
   .domainGetInfo = libxlDomainGetInfo, /* 0.9.0 */
   .domainGetState = libxlDomainGetState, /* 0.9.2 */
  +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME
   .domainSave = libxlDomainSave, /* 0.9.2 */
   .domainSaveFlags = libxlDomainSaveFlags, /* 0.9.4 */
   .domainRestore = libxlDomainRestore, /* 0.9.2 */
   .domainRestoreFlags = libxlDomainRestoreFlags, /* 0.9.4 */
  +#endif
 
 Hmm - do we do conditional registration in any other driver based on
 configure-time results?  I'd almost rather always provide the driver
 registration, and then use #ifdefs in the body of that function to
 either provide a sane result or else report that the compilation
 environment was too old, rather than omit the support altogether.  Maybe
 get Dan's opinion on this?

From the Xen test harness' point of view we'd like virsh capabilities to
be accurate, FWIW.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Ian Campbell]

On Thu, 2014-06-19 at 00:15 -0600, Jim Fehlig wrote:
 Ian Campbell wrote:
  On Tue, 2014-06-17 at 16:24 +0100, Ian Campbell wrote:

  +if (xenXMConfigGetString(conf, extra, extra, NULL)  0)
  
 
  This was subtly broken. The default needs to be .

 
 Turns out, it wasn't :).  Prior to this patch, def-os.cmdline was set
 to NULL if 'extra' did not exist or was empty.  I pushed a fix to
 restore the behavior, as your original patch did
 
 https://www.redhat.com/archives/libvir-list/2014-June/msg00857.html

Sorry about that and thanks for the fix.

I see VIR_STRDUP actually does handle NULL input correctly, which was
what I was worried about.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH] libxl: prefer qdisk for driver name='file'

[Ian Campbell]

On Fri, 2014-06-20 at 15:07 -0600, Jim Fehlig wrote:
 The libxl driver currently sets the disk backend to
 LIBXL_DISK_BACKEND_TAP when driver name='file' is specified
 in the disk config.  qdisk should be prefered with this
 configuration, otherwise existing configuration such as the
 following, which worked with the old Xen driver, will not work
 with the libxl driver

OOI why not let libxl pick the most appropriate backend for (most of)
these alternatives?

 
   disk type='file' device='cdrom'
 driver name='file'/
 source file='/path/to/some/iso'/
 target dev='hdc' bus='ide'/
 readonly/
   /disk
 
 In addition, tap performs poorly compared to qdisk.
 
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 ---
  src/libxl/libxl_conf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
 index fdbb522..4b6b5c0 100644
 --- a/src/libxl/libxl_conf.c
 +++ b/src/libxl/libxl_conf.c
 @@ -796,7 +796,7 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, 
 libxl_device_disk *x_disk)
  return -1;
  }
  x_disk-format = LIBXL_DISK_FORMAT_RAW;
 -x_disk-backend = LIBXL_DISK_BACKEND_TAP;
 +x_disk-backend = LIBXL_DISK_BACKEND_QDISK;
  } else if (STREQ(driver, phy)) {
  if (format != VIR_STORAGE_FILE_NONE 
  format != VIR_STORAGE_FILE_RAW) {


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 4/4] libxl: Add a test suite for libxl option generator

[Ian Campbell]

On Tue, 2014-06-17 at 12:40 -0600, Jim Fehlig wrote:
 Ian Campbell wrote:
  On Mon, 2014-06-16 at 17:11 -0600, Jim Fehlig wrote:

  This function exists in Xen 4.2 as well, in libxl.h.


  Any ideas on how to handle this?  I'm not aware of an existing macro to
  check for func 'foo' defined in header 'bar'.  Is writing a custom macro
  along these lines a good solution?  A bad solution I tried was hacking
  the test to check libxl version via libxl_get_version_info(), but that
  API does not work if not running Xen.
  
 
  Given that it exists in everything from 4.2 onwards why do you need to
  check for it?

 
 Hrm, right.  I had the half-brained idea to use this to solve the
 failures I saw when testing this series against Xen 4.2
 
 https://www.redhat.com/archives/libvir-list/2014-June/msg00170.html
 
 I think the solution to that specific problem is to use Xen 4.2 config
 as the baseline.  But it got me thinking about the general problem you
 mentioned near the end of this mail
 
 https://www.redhat.com/archives/libvir-list/2014-June/msg00032.html
 
 With virJSONStringCompare in 1/1, Daniel provides a way to handle
 existence of new fields showing up in the json.  But what if I want to
 write a test where the expected data is not supported on earlier
 versions?   E.g. how would I add a test to check conversion of 'tpm
 ...' to 'vtpms: [ ]' and expect that to work when running 'make check'
 against a 4.2 libxl where vtpms were not yet supported?  I suppose each
 such test would have to probe for the feature it checks and skip if not
 found.

Right. You'd probably want to gate such a test case on the corresponding
LIBXL_HAVE_XXX #define from libxl.h.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Ian Campbell]

On Tue, 2014-06-17 at 16:24 +0100, Ian Campbell wrote:
 +if (xenXMConfigGetString(conf, extra, extra, NULL)  0)

This was subtly broken. The default needs to be .

-8--

From 539412a6deac8b928c82945d692ef20a49535d65 Mon Sep 17 00:00:00 2001
From: Ian Campbell ian.campb...@citrix.com
Date: Tue, 17 Jun 2014 15:48:48 +0100
Subject: [PATCH] xen: handle root= in xen-xm configuration files.

In addition to extra= xm supported a root= option which was supposed
to be incorporated into the final command line. Handle that for virsh
domxml-from-native xen-xm. Tested with the libxl backend.

Signed-off-by: Ian Campbell ian.campb...@citrix.com
---
v2: extra should default to .
---
 .gnulib|2 +-

--- WTF. I stripped this out of the patch shown below...

 src/xenxs/xen_xm.c |   14 +-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
index b2db97d..745041b 100644
--- a/src/xenxs/xen_xm.c
+++ b/src/xenxs/xen_xm.c
@@ -339,6 +339,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
 def-os.nBootDevs++;
 }
 } else {
+const char *extra, *root;
+
 if (xenXMConfigCopyStringOpt(conf, bootloader, def-os.bootloader) 
 0)
 goto cleanup;
 if (xenXMConfigCopyStringOpt(conf, bootargs, 
def-os.bootloaderArgs)  0)
@@ -348,8 +350,18 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
 goto cleanup;
 if (xenXMConfigCopyStringOpt(conf, ramdisk, def-os.initrd)  0)
 goto cleanup;
-if (xenXMConfigCopyStringOpt(conf, extra, def-os.cmdline)  0)
+if (xenXMConfigGetString(conf, extra, extra, )  0)
+goto cleanup;
+if (xenXMConfigGetString(conf, root, root, NULL)  0)
 goto cleanup;
+
+if (root) {
+if (virAsprintf(def-os.cmdline, root=%s %s, root, extra)  0)
+goto cleanup;
+} else {
+if (VIR_STRDUP(def-os.cmdline, extra)  0)
+goto cleanup;
+}
 }
 
 if (xenXMConfigGetULongLong(conf, memory, def-mem.cur_balloon,
-- 
1.7.10.4



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Ian Campbell]

On Tue, 2014-06-17 at 15:36 -0600, Jim Fehlig wrote:
 Eric Blake wrote:
  On 06/17/2014 09:24 AM, Ian Campbell wrote:

  In addition to extra= xm supported a root= option which was supposed
  to be incorporated into the final command line. Handle that for virsh
  domxml-from-native xen-xm. Tested with the libxl backend.
 
  Signed-off-by: Ian Campbell ian.campb...@citrix.com
  ---
   .gnulib|2 +-
   src/xenxs/xen_xm.c |   14 +-
   2 files changed, 14 insertions(+), 2 deletions(-)
 
  diff --git a/.gnulib b/.gnulib
  index d55899f..e8e0eb6 16
  --- a/.gnulib
  +++ b/.gnulib
  @@ -1 +1 @@
  -Subproject commit d55899fd2c5794ac85ecb14d5e2f646a89e4b4dd
  +Subproject commit e8e0eb6bfb728685ec8d5afd924e41b18e9d928d
  
 
  Was the submodule bump intended?

No, sorry, I've no idea how that happened (/me curses git submodules yet
again).

Once I understand that, then this patch (minus
  the .gnulib bump) seems okay.

NB I just sent out a v2 -- extra should default to  not NULL for this
to work as intended.

 BTW, if cmdline contains root=, I noticed that domxml-to-native will
 put it in extra= instead of creating a root= entry.  E.g.
 cmdlineroot=/dev/bla foo=bar/cmdline converts to
 extra=root=/dev/bla foo=bar, which is still valid config so perhaps
 not such a big deal.

I think this is fine.

Personally I consider the root= stuff to be a weird wart, in that it
effectively exposes details of the Linux command line syntax in the
xm/xl cfg file. It's far better IMHO to ignore it and write root=foo in
the actual command line.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v7 RFC 0/2] libxl USB prototype and design discussion

[Ian Campbell]

On Mon, 2014-06-02 at 14:44 +0100, George Dunlap wrote:
 == Open questions ==
 
 Those are things I think I know; there are a couple of pertinent
 factual questions which I'm not sure of:
 
 * Is it possible to specify PVUSB controllers and attach USB devices
 to them before the guest is up and running (i.e., before the frontend
 is connected)?  It looks like xend had a syntax for specifying virtual
 controllers and attaching virtual devices, so it seems like it should
 be possible.

Unless the PVUSB drivers are radically different to other PV devices
this ought to be possible and should just work. (Essentially you just
preload the backend with all the settings and htey get handled when the
f.e. turns up)

 * Is it possible to connect a USB 1.1 device to a PVUSB controller
 which has been specified 2.0, or would there have to be a separate
 virtual controller for each?

I'm a bit surprised that PVUSB exposes the concept of a version to the
FE at all. I suppose there is some USBish reason why the f.e. would
care.

But I don't know the answer to your question.

 * Is it possible for the toolstack to tell if dom0 (or whatever the
 specified backend domain) has PVUSB support before starting the guest?

After creating the nodes with state == XenbusStateInitialising (1) the
toolstack waits for the backend to move to XenbusStateInitWait (2)
before continuing, with a timeout. So you will detect this in a
controlled way. You can't tell before try the setup though since the
driver might be autoloaded.

(Assuming pvusbback is the same as everything else)

 * Is it possible for the toolstack to tell if domU has a working and
 connected PVUSB front-end?

It can observe the state variable being 4 I suppose. Why do you need to
know?

 * Do we want to be able to create virtual hubs for qemu-backed
 controllers at some point in the future?  Is there any groundwork we
 want to lay for that?

qemu-backed emulated or PV controllers?

I don't think emulated would make sense for a PV guest and if qemu
wanted to be a PV backend it would have to implement the usual xenstore
watches etc.

I suppose a backend type a la libxl_device_disk's = phy|tap|qdisk might
be needed for this, but I think you can probably add that in a
compatible way in the future if necessary.

 == Design questions ==
 
 Then based on that, we have several design questions.
 
 * How much of the controller thing should be exposed via libxl?  Via xl?
 
 * This series only allows you to specify the protocol, either PV or
 DEVICE_MODEL.  Would it be better, for instance, to get rid of that,
 and instead allow the user to specify the creation of USB controllers,
 allow them to have a type of HCI (or emulated) or PV, and allow
 the user to specify which controller to attach a specific device to?
 
 * How much smarts should we have in the libxl / xl about creating
 emulated/virtual controllers and of what kinds?
 
 * How much / what kind of smarts should be in libxl / xl about
 choosing a controller to plug a device into?

Dunno * 4. Your proposed design looked ok to me.

 * What about config file syntax?  Should we try to reuse / extend the
 current config syntax, or create a new one?  Should the new one allow
 the specification of controllers?  Should it perhaps *require* the
 specification of controllers?

We should at least strive for any existing xm config files which use USB
to continue working, but that needn't imply that the preferred xl syntax
looks that way.

Of course if the xm syntax is horribly terribly broken then we might
make a concious choice not to carry it forward, but the default should
be compatibility.


 For reference, here are some example config snippets from the current
 xl / xm config files:
 
 -- snip --
 # HVM USB
 usb=1
 usbdevice=['tablet','host:4.3']
 
 # HVM USB, not compatible with the above
 usbversion=3
 
 # xend's PVUSB config file; this creates one virtual controller, then
 # plugs hostdev 1.8 into port 1
 vusb=['usbver=2, numports=4, port_1=1-8']

Oh my. That last one is quite exciting.

 -- snip --
 
 Given that, here is a potential config file format:
 
 -- snip --
 # Create two controllers, one pv, one emulated
 usbcontroller=['type=pv,name=pv0,usbversion=2,numports=4',
'type=emul,name=hci0,usbversion=2']
 
 # Create a controller with the defaults; PV for PV guests, emul for HVM guests
 usbcontroller=[''] 
 
 # Same as above, but defaulting to version 2
 usbversion=2 
 
 # I think we should be able to automatically detect which format to
 # use; so I think we should re-use usbdevice.  I could be convinced otherwise.
 usbdevice=['type=tablet','type=hostdev,hostaddr=4.3,bus=pv0']

Does this require that the usbcontroller have been specified?

I think it would be good if xl would by default create some number of
appropriate controllers without my having to specify them explicitly,
iow just saying usbdevice=[...] should be enough.

I'm not saying that you shouldn't support more specific syntax for
people who want 

Re: [libvirt] [PATCH v2 4/4] libxl: Add a test suite for libxl option generator

[Ian Campbell]

On Mon, 2014-06-16 at 17:11 -0600, Jim Fehlig wrote:
  This function exists in Xen 4.2 as well, in libxl.h.

 
 Any ideas on how to handle this?  I'm not aware of an existing macro to
 check for func 'foo' defined in header 'bar'.  Is writing a custom macro
 along these lines a good solution?  A bad solution I tried was hacking
 the test to check libxl version via libxl_get_version_info(), but that
 API does not work if not running Xen.

Given that it exists in everything from 4.2 onwards why do you need to
check for it?

From the PoV of these tests (or any application generally) I'd have
thought you wouldn't really care if you get this function from libxl.h
directly or indirectly via libxl.h including _libxl_types.h.

Ian.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list