Re: [libvirt] [PATCH] Fix virNodeDeviceListCaps always returns empty

[Ján Tomko]

On 03/29/2014 12:13 AM, Jincheng Miao wrote:
> virNodeDeviceListCaps will always return empty for a pci nodedevice,
> actually it should return 'pci'.
> 
> It is because the loop variable ncaps isn't increased.
> 

Introduced by commit be2636f.

> https://bugzilla.redhat.com/show_bug.cgi?id=1081932
> 
> Signed-off-by: Jincheng Miao 
> ---
>  src/node_device/node_device_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

ACK and pushed.

Jan



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Guido Günther]

Hi Daniel,
On Fri, Mar 28, 2014 at 02:48:41PM +, Daniel P. Berrange wrote:
> On Fri, Mar 28, 2014 at 03:16:37PM +0100, Guido Günther wrote:
> > On Fri, Mar 28, 2014 at 12:26:26PM +, Daniel P. Berrange wrote:
> > > A bunch of tests currently attempt to kickstart a full Fedora
> > > OS image install. Everytime I try to update this kickstart to
> > > a new version of Fedora it causes no end of pain. Switch the
> > > tests over to use Richard Jones' virt-builder command which
> > > is part of libguestfs. This makes it trivial to deploy and
> > > customize full OS images from pre-built templates.
> > > 
> > > Mike - does Suse include a new enough version of libguestfs
> > > to get access to the 'virt-builder' tool yet ?
> > 
> > Just as a data point: I'm running on Debian Wheezy via Jenkins on a
> > regular basis (i.e. on libvirt commit). Wheezy has 1.18.1 which isn't
> > recent enough. I also ran into kickstart hazzles several times but
> > keeping the entry barrier low to use libvirt-tck might be more
> > important.
> 
> So if lack of virt-builder is a significant problem for you, then one
> option is for us to directly download the disk images from
> 
>   http://libguestfs.org/download/builder/
> 
> and then just run virt-sysprep on it ourselves. We don't actually
> need much of the fancy code virt-builder has, so it wouldn't be too
> much work to do it.
> 
> How much longer do you expect to be needing to support running on
> Wheezy ? Are we talking years, or just months ?

I aim to support testing on wheezy until it goes EOL which will be
years rather than months. However updating the machine the tests are run
_from_ to a newer version that has a recent guestfs will be easy. I'll
just have to check how well running libvirt-tck against remote URIs is
supported. I've disabled updating libvirt-tck for now:

http://honk.sigxcpu.org:8001/view/libvirt/job/libvirt-tck-build/

so no worries merging your changes.
Cheers,
 -- Guido


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

[libvirt] [PATCH] Fix virNodeDeviceListCaps always returns empty

[Jincheng Miao]

virNodeDeviceListCaps will always return empty for a pci nodedevice,
actually it should return 'pci'.

It is because the loop variable ncaps isn't increased.

https://bugzilla.redhat.com/show_bug.cgi?id=1081932

Signed-off-by: Jincheng Miao 
---
 src/node_device/node_device_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 1f3a083..6906463 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -420,7 +420,7 @@ nodeDeviceListCaps(virNodeDevicePtr dev, char **const 
names, int maxnames)
 goto cleanup;
 
 for (caps = obj->def->caps; caps && ncaps < maxnames; caps = caps->next) {
-if (VIR_STRDUP(names[ncaps], virNodeDevCapTypeToString(caps->type)) < 
0)
+if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->type)) 
< 0)
 goto cleanup;
 }
 ret = ncaps;
-- 
1.8.2.1

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

Re: [libvirt] [PATCH 3/6] storage: netfs: Support lookup of glusterfs pool sources

[Ján Tomko]

On 03/28/2014 11:01 PM, Peter Krempa wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1072714
> 
> Use the "gluster" command line tool to retrieve information about remote
> volumes on a gluster server to allow storage pool source lookup.
> 
> Unfortunately gluster doesn't provide a management library so that we
> could use that directly, instead the RPC calls are hardcoded in the
> command line tool.
> ---
>  configure.ac |  6 +++
>  src/storage/storage_backend.c| 86 
> 
>  src/storage/storage_backend.h|  4 ++
>  src/storage/storage_backend_fs.c |  5 +++
>  4 files changed, 101 insertions(+)
> 

> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 5b3b536..c7e4688 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1640,3 +1640,89 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool,
> 
>  return stablepath;
>  }
> +
> +#ifdef GLUSTER_CLI
> +int
> +virStorageBackendFindGlusterPoolSources(const char *host,
> +int pooltype,
> +virStoragePoolSourceListPtr list)
> +{
> +char *outbuf = NULL;
> +virCommandPtr cmd = NULL;

> +xmlDocPtr doc = NULL;
> +xmlXPathContextPtr ctxt = NULL;
> +xmlNodePtr *nodes = NULL;

These three are never freed.

Jan




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/6] storage: gluster: Implement storage pool lookup

[Eric Blake]

On 03/28/2014 04:01 PM, Peter Krempa wrote:
> Use the previously implemented function to lookup glusterfs source pools
> for the netfs pool to lookup native gluster pools too.
> ---
>  src/storage/storage_backend_gluster.c | 50 
> +++
>  1 file changed, 50 insertions(+)
> 

> +
> +if (!srcSpec) {
> +virReportError(VIR_ERR_INVALID_ARG, "%s",
> +   _("hostname must be specified for gluster sources"));

Would it make sense to check localhost if srcSpec is NULL?

Again, post-1.2.3, but looks reasonable.  ACK

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/6] storage: netfs: Support lookup of glusterfs pool sources

[Eric Blake]

On 03/28/2014 04:01 PM, Peter Krempa wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1072714
> 
> Use the "gluster" command line tool to retrieve information about remote
> volumes on a gluster server to allow storage pool source lookup.
> 
> Unfortunately gluster doesn't provide a management library so that we
> could use that directly, instead the RPC calls are hardcoded in the
> command line tool.
> ---
>  configure.ac |  6 +++
>  src/storage/storage_backend.c| 86 
> 
>  src/storage/storage_backend.h|  4 ++
>  src/storage/storage_backend_fs.c |  5 +++
>  4 files changed, 101 insertions(+)

New feature rather than bug fix; please wait until after 1.2.3 to push.


> +cmd = virCommandNewArgList(GLUSTER_CLI,
> +   "--xml",

At least it's machine-parseable, and not free-form regex prone to
mistakes on funky names :)


> +if (virCommandRun(cmd, &rc) < 0)
> +goto cleanup;
> +
> +if (rc != 0) {
> +VIR_INFO("failed to query host '%s' for gluster volumes: %s",
> + host, outbuf);

Fair enough to suppress log messages on failure.

ACK

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/6] storage: netfs: Split up and tidy up NFS storage pool source function

[Eric Blake]

On 03/28/2014 04:01 PM, Peter Krempa wrote:
> Extract the NFS related stuff into a separate function and tidy up the
> rest of the code so we can reuse it to add gluster backend detection.
> 
> Additionally avoid reporting of errors from "showmount" and return an
> empty source list instead. This will help when adding other detection
> backends.
> ---
>  src/storage/storage_backend_fs.c | 53 
> +++-
>  1 file changed, 31 insertions(+), 22 deletions(-)

Mostly mechanical, but it's borderline (used by patch 4/6, which feels
more like a feature than bug fix) and big enough that I'd feel better
waiting for 1.2.4 before pushing this one.  But once that happens,

ACK

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 8/n] conf: move storage source type to util/

[Eric Blake]

With this patch, all information related to a host resource in
a storage file backing chain now lives in util/virstoragefile.h.
The next step will be to consolidate various places that have
been tracking backing chain details to all use a common struct.

The changes to tools/Makefile.am were made necessary by the
fact that virstorageencryption includes uses of libxml, and is
now pulled in by inclusion from virstoragefile.h.  No
additional libraries are linked into the final image, and in
comparison, the build of the setuid library in src/Makefile.am
already was using LIBXML_CFLAGS via AM_CFLAGS.

* src/conf/domain_conf.h (virDomainDiskSourceDef): Move...
* src/util/virstoragefile.h (virStorageSource): ...and rename.
* src/conf/domain_conf.c (virDomainDiskSourceDefClear)
(virDomainDiskAuthClear): Adjust clients.
* tools/Makefile.am (virt_login_shell_CFLAGS)
(virt_host_validate_CFLAGS): Add libxml headers.

Signed-off-by: Eric Blake 
---

Surprisingly small in size; a lot of the heavy lifting was
done piecemeal in the earlier commits.

This one took me a while to figure out, because it took me
longer than expected to figure out the best way to resolve
the build failure of virt-login-shell and still make sure
I wasn't introducing a problem into that setuid helper.

 src/conf/domain_conf.c|  4 ++--
 src/conf/domain_conf.h| 32 ++--
 src/util/virstoragefile.h | 32 
 tools/Makefile.am |  2 ++
 4 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a5afacf..b38021d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1193,7 +1193,7 @@ virDomainDiskSourcePoolDefFree(virStorageSourcePoolDefPtr 
def)


 static void
-virDomainDiskSourceDefClear(virDomainDiskSourceDefPtr def)
+virDomainDiskSourceDefClear(virStorageSourcePtr def)
 {
 size_t i;

@@ -1237,7 +1237,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def)


 void
-virDomainDiskAuthClear(virDomainDiskSourceDefPtr def)
+virDomainDiskAuthClear(virStorageSourcePtr def)
 {
 VIR_FREE(def->auth.username);

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 52b59a1..b011847 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -595,38 +595,10 @@ struct _virDomainBlockIoTuneInfo {
 };
 typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;

-typedef struct _virDomainDiskSourceDef virDomainDiskSourceDef;
-typedef virDomainDiskSourceDef *virDomainDiskSourceDefPtr;
-
-/* Stores information related to a host resource.  In the case of
- * backing chains, multiple source disks join to form a single guest
- * view.  TODO Move this to util/ */
-struct _virDomainDiskSourceDef {
-int type; /* enum virStorageType */
-char *path;
-int protocol; /* enum virStorageNetProtocol */
-size_t nhosts;
-virStorageNetHostDefPtr hosts;
-virStorageSourcePoolDefPtr srcpool;
-struct {
-char *username;
-int secretType; /* enum virStorageSecretType */
-union {
-unsigned char uuid[VIR_UUID_BUFLEN];
-char *usage;
-} secret;
-} auth;
-virStorageEncryptionPtr encryption;
-char *driverName;
-int format; /* enum virStorageFileFormat */
-
-size_t nseclabels;
-virSecurityDeviceLabelDefPtr *seclabels;
-};

 /* Stores the virtual disk configuration */
 struct _virDomainDiskDef {
-virDomainDiskSourceDef src;
+virStorageSource src;

 int device; /* enum virDomainDiskDevice */
 int bus; /* enum virDomainDiskBus */
@@ -2153,7 +2125,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr 
def);
 void virDomainInputDefFree(virDomainInputDefPtr def);
 void virDomainDiskDefFree(virDomainDiskDefPtr def);
 void virDomainLeaseDefFree(virDomainLeaseDefPtr def);
-void virDomainDiskAuthClear(virDomainDiskSourceDefPtr def);
+void virDomainDiskAuthClear(virStorageSourcePtr def);
 int virDomainDiskGetType(virDomainDiskDefPtr def);
 void virDomainDiskSetType(virDomainDiskDefPtr def, int type);
 int virDomainDiskGetActualType(virDomainDiskDefPtr def);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 68172c8..a6dcfa4 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -25,6 +25,8 @@
 # define __VIR_STORAGE_FILE_H__

 # include "virbitmap.h"
+# include "virseclabel.h"
+# include "virstorageencryption.h"
 # include "virutil.h"

 /* Minimum header size required to probe all known formats with
@@ -182,6 +184,36 @@ enum virStorageSecretType {
 };


+typedef struct _virStorageSource virStorageSource;
+typedef virStorageSource *virStorageSourcePtr;
+
+/* Stores information related to a host resource.  In the case of
+ * backing chains, multiple source disks join to form a single guest
+ * view.  */
+struct _virStorageSource {
+int type; /* enum virStorageType */
+char *path;
+int protocol; /* enum virStorageNetProtocol */
+size_t nhosts;
+virStorageN

Re: [libvirt] [PATCH 6/6] util: storagefile: Don't pursue backing chain of NULL image

[Eric Blake]

On 03/28/2014 04:01 PM, Peter Krempa wrote:
> When virStorageFileGetMetadata is called with NULL path argument, the
> invalid pointer boils down through the recursive worker and is caught by
> virHashAddEntry which is thankfully resistant to NULL arguments. As it
> doesn't make sense to pursue backing chains of NULL volumes, exit
> earlier.
> 
> This was noticed in the virt-aahelper-test with a slightly modified
> codebase.
> ---
>  src/util/virstoragefile.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

ACK; safe for 1.2.3

> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index c7384d1..8370d23 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1118,7 +1118,7 @@ virStorageFileGetMetadata(const char *path, int format,
>  virHashTablePtr cycle = virHashCreate(5, NULL);
>  virStorageFileMetadataPtr ret;
> 
> -if (!cycle)
> +if (!cycle || !path)
>  return NULL;
> 
>  if (format <= VIR_STORAGE_FILE_NONE)
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/6] storage: gluster: Fix crash when initialization of storage backend fails

[Eric Blake]

On 03/28/2014 04:01 PM, Peter Krempa wrote:
> The libgfapi function glfs_fini doesn't tolerate NULL pointers. Add a
> check on the error paths as it's possible to crash libvirtd if the
> gluster volume can't be initialized.
> ---
>  src/storage/storage_backend_gluster.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

ACK; safe for 1.2.3

> 
> diff --git a/src/storage/storage_backend_gluster.c 
> b/src/storage/storage_backend_gluster.c
> index 5b79146..fe92f15 100644
> --- a/src/storage/storage_backend_gluster.c
> +++ b/src/storage/storage_backend_gluster.c
> @@ -548,7 +548,8 @@ virStorageFileBackendGlusterDeinit(virStorageFilePtr file)
>file, file->hosts[0].name, file->path);
>  virStorageFileBackendGlusterPrivPtr priv = file->priv;
> 
> -glfs_fini(priv->vol);
> +if (priv->vol)
> +glfs_fini(priv->vol);
>  VIR_FREE(priv->volname);
> 
>  VIR_FREE(priv);
> @@ -621,7 +622,8 @@ virStorageFileBackendGlusterInit(virStorageFilePtr file)
> 
>   error:
>  VIR_FREE(priv->volname);
> -glfs_fini(priv->vol);
> +if (priv->vol)
> +glfs_fini(priv->vol);
>  VIR_FREE(priv);
> 
>  return -1;
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/6] storage: pool: Fix XML indentation in pool source lookup

[Eric Blake]

On 03/28/2014 04:01 PM, Peter Krempa wrote:
> The  elements need to be indented from  elements.
> ---
>  src/conf/storage_conf.c | 2 ++
>  1 file changed, 2 insertions(+)

ACK; safe for 1.2.3

> 
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index a300476..65504b4 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2006,11 +2006,13 @@ 
> virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def)
>  }
> 
>  virBufferAddLit(&buf, "\n");
> +virBufferAdjustIndent(&buf, 2);
> 
>  for (i = 0; i < def->nsources; i++) {
>  virStoragePoolSourceFormat(&buf, options, &def->sources[i]);
>  }
> 
> +virBufferAdjustIndent(&buf, -2);
>  virBufferAddLit(&buf, "\n");
> 
>  if (virBufferError(&buf))
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 5/6] storage: gluster: Fix crash when initialization of storage backend fails

[Peter Krempa]

The libgfapi function glfs_fini doesn't tolerate NULL pointers. Add a
check on the error paths as it's possible to crash libvirtd if the
gluster volume can't be initialized.
---
 src/storage/storage_backend_gluster.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_backend_gluster.c 
b/src/storage/storage_backend_gluster.c
index 5b79146..fe92f15 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -548,7 +548,8 @@ virStorageFileBackendGlusterDeinit(virStorageFilePtr file)
   file, file->hosts[0].name, file->path);
 virStorageFileBackendGlusterPrivPtr priv = file->priv;

-glfs_fini(priv->vol);
+if (priv->vol)
+glfs_fini(priv->vol);
 VIR_FREE(priv->volname);

 VIR_FREE(priv);
@@ -621,7 +622,8 @@ virStorageFileBackendGlusterInit(virStorageFilePtr file)

  error:
 VIR_FREE(priv->volname);
-glfs_fini(priv->vol);
+if (priv->vol)
+glfs_fini(priv->vol);
 VIR_FREE(priv);

 return -1;
-- 
1.9.1

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

[libvirt] [PATCH 2/6] storage: netfs: Split up and tidy up NFS storage pool source function

[Peter Krempa]

Extract the NFS related stuff into a separate function and tidy up the
rest of the code so we can reuse it to add gluster backend detection.

Additionally avoid reporting of errors from "showmount" and return an
empty source list instead. This will help when adding other detection
backends.
---
 src/storage/storage_backend_fs.c | 53 +++-
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 0f8da06..8fb03c5 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -242,10 +242,8 @@ virStorageBackendFileSystemNetFindPoolSourcesFunc(char 
**const groups,
 }


-static char *
-virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn 
ATTRIBUTE_UNUSED,
-  const char *srcSpec,
-  unsigned int flags)
+static void
+virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state)
 {
 /*
  *  # showmount --no-headers -e HOSTNAME
@@ -261,6 +259,29 @@ 
virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE
 int vars[] = {
 1
 };
+
+virCommandPtr cmd = NULL;
+
+cmd = virCommandNewArgList(SHOWMOUNT,
+   "--no-headers",
+   "--exports",
+   state->host,
+   NULL);
+
+if (virCommandRunRegex(cmd, 1, regexes, vars,
+   virStorageBackendFileSystemNetFindPoolSourcesFunc,
+   &state, NULL) < 0)
+virResetLastError();
+
+virCommandFree(cmd);
+}
+
+
+static char *
+virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn 
ATTRIBUTE_UNUSED,
+  const char *srcSpec,
+  unsigned int flags)
+{
 virNetfsDiscoverState state = {
 .host = NULL,
 .list = {
@@ -270,15 +291,14 @@ 
virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE
 }
 };
 virStoragePoolSourcePtr source = NULL;
-char *retval = NULL;
+char *ret = NULL;
 size_t i;
-virCommandPtr cmd = NULL;

 virCheckFlags(0, NULL);

 if (!srcSpec) {
-virReportError(VIR_ERR_INVALID_ARG,
-   "%s", _("hostname must be specified for netfs 
sources"));
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("hostname must be specified for netfs sources"));
 return NULL;
 }

@@ -294,19 +314,9 @@ 
virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE

 state.host = source->hosts[0].name;

-cmd = virCommandNewArgList(SHOWMOUNT,
-   "--no-headers",
-   "--exports",
-   source->hosts[0].name,
-   NULL);
+virStorageBackendFileSystemNetFindNFSPoolSources(&state);

-if (virCommandRunRegex(cmd, 1, regexes, vars,
-   virStorageBackendFileSystemNetFindPoolSourcesFunc,
-   &state, NULL) < 0)
-goto cleanup;
-
-retval = virStoragePoolSourceListFormat(&state.list);
-if (retval == NULL)
+if (!(ret = virStoragePoolSourceListFormat(&state.list)))
 goto cleanup;

  cleanup:
@@ -315,8 +325,7 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr 
conn ATTRIBUTE_UNUSE
 VIR_FREE(state.list.sources);

 virStoragePoolSourceFree(source);
-virCommandFree(cmd);
-return retval;
+return ret;
 }


-- 
1.9.1

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

[libvirt] [PATCH 4/6] storage: gluster: Implement storage pool lookup

[Peter Krempa]

Use the previously implemented function to lookup glusterfs source pools
for the netfs pool to lookup native gluster pools too.
---
 src/storage/storage_backend_gluster.c | 50 +++
 1 file changed, 50 insertions(+)

diff --git a/src/storage/storage_backend_gluster.c 
b/src/storage/storage_backend_gluster.c
index 9a6180e..5b79146 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -472,10 +472,60 @@ virStorageBackendGlusterVolDelete(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 }


+static char *
+virStorageBackendGlusterFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED,
+const char *srcSpec,
+unsigned int flags)
+{
+virStoragePoolSourceList list = { .type = VIR_STORAGE_POOL_GLUSTER,
+  .nsources = 0,
+  .sources = NULL
+};
+virStoragePoolSourcePtr source = NULL;
+char *ret = NULL;
+size_t i;
+
+virCheckFlags(0, NULL);
+
+if (!srcSpec) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("hostname must be specified for gluster sources"));
+return NULL;
+}
+
+if (!(source = virStoragePoolDefParseSourceString(srcSpec,
+  
VIR_STORAGE_POOL_GLUSTER)))
+return NULL;
+
+if (source->nhost != 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Expected exactly 1 host for the storage pool"));
+goto cleanup;
+}
+
+if (virStorageBackendFindGlusterPoolSources(source->hosts[0].name,
+0, /* currently ignored */
+&list) < 0)
+goto cleanup;
+
+if (!(ret = virStoragePoolSourceListFormat(&list)))
+goto cleanup;
+
+ cleanup:
+for (i = 0; i < list.nsources; i++)
+virStoragePoolSourceClear(&list.sources[i]);
+VIR_FREE(list.sources);
+
+virStoragePoolSourceFree(source);
+return ret;
+}
+
+
 virStorageBackend virStorageBackendGluster = {
 .type = VIR_STORAGE_POOL_GLUSTER,

 .refreshPool = virStorageBackendGlusterRefreshPool,
+.findPoolSources = virStorageBackendGlusterFindPoolSources,

 .deleteVol = virStorageBackendGlusterVolDelete,
 };
-- 
1.9.1

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

[libvirt] [PATCH 0/6] Gluster pool lookup and few gluster related fixes

[Peter Krempa]

Peter Krempa (6):
  storage: pool: Fix XML indentation in pool source lookup
  storage: netfs: Split up and tidy up NFS storage pool source function
  storage: netfs: Support lookup of glusterfs pool sources
  storage: gluster: Implement storage pool lookup
  storage: gluster: Fix crash when initialization of storage backend
fails
  util: storagefile: Don't pursue backing chain of NULL image

 configure.ac  |  6 +++
 src/conf/storage_conf.c   |  2 +
 src/storage/storage_backend.c | 86 +++
 src/storage/storage_backend.h |  4 ++
 src/storage/storage_backend_fs.c  | 54 ++
 src/storage/storage_backend_gluster.c | 56 ++-
 src/util/virstoragefile.c |  2 +-
 7 files changed, 187 insertions(+), 23 deletions(-)

-- 
1.9.1

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

[libvirt] [PATCH 1/6] storage: pool: Fix XML indentation in pool source lookup

[Peter Krempa]

The  elements need to be indented from  elements.
---
 src/conf/storage_conf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index a300476..65504b4 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2006,11 +2006,13 @@ 
virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def)
 }

 virBufferAddLit(&buf, "\n");
+virBufferAdjustIndent(&buf, 2);

 for (i = 0; i < def->nsources; i++) {
 virStoragePoolSourceFormat(&buf, options, &def->sources[i]);
 }

+virBufferAdjustIndent(&buf, -2);
 virBufferAddLit(&buf, "\n");

 if (virBufferError(&buf))
-- 
1.9.1

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

[libvirt] [PATCH 6/6] util: storagefile: Don't pursue backing chain of NULL image

[Peter Krempa]

When virStorageFileGetMetadata is called with NULL path argument, the
invalid pointer boils down through the recursive worker and is caught by
virHashAddEntry which is thankfully resistant to NULL arguments. As it
doesn't make sense to pursue backing chains of NULL volumes, exit
earlier.

This was noticed in the virt-aahelper-test with a slightly modified
codebase.
---
 src/util/virstoragefile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index c7384d1..8370d23 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1118,7 +1118,7 @@ virStorageFileGetMetadata(const char *path, int format,
 virHashTablePtr cycle = virHashCreate(5, NULL);
 virStorageFileMetadataPtr ret;

-if (!cycle)
+if (!cycle || !path)
 return NULL;

 if (format <= VIR_STORAGE_FILE_NONE)
-- 
1.9.1

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

[libvirt] [PATCH 3/6] storage: netfs: Support lookup of glusterfs pool sources

[Peter Krempa]

https://bugzilla.redhat.com/show_bug.cgi?id=1072714

Use the "gluster" command line tool to retrieve information about remote
volumes on a gluster server to allow storage pool source lookup.

Unfortunately gluster doesn't provide a management library so that we
could use that directly, instead the RPC calls are hardcoded in the
command line tool.
---
 configure.ac |  6 +++
 src/storage/storage_backend.c| 86 
 src/storage/storage_backend.h|  4 ++
 src/storage/storage_backend_fs.c |  5 +++
 4 files changed, 101 insertions(+)

diff --git a/configure.ac b/configure.ac
index 73efffa..b60da1c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1936,6 +1936,12 @@ if test "$with_storage_gluster" = "yes"; then
 fi
 AM_CONDITIONAL([WITH_STORAGE_GLUSTER], [test "$with_storage_gluster" = "yes"])

+if test "$with_storage_fs" = "yes" ||
+   test "$with_storage_gluster" = "yes"; then
+  AC_PATH_PROG([GLUSTER_CLI], [gluster], [], [$PATH:/sbin:/usr/sbin])
+  AC_DEFINE_UNQUOTED([GLUSTER_CLI], ["$GLUSTER_CLI"],
+[Location or name of the gluster command line tool])
+fi

 LIBPARTED_CFLAGS=
 LIBPARTED_LIBS=
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 5b3b536..c7e4688 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1640,3 +1640,89 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool,

 return stablepath;
 }
+
+#ifdef GLUSTER_CLI
+int
+virStorageBackendFindGlusterPoolSources(const char *host,
+int pooltype,
+virStoragePoolSourceListPtr list)
+{
+char *outbuf = NULL;
+virCommandPtr cmd = NULL;
+xmlDocPtr doc = NULL;
+xmlXPathContextPtr ctxt = NULL;
+xmlNodePtr *nodes = NULL;
+virStoragePoolSource *src = NULL;
+size_t i;
+int nnodes;
+int rc;
+
+int ret = -1;
+
+cmd = virCommandNewArgList(GLUSTER_CLI,
+   "--xml",
+   "--log-file=/dev/null",
+   "volume", "info", "all", NULL);
+
+virCommandAddArgFormat(cmd, "--remote-host=%s", host);
+virCommandSetOutputBuffer(cmd, &outbuf);
+
+if (virCommandRun(cmd, &rc) < 0)
+goto cleanup;
+
+if (rc != 0) {
+VIR_INFO("failed to query host '%s' for gluster volumes: %s",
+ host, outbuf);
+ret = 0;
+goto cleanup;
+}
+
+if (!(doc = virXMLParseStringCtxt(outbuf, _("(gluster_cli_output)"),
+  &ctxt)))
+goto cleanup;
+
+if ((nnodes = virXPathNodeSet("//volumes/volume", ctxt, &nodes)) <= 0) {
+VIR_INFO("no gluster volumes available on '%s'", host);
+ret = 0;
+goto cleanup;
+}
+
+for (i = 0; i < nnodes; i++) {
+ctxt->node = nodes[i];
+
+if (!(src = virStoragePoolSourceListNewSource(list)))
+goto cleanup;
+
+if (!(src->dir = virXPathString("string(//name)", ctxt))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to extract gluster volume name"));
+goto cleanup;
+}
+
+if (VIR_ALLOC_N(src->hosts, 1) < 0)
+goto cleanup;
+src->nhost = 1;
+
+if (VIR_STRDUP(src->hosts[0].name, host) < 0)
+goto cleanup;
+
+src->format = pooltype;
+}
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(outbuf);
+virCommandFree(cmd);
+return ret;
+}
+#else /* #ifdef GLUSTER_CLI */
+int
+virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED,
+int pooltype ATTRIBUTE_UNUSED,
+virStoragePoolSourceListPtr list 
ATTRIBUTE_UNUSED)
+{
+VIR_INFO("gluster cli tool not installed");
+return 0;
+}
+#endif /* #ifdef GLUSTER_CLI */
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 2034a22..042ecfa 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -87,6 +87,10 @@ int virStorageBackendFindFSImageTool(char **tool);
 virStorageBackendBuildVolFrom
 virStorageBackendFSImageToolTypeToFunc(int tool_type);

+int virStorageBackendFindGlusterPoolSources(const char *host,
+int pooltype,
+virStoragePoolSourceListPtr list);
+

 typedef struct _virStorageBackend virStorageBackend;
 typedef virStorageBackend *virStorageBackendPtr;
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 8fb03c5..fd53691 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -316,6 +316,11 @@ 
virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE

 virStorageBackendFileSystemNetFindNFSPoolSources(&state);

+if (virStorageBackendFindGlusterPoolSources(state.host,
+  

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Richard W.M. Jones]

On Fri, Mar 28, 2014 at 11:44:43AM -0600, Mike Latimer wrote:
> On Friday, March 28, 2014 05:34:19 PM Richard W.M. Jones wrote:
> > On Fri, Mar 28, 2014 at 11:06:56AM -0600, Mike Latimer wrote:
> > > ACK to all of this. For some reason, Fedora 20 didn't want to install for
> > > me (it failed when resizing the disk, and I don't have time to chase it
> > > today), but Fedora 19 did. Looks like there are no SUSE images in
> > > virt-builder, so I'll work on adding those later.
> > 
> > On this point, you only need to publish a metadata file in the format
> > described here:
> > 
> > http://libguestfs.org/virt-builder.1.html#sources-of-templates
> > 
> > See http://libguestfs.org/download/builder/index.asc for an example.
> > 
> > If you have cloud images already, it makes sense to point the metadata
> > at those.
> 
> Thanks for the pointers. I saw your earlier messages about virt-builder, just 
> never had a chance to dive into it. With Daniel's push, I now have an excuse 
> to figure it out.  ;)

You probably want to CC any questions to me and Pino Toscano.
He wrote most of it ..

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

Re: [libvirt] [PATCH] Drop dependency on pm-is-supported

[Eric Blake]

On 03/28/2014 10:32 AM, Cédric Bosdonnat wrote:
> From: Cédric Bosdonnat 
> 
> pm-is-supported is the only thing needed in pm-utils, better get rid of
> it since systemd is heavily used for libvirt.
> ---
>  src/util/virnodesuspend.c | 34 --
>  1 file changed, 20 insertions(+), 14 deletions(-)

You also need to modify libvirt.spec.in to drop the dependency.

> 
>  
> +if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
> +goto cleanup;
> +
> +states = virStringSplit(buf, " ", 0);
> +
> +canSuspend = (virStringArrayHasString(states, "mem") ||
> +  virStringArrayHasString(states, "standby"));
> +canHibernate = virStringArrayHasString(states, "disk");

pm-is-supported checks a bit more than what your replacement checks.

For suspend, it declares yes if any of these succeed:
grep -q mem /sys/power/state
[ -c /dev/pmu ] && pm-pmu --check
grep -q standby /sys/power/state

For hibernate, it requires that BOTH of these succeed:
[ -f /sys/power/disk ]
grep -q disk /sys/power/state

For hybrid, it requires that all three succeed:
[ -f /sys/power/disk ] && \
grep -q disk /sys/power/state && \
grep -q suspend /sys/power/disk

as well as having fallback code to fake a hybrid sleep by joining the
other two states.


> +
>  switch (target) {
>  case VIR_NODE_SUSPEND_TARGET_MEM:
> -cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL);
> +*supported = canSuspend;
>  break;
>  case VIR_NODE_SUSPEND_TARGET_DISK:
> -cmd = virCommandNewArgList("pm-is-supported", "--hibernate", NULL);
> +*supported = canHibernate;
>  break;
>  case VIR_NODE_SUSPEND_TARGET_HYBRID:
> -cmd = virCommandNewArgList("pm-is-supported", "--suspend-hybrid", 
> NULL);
> +*supported = canSuspend && canHibernate;

I'm not sure if your simpler checks will cause us to declare an action
unsupported on systems where it was previously declared supported by
pm-is-supported.  I think the idea makes sense, but I'd like a second
opinion that we aren't hurting ourselves by doing fewer checks than what
we are replacing.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Drop dependency on pm-is-supported

[Eric Blake]

On 03/28/2014 01:49 PM, Doug Goldstein wrote:
> 
> On Mar 28, 2014, at 11:32 AM, Cédric Bosdonnat  wrote:
>>
>> From: Cédric Bosdonnat 
>>
>> pm-is-supported is the only thing needed in pm-utils, better get rid of
>> it since systemd is heavily used for libvirt.
>> ---
>> src/util/virnodesuspend.c | 34 --
>> 1 file changed, 20 insertions(+), 14 deletions(-)
>>

>>
>> +if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
>> +goto cleanup;
>> +

>> case VIR_NODE_SUSPEND_TARGET_MEM:
>> -cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL);
>> +*supported = canSuspend;

> While systemd is used for a number of things there are other platforms than 
> Linux and there are Linux platforms that don't use systemd. Can't we just 
> wrap this in if not systemd?

How long has the kernel been providing /sys/power/state and
/sys/power/disk?  It looks like pm-is-supported is just a shell script
that greps these two files - so it makes total sense to me to likewise
just search these files ourselves instead of going through a shell
script to do it on our behalf.  Thus, this patch is independent of
whether we use systemd, and more a case of avoiding a dependence on a
package for the use of a single shell script, especially for platforms
where systemd is taking over most other functionality in that package.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Drop dependency on pm-is-supported

[Doug Goldstein]

On Mar 28, 2014, at 11:32 AM, Cédric Bosdonnat  wrote:
> 
> From: Cédric Bosdonnat 
> 
> pm-is-supported is the only thing needed in pm-utils, better get rid of
> it since systemd is heavily used for libvirt.
> ---
> src/util/virnodesuspend.c | 34 --
> 1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c
> index 8088931..9839de2 100644
> --- a/src/util/virnodesuspend.c
> +++ b/src/util/virnodesuspend.c
> @@ -29,6 +29,8 @@
> #include "viralloc.h"
> #include "virlog.h"
> #include "virerror.h"
> +#include "virfile.h"
> +#include "virstring.h"
> 
> #define VIR_FROM_THIS VIR_FROM_NONE
> 
> @@ -263,41 +265,45 @@ int nodeSuspendForDuration(unsigned int target,
> static int
> virNodeSuspendSupportsTarget(unsigned int target, bool *supported)
> {
> -virCommandPtr cmd;
> -int status;
> int ret = -1;
> +char *buf = NULL;
> +char **states = NULL;
> +bool canSuspend = false;
> +bool canHibernate = false;
> 
> if (virNodeSuspendInitialize() < 0)
> return -1;
> 
> *supported = false;
> 
> +if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
> +goto cleanup;
> +
> +states = virStringSplit(buf, " ", 0);
> +
> +canSuspend = (virStringArrayHasString(states, "mem") ||
> +  virStringArrayHasString(states, "standby"));
> +canHibernate = virStringArrayHasString(states, "disk");
> +
> switch (target) {
> case VIR_NODE_SUSPEND_TARGET_MEM:
> -cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL);
> +*supported = canSuspend;
> break;
> case VIR_NODE_SUSPEND_TARGET_DISK:
> -cmd = virCommandNewArgList("pm-is-supported", "--hibernate", NULL);
> +*supported = canHibernate;
> break;
> case VIR_NODE_SUSPEND_TARGET_HYBRID:
> -cmd = virCommandNewArgList("pm-is-supported", "--suspend-hybrid", 
> NULL);
> +*supported = canSuspend && canHibernate;
> break;
> default:
> return ret;
> }
> 
> -if (virCommandRun(cmd, &status) < 0)
> -goto cleanup;
> -
> -   /*
> -* Check return code of command == 0 for success
> -* (i.e., the PM capability is supported)
> -*/
> -*supported = (status == 0);
> ret = 0;
> 
>  cleanup:
> -virCommandFree(cmd);
> +VIR_FREE(buf);
> +virStringFreeList(states);
> return ret;
> }
> 
> -- 
> 1.8.4.5
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

While systemd is used for a number of things there are other platforms than 
Linux and there are Linux platforms that don't use systemd. Can't we just wrap 
this in if not systemd?

--
Doug

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

Re: [libvirt] [Qemu-devel] [PATCH v5 for 2.0 1/3] only add qemu_tpmdev_opts when CONFIG_TPM is defined

[Markus Armbruster]

Amos Kong  writes:

> Signed-off-by: Amos Kong 
> ---
>  vl.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/vl.c b/vl.c
> index 2355227..596ecfa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -449,6 +449,7 @@ static QemuOptsList qemu_object_opts = {
>  },
>  };
>  
> +#ifdef CONFIG_TPM
>  static QemuOptsList qemu_tpmdev_opts = {
>  .name = "tpmdev",
>  .implied_opt_name = "type",
> @@ -458,6 +459,7 @@ static QemuOptsList qemu_tpmdev_opts = {
>  { /* end of list */ }
>  },
>  };
> +#endif
>  
>  static QemuOptsList qemu_realtime_opts = {
>  .name = "realtime",
> @@ -2992,7 +2994,9 @@ int main(int argc, char **argv, char **envp)
>  qemu_add_opts(&qemu_sandbox_opts);
>  qemu_add_opts(&qemu_add_fd_opts);
>  qemu_add_opts(&qemu_object_opts);
> +#ifdef CONFIG_TPM
>  qemu_add_opts(&qemu_tpmdev_opts);
> +#endif
>  qemu_add_opts(&qemu_realtime_opts);
>  qemu_add_opts(&qemu_msg_opts);
>  qemu_add_opts(&qemu_name_opts);

Making this conditional permits checking whether TPM is enabled via
QMP.  Mentioning that in the commit message wouldn't hurt.

Precedence for conditional adding: "iscsi" (CONFIG_LIBISCSI), "fsdev"
(CONFIG_VIRTFS), and possibly more.  But it's done differently there: we
call qemu_add_opts() from block/iscsi.c, fsdev/qemu-fsdev-opts.c.  Call
it from tpm.c?  Could be done as follow-up cleanup, if that helps
getting the fix into 2.0.

Related: "add-fd" is defined unconditionally, but works only #ifndef
_WIN32.  Should it be made conditional to permit querying via QMP?

Taking a step back: quite a few command line options make sense only in
certain build configurations.  We deal with that in several different
ways:

1. Target-specific options: qemu-options.hx declares a target mask.
   main() rejects options that don't apply to the target.

   Example: --no-acpi is only valid for QEMU_ARCH_I386.

2. Options specific to the host OS are recognized by
   os_parse_cmd_args().  Any of them not recognized by the host OS's
   os_parse_cmd_args() are silently ignored.  *boggle*

   Example: --runas is ignored by the Windows build.

3. Options depending on configuration are handled in (at least) three
   ways:

   a. The option is only recognized #ifdef CONFIG_FOO.  Which means it's
  silently ignored when FOO isn't enabled.  *boggle again*

  Example: --iscsi is ignored #ifndef CONFIG_LIBISCSI.

   b. The option is always recognized, but rejected when #ifndef
  CONFIG_FOO.

  Example: --curses is rejected #ifndef CONFIG_CURSES.

   c. The option is always recognized, but rejected when its
  QemuOptsList hasn't been registered.  Essentially just an #if-less
  way to do 3b.

  Example: --fsdev is rejected unless fsdev/qemu-fsdev-opts.c is
  compiled in.

In my opinion, silently ignoring an option is a bug until proven
otherwise.

Whether a non-sensical option should be recognized and rejected, or just
not recognized is debatable.

Regardless, telling QMP clients that an option works when it's always
rejected isn't useful.  We can either hide them in QMP, or add suitable
"invalid" markers, possibly identifying the reason.  Let's hide, unless
somebody can come up with a convincing use case for the additional
information.

For each of the cases above, how can we hide?  

1. Easy, check the target mask.

2. Turning "makes sense for host OS" from code into data we can check.

3a. Likewise.

3b. Likewise.

3c. If qemu-options.hx declares the QemuOptsList name, we can check
whether the named list exists.  Could also be used to factor the
qemu_opt_parse() out of the option switch.

We may not be able to get this wrapped in time for 2.0.  I'm not opposed
to a partial solution in 2.0, but let's think through the full solution,
to ensure the partial solution doesn't conflict with it.

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

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Richard W.M. Jones]

On Fri, Mar 28, 2014 at 08:55:05AM -0600, Mike Latimer wrote:
> On Friday, March 28, 2014 03:16:37 PM Guido Günther wrote:
> > On Fri, Mar 28, 2014 at 12:26:26PM +, Daniel P. Berrange wrote:
> > > A bunch of tests currently attempt to kickstart a full Fedora
> > > OS image install. Everytime I try to update this kickstart to
> > > a new version of Fedora it causes no end of pain. Switch the
> > > tests over to use Richard Jones' virt-builder command which
> > > is part of libguestfs. This makes it trivial to deploy and
> > > customize full OS images from pre-built templates.
> > > 
> > > Mike - does Suse include a new enough version of libguestfs
> > > to get access to the 'virt-builder' tool yet ?
> > 
> > Just as a data point: I'm running on Debian Wheezy via Jenkins on a
> > regular basis (i.e. on libvirt commit). Wheezy has 1.18.1 which isn't
> > recent enough. I also ran into kickstart hazzles several times but
> > keeping the entry barrier low to use libvirt-tck might be more
> > important.
> 
> I'm also running this through Jenkins on libvirt commits. I'm currently 
> running against SLES11 SP3, SLES12 (beta), and openSUSE 13.1. In the case of 
> SLES11, I don't have guestfs at all, so this change will be a problem there. 
> On the other hand, the move to virt-builder is likely worth it.
> 
> In my case, I will likely just create a local fork of libvirt-tck before this 
> commit that I use specifically for SLES11 testing. Another option is to 
> continue to use pre-built images (although I'd have to hide the libguestfs 
> requirement).

libguestfs exists on SUSE.  Olaf Hering is packaging it.  Not sure
about SLES, but I was under the impression it was packaged for at
least some version of SLES.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)

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

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Guido Günther]

On Fri, Mar 28, 2014 at 12:26:26PM +, Daniel P. Berrange wrote:
> A bunch of tests currently attempt to kickstart a full Fedora
> OS image install. Everytime I try to update this kickstart to
> a new version of Fedora it causes no end of pain. Switch the
> tests over to use Richard Jones' virt-builder command which
> is part of libguestfs. This makes it trivial to deploy and
> customize full OS images from pre-built templates.
> 
> Mike - does Suse include a new enough version of libguestfs
> to get access to the 'virt-builder' tool yet ?

Just as a data point: I'm running on Debian Wheezy via Jenkins on a
regular basis (i.e. on libvirt commit). Wheezy has 1.18.1 which isn't
recent enough. I also ran into kickstart hazzles several times but
keeping the entry barrier low to use libvirt-tck might be more
important.
Cheers,
 -- Guido


> 
> Daniel P. Berrange (6):
>   Add ability to provision full OS using virt-builder
>   Fix path to dnsmasq leases file
>   Add ability to setup NIC when creating guest XML
>   Force destroy guests if graceful shutdown fails
>   Convert nwfilter and domain balloon tests to virtbuilder images
>   Remove obsolete kickstart provisioning helper methods
> 
>  Build.PL  |   1 -
>  conf/default.cfg  |  25 +++
>  conf/ks.cfg   |  30 
>  lib/Sys/Virt/TCK.pm   | 280 
> +-
>  lib/Sys/Virt/TCK/DomainBuilder.pm |  20 ++-
>  lib/Sys/Virt/TCK/NetworkHelpers.pm| 140 +--
>  perl-Sys-Virt-TCK.spec.PL |   1 -
>  scripts/domain/110-memory-balloon.t   |  10 +-
>  scripts/nwfilter/090-install-image.t  |  55 --
>  scripts/nwfilter/100-ping-still-working.t |  54 +++---
>  scripts/nwfilter/210-no-mac-spoofing.t|  81 +
>  scripts/nwfilter/220-no-ip-spoofing.t |  78 +
>  scripts/nwfilter/230-no-mac-broadcast.t   |  53 +++---
>  scripts/nwfilter/240-no-arp-spoofing.t|  60 ---
>  scripts/nwfilter/300-vsitype.t|  40 +++--
>  scripts/nwfilter/nwfilter_concurrent.sh   |   4 +-
>  16 files changed, 493 insertions(+), 439 deletions(-)
>  delete mode 100644 conf/ks.cfg
>  delete mode 100644 scripts/nwfilter/090-install-image.t
> 
> -- 
> 1.8.5.3
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

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

Re: [libvirt] [PATCH v4 0/5] Expose FSFreeze/FSThaw within the guest as API

[Tomoki Sekiyama]

On 3/27/14 20:32 , "Eric Blake"  wrote:
>On 03/27/2014 05:54 PM, Tomoki Sekiyama wrote:
>> This sounds reasonable for me to add disks parameters.
>> I will try adding them in next version. Then the api will look like:
>> 
>> int virDomainFSFreeze(virDomainPtr dom, char** disks, int ndisks,
>>unsigned
>> int flags);
>>  and
>> int virDomainFSThaw(virDomainPtr dom, unsigned int flags);
>> 
>> 
>> I feel that thaw api shouldn't have disks parameter and thaw every
>>frozen
>> disk in order to simplify fsfreeze exclusion. Is it acceptable?
>
>Maybe, maybe not.  It means on a guest with 2 disks, I can't freeze one,
>then freeze the second - I can only have one atomic freeze at a time,
>and therefore thaw needs no parameters because it is always thawing the
>most recent freeze action.
>
>But we already know from painful experience that assuming at most one
>active job at a time doesn't scale well.  Maybe it should be:
>
>int virDomainFSFreeze(...char **disks...) returns a handle
>virDomainFSThaw(virDomainPtr dom, int handle, unsigned int flags);
>
>where FSThaw then thaws according to the handle returned by a freeze.
>That would allow me to do:
>
>interleaved subsets:
>freeze A => returns 1
>freeze B => returns 2
>thaw 1 => thaws A
>thaw 2 => thaws B
>
>nested subsets
>freeze A => returns 3
>freeze B => returns 4
>thaw 4 => thaws B
>thaw 3 => thaws A

What should happen when freezing the same disk is requested?
Currently it causes error. Another model can be counting freeze request
and defer thawing until thaw is requested the same time:

freeze A,B => returns 1  [counter A:1 B:1]
freeze A   => returns 2  [counter A:2 B:1]
thaw 1 => thaw B [counter A:1 B:0]
thaw 2 => thaw A [counter A:0 B:0]


And in qemu driver, until the issue about mapping disk name of guests and
libvirt is resolved, we could fallback to freeze every filesystem while
the counter > 0:

freeze A,B => returns 1 [counter 1] freeze every fs
freeze A   => returns 2 [counter 2]
thaw 1 =>   [counter 1]
thaw 2 =>   [counter 0] thaw every fs

Another aspect we need to consider is that freezing is done on filesystem
level, not on disk level. So if the filesystems lay on multiple disks
using LVM and so on, the situation will be more complex. Maybe the fs
should be frozen if a part of its disks are contained in the request,
although that wouldn't be useful for consistent snapshotting



>> 
>> BTW, in current qemu-kvm, disk device names in Linux guests don't seem
>>to always correspond to that is specified by libvirt (e.g. the first
>>virtio storage is named "vda" even if it is specified "vdz" in the
>>libvirt's xml).
>
>Correct.  Libvirt specifies destination names as a hint that must be
>unique to libvirt, but which has no bearing on the names used by the
>guest.
>
>> Until this is resolved, guest agent may ignore the disks parameter and
>>fallback to freezing all mounted filesystems. But still, having disks
>>parameter in the API level would be good for future extension.
>
>Hmm, interesting point.  Really, that means we want some way to map the
>strings understood by libvirt API into the disk names understood by the
>guest agent, and I don't know if we have such a means.  But I guess this
>is an issue even for the existing virDomainSnapshotCreateXML with the
>quiesce flag, so it's not a new problem.

Was there any discussion about this issue before?

Thanks,
Tomoki Sekiyama


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

[libvirt] [PATCH 7/n] conf: move storage secret type to util/

[Eric Blake]

This one is a relatively easy move.  We don't ever convert the
enum to or from strings (it is inferred from other elements in
the xml, rather than directly represented).

* src/conf/domain_conf.h (virDomainDiskSecretType): Move...
* src/util/virstoragefile.h (virStorageSecreteType): ...and
rename.
* src/conf/domain_conf.c (virDomainDiskSecretType): Drop unused
enum conversion.
(virDomainDiskAuthClear, virDomainDiskDefParseXML)
(virDomainDiskDefFormat): Adjust clients.
* src/qemu/qemu_command.c (qemuGetSecretString): Likewise.
* src/qemu/qemu_conf.c (qemuTranslateDiskSourcePoolAuth):
Likewise.

Signed-off-by: Eric Blake 
---
 src/conf/domain_conf.c| 19 +++
 src/conf/domain_conf.h| 10 +-
 src/qemu/qemu_command.c   |  8 
 src/qemu/qemu_conf.c  |  8 
 src/util/virstoragefile.h |  9 +
 5 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 643b066..a5afacf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -257,11 +257,6 @@ VIR_ENUM_IMPL(virDomainDiskErrorPolicy, 
VIR_DOMAIN_DISK_ERROR_POLICY_LAST,
   "ignore",
   "enospace")

-VIR_ENUM_IMPL(virDomainDiskSecretType, VIR_DOMAIN_DISK_SECRET_TYPE_LAST,
-  "none",
-  "uuid",
-  "usage")
-
 VIR_ENUM_IMPL(virDomainDiskIo, VIR_DOMAIN_DISK_IO_LAST,
   "default",
   "native",
@@ -1246,10 +1241,10 @@ virDomainDiskAuthClear(virDomainDiskSourceDefPtr def)
 {
 VIR_FREE(def->auth.username);

-if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE)
+if (def->auth.secretType == VIR_STORAGE_SECRET_TYPE_USAGE)
 VIR_FREE(def->auth.secret.usage);

-def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_NONE;
+def->auth.secretType = VIR_STORAGE_SECRET_TYPE_NONE;
 }


@@ -5349,7 +5344,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 goto error;
 }

-def->src.auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_NONE;
+def->src.auth.secretType = VIR_STORAGE_SECRET_TYPE_NONE;
 child = cur->children;
 while (child != NULL) {
 if (child->type == XML_ELEMENT_NODE &&
@@ -5386,7 +5381,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 }

 if (authUUID != NULL) {
-def->src.auth.secretType = 
VIR_DOMAIN_DISK_SECRET_TYPE_UUID;
+def->src.auth.secretType = 
VIR_STORAGE_SECRET_TYPE_UUID;
 if (virUUIDParse(authUUID,
  def->src.auth.secret.uuid) < 0) {
 virReportError(VIR_ERR_XML_ERROR,
@@ -5395,7 +5390,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 goto error;
 }
 } else if (authUsage != NULL) {
-def->src.auth.secretType = 
VIR_DOMAIN_DISK_SECRET_TYPE_USAGE;
+def->src.auth.secretType = 
VIR_STORAGE_SECRET_TYPE_USAGE;
 def->src.auth.secret.usage = authUsage;
 authUsage = NULL;
 }
@@ -14973,11 +14968,11 @@ virDomainDiskDefFormat(virBufferPtr buf,
 virBufferAddLit(buf, "src.auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_UUID) {
+if (def->src.auth.secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
 virUUIDFormat(def->src.auth.secret.uuid, uuidstr);
 virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr);
 }
-if (def->src.auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) {
+if (def->src.auth.secretType == VIR_STORAGE_SECRET_TYPE_USAGE) {
 virBufferEscapeString(buf, " usage='%s'/>\n",
   def->src.auth.secret.usage);
 }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e5d945f..52b59a1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -567,13 +567,6 @@ enum virDomainStartupPolicy {
 VIR_DOMAIN_STARTUP_POLICY_LAST
 };

-enum virDomainDiskSecretType {
-VIR_DOMAIN_DISK_SECRET_TYPE_NONE,
-VIR_DOMAIN_DISK_SECRET_TYPE_UUID,
-VIR_DOMAIN_DISK_SECRET_TYPE_USAGE,
-
-VIR_DOMAIN_DISK_SECRET_TYPE_LAST
-};

 enum virDomainDeviceSGIO {
 VIR_DOMAIN_DEVICE_SGIO_DEFAULT = 0,
@@ -617,7 +610,7 @@ struct _virDomainDiskSourceDef {
 virStorageSourcePoolDefPtr srcpool;
 struct {
 char *username;
-int secretType; /* enum virDomainDiskSecretType */
+int secretType; /* enum virStorageSecretType */
 union {
 unsigned char uuid[VIR_UUID_BUFLEN];
 char *usage;
@@ -2588,7 +2581,6 @@ VIR_ENUM_DECL(virDomainDiskBus)
 VIR_ENUM_DECL(virDomainDiskCache)
 VIR_ENUM_DECL(virDomainDiskErrorPoli

Re: [libvirt] [PATCH tck] 300-vsitype.t: skip earlier if lldptool is not available

[Mike Latimer]

On Friday, March 28, 2014 12:28:43 PM Daniel P. Berrange wrote:
> On Thu, Mar 27, 2014 at 04:12:16PM -0600, Mike Latimer wrote:
> > Move the test for /usr/sbin/lldptool up so libvirt-tck will report the
> > skip
> > and reason, rather than passing the test as 'ok'.
> 
> So we're talking about the difference between the test
> indicating "OK 3 out of 3 tests skipped" vs "SKIPPED" ?
> If so, that sounds like a nice improvement

Yes, but the real reason for the change is when you are running the full 
libvirt-tck suite. This patch changes the following output:

.../230-no-mac-broadcast.t .. ok
.../240-no-arp-spoofing.t ... ok
.../300-vsitype.t ... ok
.../100-disk-encryption.t ... ok
.../200-qcow2-single-backing-file.t . ok

To this:

.../230-no-mac-broadcast.t .. ok
.../240-no-arp-spoofing.t ... ok
.../300-vsitype.t ... skipped: lldptool is not available
.../100-disk-encryption.t ... ok
.../200-qcow2-single-backing-file.t . ok

-Mike

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

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Daniel P. Berrange]

On Fri, Mar 28, 2014 at 03:16:37PM +0100, Guido Günther wrote:
> On Fri, Mar 28, 2014 at 12:26:26PM +, Daniel P. Berrange wrote:
> > A bunch of tests currently attempt to kickstart a full Fedora
> > OS image install. Everytime I try to update this kickstart to
> > a new version of Fedora it causes no end of pain. Switch the
> > tests over to use Richard Jones' virt-builder command which
> > is part of libguestfs. This makes it trivial to deploy and
> > customize full OS images from pre-built templates.
> > 
> > Mike - does Suse include a new enough version of libguestfs
> > to get access to the 'virt-builder' tool yet ?
> 
> Just as a data point: I'm running on Debian Wheezy via Jenkins on a
> regular basis (i.e. on libvirt commit). Wheezy has 1.18.1 which isn't
> recent enough. I also ran into kickstart hazzles several times but
> keeping the entry barrier low to use libvirt-tck might be more
> important.

So if lack of virt-builder is a significant problem for you, then one
option is for us to directly download the disk images from

  http://libguestfs.org/download/builder/

and then just run virt-sysprep on it ourselves. We don't actually
need much of the fancy code virt-builder has, so it wouldn't be too
much work to do it.

How much longer do you expect to be needing to support running on
Wheezy ? Are we talking years, or just months ?

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH]execute netdev_del after receive DEVICE_DELETED event

[Laine Stump]

On 03/28/2014 01:30 PM, xiexiangyou wrote:
> Thanks for your reply.
>
> On 2014/3/27 22:14, Jiri Denemark wrote:
>
>> On Thu, Mar 27, 2014 at 20:51:24 +0800, x00221466 wrote:
>>> Hi,
>>>
>>> When live detaching the virtual net device, such as virtio nic、
>>> RTL8139、E1000, there are some problems:
>>>
>>> (1)If the Guest OS don't support the hot plugging pci device, detach
>>> the virtual network device by Libvirt, the "net device" in Qemu will
>>> still exist, but "hostnet"(tap) in Qemu will be removed. so the net device
>>> in Guest OS will be of no effect.
>>>
>>> (2)If reject the nic in Guest OS, Qemu will remove the "net device",
>>> then Qemu send DEVICE_DELETED to Libvirt, Libvirt receive the event
>>> in event-loop thread and release info of the net device in
>>> qemuDomainRemoveNetDevice func. but "hostnet" in Qemu still exist.
>>> So next live attaching virtual net device will be failed because of
>>> "Duplicate ID".
>>>
>>> #virsh attach-device win2008_st_r2_64 net.xml --live
>>> error: Failed to attach device from net.xml
>>> error: internal error: unable to execute QEMU command 'netdev_add':
>>> Duplicate ID 'hostnet0' for netdev
>>>
>>> (3)In addition, in qemuDomainDetachNetDevice, detach net device func,
>>> "netdev_del" command will be sent after sending "device_del" command
>>> at once. So it is violent to remove the tap device before the net device
>>> is completely removed.
>>>
>>> So I think it's more logical that doing the work of sending Qemu command
>>> "netdev_del" after receive the DEVICE_DELETED event. It can avoid the 
>>> conflict
>>> of device info between Libvirt side and Qemu side.
>> This sounds like it could be correct, although I'd prefer Laine to
>> express his opinion on this since he knows the corners in network device
>> assignment...
>>
>>> I create a thread in qemuDomainRemoveDevice,the handle of DEVICE_DELETED 
>>> event,
>>> to execute QEMU command "netdev_del".
>> Hmm, it took me some time to realize why you'd need to do this. It's
>> because qemuDomainRemoveDevice is run from a DEVICE_DELETED event
>> handler and thus it cannot talk back to the monitor, right? In that
>
> Yep! Sending the Qemu monitor command in event handler is no allowed, so I 
> create
> a new thread to do this.
>
>> case, I suggest spawning a thread for qemuDomainRemoveDevice itself
>> within the event handler (qemuProcessHandleDeviceDeleted) so that all
>> qemuDomainRemove* methods can talk to monitor if they need to.
>
> I will modify it as your suggest
>
>> To make the changes easier to follow, please do the change in two
>> patches. The first one to move qemuDomainRemoveDevice into a new thread
>> and the second one to move qemuMonitorRemoveNetdev and
>> qemuMonitorRemoveHostNetwork calls inside qemuDomainRemoveNetDevice.
>>
>> But first, wait for Laine's input, please.

Well, the level of my knowledge was that I noticed the problem caused by
the asynchronous nature of device_del (exactly the error message that
you're reporting) and reported this to QEMU, asking for an event to let
us know when it is okay to reuse a device ID (i.e. the DEVICE_DELETED
event). It appears that this isn't always good enough, though, so
*something* apparently needs to be done.

My understanding is that the problem is caused by the netdev_del being
executed too soon after device_del, and then the device ID is forever
lost due to the unclean "cleanup", is that correct? If so, then your
solution sounds correct.

But does netdev_del complete synchronously? If not, then we will also
need a completion event for that as well.

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

Re: [libvirt] [Qemu-devel] [PATCH v5 for 2.0 1/3] only add qemu_tpmdev_opts when CONFIG_TPM is defined

[Markus Armbruster]

Eric Blake  writes:

> On 03/28/2014 06:04 AM, Markus Armbruster wrote:
>> Amos Kong  writes:
>> 
>
>> Taking a step back: quite a few command line options make sense only in
>> certain build configurations.  We deal with that in several different
>> ways:
>> 
>> 1. Target-specific options: qemu-options.hx declares a target mask.
>>main() rejects options that don't apply to the target.
>> 
>>Example: --no-acpi is only valid for QEMU_ARCH_I386.
>
> Which means 'query-command-line-options' should not report 'no-acpi'
> except when built for i386 emulation.

Yes.

>> 2. Options specific to the host OS are recognized by
>>os_parse_cmd_args().  Any of them not recognized by the host OS's
>>os_parse_cmd_args() are silently ignored.  *boggle*
>> 
>>Example: --runas is ignored by the Windows build.
>
> Sounds like a bug.  Libvirt doesn't yet run qemu on a Windows build, but
> ideally, 'query-command-line-options' should not report 'runas' on a
> qemu binary built for windows.

Yes.

>> 3. Options depending on configuration are handled in (at least) three
>>ways:
>> 
>>a. The option is only recognized #ifdef CONFIG_FOO.  Which means it's
>>   silently ignored when FOO isn't enabled.  *boggle again*
>> 
>>   Example: --iscsi is ignored #ifndef CONFIG_LIBISCSI.
>
> Eww.  That's a bug - advertising a feature only to silently ignore it is
> not helpful.

Indeed.

>>b. The option is always recognized, but rejected when #ifndef
>>   CONFIG_FOO.
>> 
>>   Example: --curses is rejected #ifndef CONFIG_CURSES.
>
> Recognizing and rejecting with a nice message, vs. not recognizing and
> giving a generic 'unknown option' message, both have the same net
> effect.  It's more code to give the nice message, and is helpful to
> humans, but if the option is omitted from 'query-command-line-options',
> it makes no difference to libvirt.

Yes.

>>c. The option is always recognized, but rejected when its
>>   QemuOptsList hasn't been registered.  Essentially just an #if-less
>>   way to do 3b.
>> 
>>   Example: --fsdev is rejected unless fsdev/qemu-fsdev-opts.c is
>>   compiled in.
>> 
>> In my opinion, silently ignoring an option is a bug until proven
>> otherwise.
>
> Agreed.
>
>> 
>> Whether a non-sensical option should be recognized and rejected, or just
>> not recognized is debatable.
>
> Less code to not recognize it, you are then up to the question of
> whether the nicer error message warrants the extra code.
>
>> 
>> Regardless, telling QMP clients that an option works when it's always
>> rejected isn't useful.  We can either hide them in QMP, or add suitable
>> "invalid" markers, possibly identifying the reason.  Let's hide, unless
>> somebody can come up with a convincing use case for the additional
>> information.
>
> Agreed - hiding is nicer than having to expose yet more QMP details to
> mark an option that is "recognized but will never work".

Let's keep it simple until there's a clear need for fancy.

>> For each of the cases above, how can we hide?  
>> 
>> 1. Easy, check the target mask.
>> 
>> 2. Turning "makes sense for host OS" from code into data we can check.
>> 
>> 3a. Likewise.
>> 
>> 3b. Likewise.
>> 
>> 3c. If qemu-options.hx declares the QemuOptsList name, we can check
>> whether the named list exists.  Could also be used to factor the
>> qemu_opt_parse() out of the option switch.
>> 
>> We may not be able to get this wrapped in time for 2.0.  I'm not opposed
>> to a partial solution in 2.0, but let's think through the full solution,
>> to ensure the partial solution doesn't conflict with it.
>
> We're no worse than we were for 1.5 when query-command-line-options was
> first introduced - at this point, since we're not fixing a regression
> and since the bug is longstanding, it may be wiser to just leave 2.0
> as-is and save all the work for 2.1, rather than rushing in a partial
> fix for 2.0 only to have to redo it again later.

Let's aim for 2.1 then.

Amos, thanks for trying so hard to get it fixed in 2.0.  Us finding one
can of worms after the other is not your fault.

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

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Mike Latimer]

On Friday, March 28, 2014 02:32:28 PM Daniel P. Berrange wrote:
> I already made it configurable - just change the OS name in the config
> file to any that virt-builder supports.

Even better.  I'm very much liking what I'm seeing in the patches - 
specifically patch 4/6. I'll try to test it today (short day for me though).

-Mike

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

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Mike Latimer]

On Friday, March 28, 2014 05:34:19 PM Richard W.M. Jones wrote:
> On Fri, Mar 28, 2014 at 11:06:56AM -0600, Mike Latimer wrote:
> > ACK to all of this. For some reason, Fedora 20 didn't want to install for
> > me (it failed when resizing the disk, and I don't have time to chase it
> > today), but Fedora 19 did. Looks like there are no SUSE images in
> > virt-builder, so I'll work on adding those later.
> 
> On this point, you only need to publish a metadata file in the format
> described here:
> 
> http://libguestfs.org/virt-builder.1.html#sources-of-templates
> 
> See http://libguestfs.org/download/builder/index.asc for an example.
> 
> If you have cloud images already, it makes sense to point the metadata
> at those.

Thanks for the pointers. I saw your earlier messages about virt-builder, just 
never had a chance to dive into it. With Daniel's push, I now have an excuse 
to figure it out.  ;)

Thanks again,
Mike

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

Re: [libvirt] [Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

[Markus Armbruster]

Eric Blake  writes:

> On 03/28/2014 08:55 AM, Markus Armbruster wrote:
>> Amos Kong  writes:
>> 
>>> All the options are defined in qemu-options.hx. If we can't find a
>>> matched option definition by group name of option table, then the
>>> group name doesn't match with defined option name, it's not allowed
>>> from 2.0
>>>
>
>>> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list)
>>>  for (i = 0; i < entries; i++) {
>>>  if (vm_config_groups[i] == NULL) {
>>>  vm_config_groups[i] = list;
>>> +if (!opt_is_defined(list->name)) {
>>> +error_report("Didn't find a matched option definition, "
>>> + "group name (%s) of option table must match 
>>> with "
>>> + "defined option name (Since 2.0)", 
>>> list->name);
>>> +abort();
>>> +}
>>>  return;
>>>  }
>>>  }
>> 
>> Simple!  Wish it was my idea ;)
>> 
>> Why not simply assert(opt_is_defined(list->name))?
>
> Indeed, using assert() would also solve the problem of the error message
> being awkward.
>
>>>  
>>> -#define HAS_ARG 0x0001
>>> -
>
>>> -
>>> -static const QEMUOption qemu_options[] = {
>>> -{ "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
>>> -#define QEMU_OPTIONS_GENERATE_OPTIONS
>>> -#include "qemu-options-wrapper.h"
>>> -{ NULL },
>>> -};
>
>>>  
>>> +#undef HAS_ARG
>
> HAS_ARG is not very namespace clean.  Prior to your patch, it was used
> only in a single file (where we know it doesn't collide).  After your
> patch, it is now in a header used by multiple files.
>
>>> +
>>>  static gpointer malloc_and_trace(gsize n_bytes)
>>>  {
>>>  void *ptr = malloc(n_bytes);
>> 
>> Undefining HAS_ARG here, where it hasn't done any harm, while letting it
>> pollute every other compilation unit including qemu-options.h makes no
>> sense.
>
> Maybe a better approach would be to create an enum in qemu-options.h of
> actual flag values:
>
> typedef enum {
> QEMU_OPT_HAS_ARG = 1,
> } QEMUOptionFlags;
>
> and use QEMU_OPT_HAS_ARG instead of HAS_ARG in vl.c.  Additionally, you
> either have to s/HAS_ARG/QEMU_OPT_HAS_ARG/ throughout the .hx file, or
> you can take a shortcut in qemu-config.c:
>
> #define HAS_ARG QEMU_OPT_HAS_ARG
>
> const QEMUOption qemu_options[] = {
> { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> #define QEMU_OPTIONS_GENERATE_OPTIONS
> #include "qemu-options-wrapper.h"
> { NULL },
> };
>
> #undef HAS_ARG
>
> since that is the only place that includes the .hx file at a point where
> HAS_ARG has to be expanded to something useful.

Either something like this, or avoid moving this stuff to a header.  I
prefer the latter, and suggested how in my review.

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

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Mike Latimer]

On Friday, March 28, 2014 12:26:26 PM Daniel P. Berrange wrote:
> A bunch of tests currently attempt to kickstart a full Fedora
> OS image install. Everytime I try to update this kickstart to
> a new version of Fedora it causes no end of pain. Switch the
> tests over to use Richard Jones' virt-builder command which
> is part of libguestfs. This makes it trivial to deploy and
> customize full OS images from pre-built templates.
> 
> Mike - does Suse include a new enough version of libguestfs
> to get access to the 'virt-builder' tool yet ?
> 
> Daniel P. Berrange (6):
>   Add ability to provision full OS using virt-builder
>   Fix path to dnsmasq leases file
>   Add ability to setup NIC when creating guest XML
>   Force destroy guests if graceful shutdown fails
>   Convert nwfilter and domain balloon tests to virtbuilder images
>   Remove obsolete kickstart provisioning helper methods
> 
>  Build.PL  |   1 -
>  conf/default.cfg  |  25 +++
>  conf/ks.cfg   |  30 
>  lib/Sys/Virt/TCK.pm   | 280
> +- lib/Sys/Virt/TCK/DomainBuilder.pm | 
> 20 ++-
>  lib/Sys/Virt/TCK/NetworkHelpers.pm| 140 +--
>  perl-Sys-Virt-TCK.spec.PL |   1 -
>  scripts/domain/110-memory-balloon.t   |  10 +-
>  scripts/nwfilter/090-install-image.t  |  55 --
>  scripts/nwfilter/100-ping-still-working.t |  54 +++---
>  scripts/nwfilter/210-no-mac-spoofing.t|  81 +
>  scripts/nwfilter/220-no-ip-spoofing.t |  78 +
>  scripts/nwfilter/230-no-mac-broadcast.t   |  53 +++---
>  scripts/nwfilter/240-no-arp-spoofing.t|  60 ---
>  scripts/nwfilter/300-vsitype.t|  40 +++--
>  scripts/nwfilter/nwfilter_concurrent.sh   |   4 +-
>  16 files changed, 493 insertions(+), 439 deletions(-)
>  delete mode 100644 conf/ks.cfg
>  delete mode 100644 scripts/nwfilter/090-install-image.t

ACK to all of this. For some reason, Fedora 20 didn't want to install for me 
(it failed when resizing the disk, and I don't have time to chase it today), 
but Fedora 19 did. Looks like there are no SUSE images in virt-builder, so 
I'll work on adding those later.

I have a minor patch I'll submit next that will just allow me to drop one more 
local patch. If you don't want to worry about distros with older (or no) 
versions of libguestfs, then this patch series looks ready to go.

-Mike

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

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Richard W.M. Jones]

On Fri, Mar 28, 2014 at 11:06:56AM -0600, Mike Latimer wrote:
> ACK to all of this. For some reason, Fedora 20 didn't want to install for me 
> (it failed when resizing the disk, and I don't have time to chase it today), 
> but Fedora 19 did. Looks like there are no SUSE images in virt-builder, so 
> I'll work on adding those later.

On this point, you only need to publish a metadata file in the format
described here:

http://libguestfs.org/virt-builder.1.html#sources-of-templates

See http://libguestfs.org/download/builder/index.asc for an example.

If you have cloud images already, it makes sense to point the metadata
at those.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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

[libvirt] [PATCH tck] 100-ping-still-working.t: Replace vnet0 with mac address

[Mike Latimer]

Using a statically defined vnet0 may fail if multiple VMs are running on the
test machine. Switch to mac address, but replace '00' with '0' to match the
output of ebtables.

---
 scripts/nwfilter/100-ping-still-working.t | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/nwfilter/100-ping-still-working.t 
b/scripts/nwfilter/100-ping-still-working.t
index 1105d70..6c931ff 100644
--- a/scripts/nwfilter/100-ping-still-working.t
+++ b/scripts/nwfilter/100-ping-still-working.t
@@ -67,8 +67,8 @@ diag "ip is $guestip";
 my $ebtables = (-e '/sbin/ebtables') ? '/sbin/ebtables' : '/usr/sbin/ebtables';
 my $ebtable = `$ebtables -L;$ebtables -t nat -L`;
 diag $ebtable;
-# fixme to include mac adress
-ok($ebtable =~ "vnet0", "check ebtables entry");
+$mac =~ s/00/0/g;
+ok($ebtable =~ "$mac", "check ebtables entry");
 
 # ping guest1
 my $ping = `ping -c 10 $guestip`;
-- 
1.8.4.5

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

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Mike Latimer]

On Friday, March 28, 2014 05:31:13 PM Richard W.M. Jones wrote:
> libguestfs exists on SUSE.  Olaf Hering is packaging it.  Not sure
> about SLES, but I was under the impression it was packaged for at
> least some version of SLES.

Right. I thought he was working on some virt-builder images as well, but they 
don't seem to be out there. I'll get together with him and one of us will get 
them done.

Thanks,
Mike

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

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Richard W.M. Jones]

Sorry, just read your follow-up email ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

Re: [libvirt] [PATCH 0/3] Improve support for mocking APIs in tests

[Michal Privoznik]

On 21.03.2014 12:31, Daniel P. Berrange wrote:

This short series introduces some helpers for mocking tests,
provides a generic DBus mock helper using them and switches
the systemd test to use these helpers. This will remove much
code duplication in a later firewall test.

Daniel P. Berrange (3):
   Introduce a new set of helper macros for mocking symbols
   Create a re-usable DBus LD_PRELOAD mock library
   Switch systemd test to use generic dbus mock

  tests/Makefile.am  |  23 +++--
  tests/virmock.h| 264 +
  tests/virmockdbus.c|  64 
  tests/virsystemdmock.c | 132 -
  tests/virsystemdtest.c |  86 +++-
  5 files changed, 422 insertions(+), 147 deletions(-)
  create mode 100644 tests/virmock.h
  create mode 100644 tests/virmockdbus.c
  delete mode 100644 tests/virsystemdmock.c



ACK and safe to push.

Michal

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

[libvirt] [PATCH] Drop dependency on pm-is-supported

[Cédric Bosdonnat]

From: Cédric Bosdonnat 

pm-is-supported is the only thing needed in pm-utils, better get rid of
it since systemd is heavily used for libvirt.
---
 src/util/virnodesuspend.c | 34 --
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c
index 8088931..9839de2 100644
--- a/src/util/virnodesuspend.c
+++ b/src/util/virnodesuspend.c
@@ -29,6 +29,8 @@
 #include "viralloc.h"
 #include "virlog.h"
 #include "virerror.h"
+#include "virfile.h"
+#include "virstring.h"
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
@@ -263,41 +265,45 @@ int nodeSuspendForDuration(unsigned int target,
 static int
 virNodeSuspendSupportsTarget(unsigned int target, bool *supported)
 {
-virCommandPtr cmd;
-int status;
 int ret = -1;
+char *buf = NULL;
+char **states = NULL;
+bool canSuspend = false;
+bool canHibernate = false;
 
 if (virNodeSuspendInitialize() < 0)
 return -1;
 
 *supported = false;
 
+if (virFileReadAll("/sys/power/state", 1024, &buf) < 0)
+goto cleanup;
+
+states = virStringSplit(buf, " ", 0);
+
+canSuspend = (virStringArrayHasString(states, "mem") ||
+  virStringArrayHasString(states, "standby"));
+canHibernate = virStringArrayHasString(states, "disk");
+
 switch (target) {
 case VIR_NODE_SUSPEND_TARGET_MEM:
-cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL);
+*supported = canSuspend;
 break;
 case VIR_NODE_SUSPEND_TARGET_DISK:
-cmd = virCommandNewArgList("pm-is-supported", "--hibernate", NULL);
+*supported = canHibernate;
 break;
 case VIR_NODE_SUSPEND_TARGET_HYBRID:
-cmd = virCommandNewArgList("pm-is-supported", "--suspend-hybrid", 
NULL);
+*supported = canSuspend && canHibernate;
 break;
 default:
 return ret;
 }
 
-if (virCommandRun(cmd, &status) < 0)
-goto cleanup;
-
-   /*
-* Check return code of command == 0 for success
-* (i.e., the PM capability is supported)
-*/
-*supported = (status == 0);
 ret = 0;
 
  cleanup:
-virCommandFree(cmd);
+VIR_FREE(buf);
+virStringFreeList(states);
 return ret;
 }
 
-- 
1.8.4.5

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

[libvirt] [PATCH v2] Added example script on how to convert LXC container config

[Cédric Bosdonnat]

---
 Makefile.am  |   2 +-
 configure.ac |   1 +
 examples/lxcconvert/Makefile.am  |  18 ++
 examples/lxcconvert/virt-lxc-convert | 108 +++
 4 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 examples/lxcconvert/Makefile.am
 create mode 100644 examples/lxcconvert/virt-lxc-convert

diff --git a/Makefile.am b/Makefile.am
index 9847ff0..0ef983f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -23,7 +23,7 @@ SUBDIRS = . gnulib/lib include src daemon tools docs 
gnulib/tests \
   tests po examples/object-events examples/hellolibvirt \
   examples/dominfo examples/domsuspend examples/apparmor \
   examples/xml/nwfilter examples/openauth examples/systemtap \
-  tools/wireshark
+  examples/lxcconvert tools/wireshark
 
 ACLOCAL_AMFLAGS = -I m4
 
diff --git a/configure.ac b/configure.ac
index 73efffa..f84d4bb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2699,6 +2699,7 @@ AC_CONFIG_FILES([\
 examples/hellolibvirt/Makefile \
 examples/systemtap/Makefile \
 examples/xml/nwfilter/Makefile \
+examples/lxcconvert/Makefile \
 tools/wireshark/Makefile \
 tools/wireshark/src/Makefile])
 AC_OUTPUT
diff --git a/examples/lxcconvert/Makefile.am b/examples/lxcconvert/Makefile.am
new file mode 100644
index 000..09cf5d9
--- /dev/null
+++ b/examples/lxcconvert/Makefile.am
@@ -0,0 +1,18 @@
+## Copyright (C) 2014 SUSE LINUX Products GmbH, Nuernberg, Germany.
+##
+## This library is free software; you can redistribute it and/or
+## modify it under the terms of the GNU Lesser General Public
+## License as published by the Free Software Foundation; either
+## version 2.1 of the License, or (at your option) any later version.
+##
+## This library is distributed in the hope that it will be useful,
+## but WITHOUT ANY WARRANTY; without even the implied warranty of
+## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+## Lesser General Public License for more details.
+##
+## You should have received a copy of the GNU Lesser General Public
+## License along with this library.  If not, see
+## .
+
+EXTRA_DIST=\
+   virt-lxc-convert
diff --git a/examples/lxcconvert/virt-lxc-convert 
b/examples/lxcconvert/virt-lxc-convert
new file mode 100644
index 000..a6c5721
--- /dev/null
+++ b/examples/lxcconvert/virt-lxc-convert
@@ -0,0 +1,108 @@
+#!/bin/sh
+
+handler_cleanup()
+{
+if ! test -z "$conf_dir"; then
+# Remove the temporary config
+rm -r "$conf_dir"
+fi
+}
+trap handler_cleanup INT EXIT
+
+show_help()
+{
+cat << EOF
+$0 /path/to/lxc/config/file
+
+Wrapper around virsh domxml-from-native to ease conversion of LXC
+containers configuration to libvirt domain XML.
+EOF
+}
+
+if test $# != 1; then
+show_help
+exit 1
+fi
+
+if test "$1" = "--help" || test "$1" = "-h"; then
+show_help
+exit $?
+fi
+
+conf=$1
+
+conf_dir=$(mktemp --tmpdir -d virt-lxc-convert-XXX)
+conf_new=$conf_dir/config
+
+cp "$conf" "$conf_new"
+
+# Do we have lxc.mount, and is it pointing to a readable file?
+fstab=$(sed -n '/lxc.mount[[:space:]]*=/ s/[[:space:]]*=[[:space:]]*/=/p' \
+"$conf_new" | cut -f 2 -d '=')
+if test -n "$fstab" && test -r "$fstab"; then
+sed 's/^lxc.mount[[:space:]]*=.*$//' "$conf_new" >"${conf_new}.tmp"
+mv "${conf_new}.tmp" "${conf_new}"
+sed 's/^\([^#]\)/lxc.mount.entry = \1/' "$fstab" >>"${conf_new}"
+fi
+
+memory=$(free | sed -n '/Mem:/s/ \+/ /gp' | cut -f 2 -d ' ')
+default_tmpfs="size=$((memory/2))"
+
+# Do we have tmpfs without size param?
+lineno=0
+while read line; do
+lineno=$(expr $lineno + 1)
+has_rel_size=false
+case $line in
+lxc.mount.entry[[:space:]]*=[[:space:]]*tmpfs[[:space:]]*)
+is_tmpfs=true
+;;
+*)
+is_tmpfs=false
+;;
+esac
+
+# We only care about tmpfs mount entries here
+if ! $is_tmpfs; then
+continue
+fi
+
+case $line in
+*size=[0-9][0-9]*%*)
+has_rel_size=true
+has_size=true
+;;
+*size=*)
+has_size=true
+;;
+*)
+has_size=false
+;;
+esac
+
+# Add the default size here (50%) if no size is given
+if ! $has_size; then
+last_option_match="\([[:space:]]*[0-9][[:space:]]*[0-9][::space::]*$\)"
+sed "${lineno}s/$last_option_match/,$default_tmpfs\1/" \
+"$conf_new" >"${conf_new}.tmp"
+mv "${conf_new}.tmp" "${conf_new}"
+fi
+
+# Convert relative sizes
+if $has_rel_size; then
+percent=$(echo "$line" | sed 's/.*size=\([0-9][0-9]*\)%.*/\1/')
+size="$((memory*percent/100))"
+sed "${lineno}s/size=[0-9]*%/size=${size}/" \
+"$conf_new" >"${conf_new}.tmp"
+mv "${conf_new}.tmp" "${conf_new}"
+fi
+done < "$conf_new"
+
+# Do we have any

[libvirt] [PATCH tck] Find ebtables in /sbin or /usr/sbin

[Mike Latimer]

If ebtables binary is not found in /sbin, use /usr/sbin.

---
 scripts/nwfilter/100-ping-still-working.t | 3 ++-
 scripts/nwfilter/210-no-mac-spoofing.t| 3 ++-
 scripts/nwfilter/220-no-ip-spoofing.t | 3 ++-
 scripts/nwfilter/230-no-mac-broadcast.t   | 3 ++-
 scripts/nwfilter/240-no-arp-spoofing.t| 3 ++-
 5 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/scripts/nwfilter/100-ping-still-working.t 
b/scripts/nwfilter/100-ping-still-working.t
index 0bfdc00..1105d70 100644
--- a/scripts/nwfilter/100-ping-still-working.t
+++ b/scripts/nwfilter/100-ping-still-working.t
@@ -64,7 +64,8 @@ my $guestip = get_ip_from_leases($mac);
 diag "ip is $guestip";
 
 # check ebtables entry
-my $ebtable = `/sbin/ebtables -L;/sbin/ebtables -t nat -L`;
+my $ebtables = (-e '/sbin/ebtables') ? '/sbin/ebtables' : '/usr/sbin/ebtables';
+my $ebtable = `$ebtables -L;$ebtables -t nat -L`;
 diag $ebtable;
 # fixme to include mac adress
 ok($ebtable =~ "vnet0", "check ebtables entry");
diff --git a/scripts/nwfilter/210-no-mac-spoofing.t 
b/scripts/nwfilter/210-no-mac-spoofing.t
index fb20351..d7c57e6 100644
--- a/scripts/nwfilter/210-no-mac-spoofing.t
+++ b/scripts/nwfilter/210-no-mac-spoofing.t
@@ -64,7 +64,8 @@ my $guestip = get_ip_from_leases($mac);
 diag "ip is $guestip";
 
 # check ebtables entry
-my $ebtable = `/sbin/ebtables -L;/sbin/ebtables -t nat -L`;
+my $ebtables = (-e '/sbin/ebtables') ? '/sbin/ebtables' : '/usr/sbin/ebtables';
+my $ebtable = `$ebtables -L;$ebtables -t nat -L`;
 diag $ebtable;
 # ebtables shortens :00: to :0: so we need to do that too
 $_ = $mac;
diff --git a/scripts/nwfilter/220-no-ip-spoofing.t 
b/scripts/nwfilter/220-no-ip-spoofing.t
index 063bb5b..5e8e0d7 100644
--- a/scripts/nwfilter/220-no-ip-spoofing.t
+++ b/scripts/nwfilter/220-no-ip-spoofing.t
@@ -64,7 +64,8 @@ my $guestip = get_ip_from_leases($mac);
 diag "ip is $guestip";
 
 # check ebtables entry
-my $ebtable = `/sbin/ebtables -L;/sbin/ebtables -t nat -L`;
+my $ebtables = (-e '/sbin/ebtables') ? '/sbin/ebtables' : '/usr/sbin/ebtables';
+my $ebtable = `$ebtables -L;$ebtables -t nat -L`;
 diag $ebtable;
 # check if IP address is listed
 ok($ebtable =~ "$guestip", "check ebtables entry");
diff --git a/scripts/nwfilter/230-no-mac-broadcast.t 
b/scripts/nwfilter/230-no-mac-broadcast.t
index 09a758b..a7d3b8e 100644
--- a/scripts/nwfilter/230-no-mac-broadcast.t
+++ b/scripts/nwfilter/230-no-mac-broadcast.t
@@ -63,7 +63,8 @@ my $guestip = get_ip_from_leases($mac);
 diag "ip is $guestip";
 
 # check ebtables entry
-my $ebtable = `/sbin/ebtables -L;/sbin/ebtables -t nat -L`;
+my $ebtables = (-e '/sbin/ebtables') ? '/sbin/ebtables' : '/usr/sbin/ebtables';
+my $ebtable = `$ebtables -L;$ebtables -t nat -L`;
 diag $ebtable;
 # ebtables shortens :00: to :0: so we need to do that too
 $_ = $mac;
diff --git a/scripts/nwfilter/240-no-arp-spoofing.t 
b/scripts/nwfilter/240-no-arp-spoofing.t
index c31ea48..596a0ce 100644
--- a/scripts/nwfilter/240-no-arp-spoofing.t
+++ b/scripts/nwfilter/240-no-arp-spoofing.t
@@ -65,7 +65,8 @@ my $guestip = get_ip_from_leases($mac);
 diag "ip is $guestip";
 
 # check ebtables entry
-my $ebtable = `/sbin/ebtables -L;/sbin/ebtables -t nat -L`;
+my $ebtables = (-e '/sbin/ebtables') ? '/sbin/ebtables' : '/usr/sbin/ebtables';
+my $ebtable = `$ebtables -L;$ebtables -t nat -L`;
 diag $ebtable;
 # check if IP address is listed
 ok($ebtable =~ "$guestip", "check ebtables entry");
-- 
1.8.4.5

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

Re: [libvirt] [Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

[Eric Blake]

On 03/28/2014 08:55 AM, Markus Armbruster wrote:
> Amos Kong  writes:
> 
>> All the options are defined in qemu-options.hx. If we can't find a
>> matched option definition by group name of option table, then the
>> group name doesn't match with defined option name, it's not allowed
>> from 2.0
>>

>> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list)
>>  for (i = 0; i < entries; i++) {
>>  if (vm_config_groups[i] == NULL) {
>>  vm_config_groups[i] = list;
>> +if (!opt_is_defined(list->name)) {
>> +error_report("Didn't find a matched option definition, "
>> + "group name (%s) of option table must match 
>> with "
>> + "defined option name (Since 2.0)", list->name);
>> +abort();
>> +}
>>  return;
>>  }
>>  }
> 
> Simple!  Wish it was my idea ;)
> 
> Why not simply assert(opt_is_defined(list->name))?

Indeed, using assert() would also solve the problem of the error message
being awkward.

>>  
>> -#define HAS_ARG 0x0001
>> -

>> -
>> -static const QEMUOption qemu_options[] = {
>> -{ "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
>> -#define QEMU_OPTIONS_GENERATE_OPTIONS
>> -#include "qemu-options-wrapper.h"
>> -{ NULL },
>> -};

>>  
>> +#undef HAS_ARG

HAS_ARG is not very namespace clean.  Prior to your patch, it was used
only in a single file (where we know it doesn't collide).  After your
patch, it is now in a header used by multiple files.

>> +
>>  static gpointer malloc_and_trace(gsize n_bytes)
>>  {
>>  void *ptr = malloc(n_bytes);
> 
> Undefining HAS_ARG here, where it hasn't done any harm, while letting it
> pollute every other compilation unit including qemu-options.h makes no
> sense.

Maybe a better approach would be to create an enum in qemu-options.h of
actual flag values:

typedef enum {
QEMU_OPT_HAS_ARG = 1,
} QEMUOptionFlags;

and use QEMU_OPT_HAS_ARG instead of HAS_ARG in vl.c.  Additionally, you
either have to s/HAS_ARG/QEMU_OPT_HAS_ARG/ throughout the .hx file, or
you can take a shortcut in qemu-config.c:

#define HAS_ARG QEMU_OPT_HAS_ARG

const QEMUOption qemu_options[] = {
{ "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
#define QEMU_OPTIONS_GENERATE_OPTIONS
#include "qemu-options-wrapper.h"
{ NULL },
};

#undef HAS_ARG

since that is the only place that includes the .hx file at a point where
HAS_ARG has to be expanded to something useful.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Mike Latimer]

On Friday, March 28, 2014 03:16:37 PM Guido Günther wrote:
> On Fri, Mar 28, 2014 at 12:26:26PM +, Daniel P. Berrange wrote:
> > A bunch of tests currently attempt to kickstart a full Fedora
> > OS image install. Everytime I try to update this kickstart to
> > a new version of Fedora it causes no end of pain. Switch the
> > tests over to use Richard Jones' virt-builder command which
> > is part of libguestfs. This makes it trivial to deploy and
> > customize full OS images from pre-built templates.
> > 
> > Mike - does Suse include a new enough version of libguestfs
> > to get access to the 'virt-builder' tool yet ?
> 
> Just as a data point: I'm running on Debian Wheezy via Jenkins on a
> regular basis (i.e. on libvirt commit). Wheezy has 1.18.1 which isn't
> recent enough. I also ran into kickstart hazzles several times but
> keeping the entry barrier low to use libvirt-tck might be more
> important.

I'm also running this through Jenkins on libvirt commits. I'm currently 
running against SLES11 SP3, SLES12 (beta), and openSUSE 13.1. In the case of 
SLES11, I don't have guestfs at all, so this change will be a problem there. 
On the other hand, the move to virt-builder is likely worth it.

In my case, I will likely just create a local fork of libvirt-tck before this 
commit that I use specifically for SLES11 testing. Another option is to 
continue to use pre-built images (although I'd have to hide the libguestfs 
requirement).

-Mike

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

Re: [libvirt] [Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

[Markus Armbruster]

Amos Kong  writes:

> All the options are defined in qemu-options.hx. If we can't find a
> matched option definition by group name of option table, then the
> group name doesn't match with defined option name, it's not allowed
> from 2.0
>
> Signed-off-by: Amos Kong 
> ---
>  qemu-options.h | 12 
>  util/qemu-config.c | 28 
>  vl.c   | 19 ++-
>  3 files changed, 42 insertions(+), 17 deletions(-)
>
> diff --git a/qemu-options.h b/qemu-options.h
> index 89a009e..4024487 100644
> --- a/qemu-options.h
> +++ b/qemu-options.h
> @@ -28,9 +28,21 @@
>  #ifndef _QEMU_OPTIONS_H_
>  #define _QEMU_OPTIONS_H_
>  
> +#include "sysemu/arch_init.h"
> +
>  enum {
>  #define QEMU_OPTIONS_GENERATE_ENUM
>  #include "qemu-options-wrapper.h"
>  };
>  
> +#define HAS_ARG 0x0001
> +
> +typedef struct QEMUOption {
> +const char *name;
> +int flags;
> +int index;
> +uint32_t arch_mask;
> +} QEMUOption;
> +
> +extern const QEMUOption qemu_options[];
>  #endif

Instead of exposing struct QEMUOption and qemu_options[] here, why not
simply put opt_is_defined() in vl.c, and declare it in qemu-options.h?

> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index f610101..eba5428 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -6,6 +6,14 @@
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
>  #include "qmp-commands.h"
> +#include "qemu-options.h"
> +
> +const QEMUOption qemu_options[] = {
> +{ "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> +#define QEMU_OPTIONS_GENERATE_OPTIONS
> +#include "qemu-options-wrapper.h"
> +{ NULL },
> +};
>  
>  static QemuOptsList *vm_config_groups[32];
>  static QemuOptsList *drive_config_groups[4];
> @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list)
>  abort();
>  }
>  
> +/* check if the option is defined in qemu-options.hx */
> +static bool opt_is_defined(const char *name)
> +{
> +int i;
> +
> +for (i = 0; qemu_options[i].name; i++) {
> +if (!strcmp(qemu_options[i].name, name)) {
> +return true;
> +}
> +}
> +
> +return false;
> +}
> +

The same loop is hidden in lookup_opt().  We could avoid the duplication
by putting it in a function returning the option index.  That could
easily be a proper enumeration type, like this, in qemu-options.h:

typedef enum QEMUOption {
#define QEMU_OPTIONS_GENERATE_ENUM
#include "qemu-options-wrapper.h"
} QEMUOption;

QEMUOption qemu_option_lookup(const char *name);

plus a rename of vl.c's QEMUOption to QEMUOptionDesc or something.

Can be done on top.

>  void qemu_add_opts(QemuOptsList *list)
>  {
>  int entries, i;
> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list)
>  for (i = 0; i < entries; i++) {
>  if (vm_config_groups[i] == NULL) {
>  vm_config_groups[i] = list;
> +if (!opt_is_defined(list->name)) {
> +error_report("Didn't find a matched option definition, "
> + "group name (%s) of option table must match 
> with "
> + "defined option name (Since 2.0)", list->name);
> +abort();
> +}
>  return;
>  }
>  }

Simple!  Wish it was my idea ;)

Why not simply assert(opt_is_defined(list->name))?

> diff --git a/vl.c b/vl.c
> index 0464494..bd44c52 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -111,7 +111,6 @@ int main(int argc, char **argv)
>  #include "trace/control.h"
>  #include "qemu/queue.h"
>  #include "sysemu/cpus.h"
> -#include "sysemu/arch_init.h"
>  #include "qemu/osdep.h"
>  
>  #include "ui/qemu-spice.h"
> @@ -2082,22 +2081,6 @@ static void help(int exitcode)
>  exit(exitcode);
>  }
>  
> -#define HAS_ARG 0x0001
> -
> -typedef struct QEMUOption {
> -const char *name;
> -int flags;
> -int index;
> -uint32_t arch_mask;
> -} QEMUOption;
> -
> -static const QEMUOption qemu_options[] = {
> -{ "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> -#define QEMU_OPTIONS_GENERATE_OPTIONS
> -#include "qemu-options-wrapper.h"
> -{ NULL },
> -};
> -
>  static bool vga_available(void)
>  {
>  return object_class_by_name("VGA") || object_class_by_name("isa-vga");
> @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char 
> **argv,
>  return popt;
>  }
>  
> +#undef HAS_ARG
> +
>  static gpointer malloc_and_trace(gsize n_bytes)
>  {
>  void *ptr = malloc(n_bytes);

Undefining HAS_ARG here, where it hasn't done any harm, while letting it
pollute every other compilation unit including qemu-options.h makes no
sense.

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

Re: [libvirt] [Qemu-devel] [PATCH v5 for 2.0 1/3] only add qemu_tpmdev_opts when CONFIG_TPM is defined

[Eric Blake]

On 03/28/2014 06:04 AM, Markus Armbruster wrote:
> Amos Kong  writes:
> 

> Taking a step back: quite a few command line options make sense only in
> certain build configurations.  We deal with that in several different
> ways:
> 
> 1. Target-specific options: qemu-options.hx declares a target mask.
>main() rejects options that don't apply to the target.
> 
>Example: --no-acpi is only valid for QEMU_ARCH_I386.

Which means 'query-command-line-options' should not report 'no-acpi'
except when built for i386 emulation.

> 
> 2. Options specific to the host OS are recognized by
>os_parse_cmd_args().  Any of them not recognized by the host OS's
>os_parse_cmd_args() are silently ignored.  *boggle*
> 
>Example: --runas is ignored by the Windows build.

Sounds like a bug.  Libvirt doesn't yet run qemu on a Windows build, but
ideally, 'query-command-line-options' should not report 'runas' on a
qemu binary built for windows.

> 
> 3. Options depending on configuration are handled in (at least) three
>ways:
> 
>a. The option is only recognized #ifdef CONFIG_FOO.  Which means it's
>   silently ignored when FOO isn't enabled.  *boggle again*
> 
>   Example: --iscsi is ignored #ifndef CONFIG_LIBISCSI.

Eww.  That's a bug - advertising a feature only to silently ignore it is
not helpful.

> 
>b. The option is always recognized, but rejected when #ifndef
>   CONFIG_FOO.
> 
>   Example: --curses is rejected #ifndef CONFIG_CURSES.

Recognizing and rejecting with a nice message, vs. not recognizing and
giving a generic 'unknown option' message, both have the same net
effect.  It's more code to give the nice message, and is helpful to
humans, but if the option is omitted from 'query-command-line-options',
it makes no difference to libvirt.

> 
>c. The option is always recognized, but rejected when its
>   QemuOptsList hasn't been registered.  Essentially just an #if-less
>   way to do 3b.
> 
>   Example: --fsdev is rejected unless fsdev/qemu-fsdev-opts.c is
>   compiled in.
> 
> In my opinion, silently ignoring an option is a bug until proven
> otherwise.

Agreed.

> 
> Whether a non-sensical option should be recognized and rejected, or just
> not recognized is debatable.

Less code to not recognize it, you are then up to the question of
whether the nicer error message warrants the extra code.

> 
> Regardless, telling QMP clients that an option works when it's always
> rejected isn't useful.  We can either hide them in QMP, or add suitable
> "invalid" markers, possibly identifying the reason.  Let's hide, unless
> somebody can come up with a convincing use case for the additional
> information.

Agreed - hiding is nicer than having to expose yet more QMP details to
mark an option that is "recognized but will never work".

> 
> For each of the cases above, how can we hide?  
> 
> 1. Easy, check the target mask.
> 
> 2. Turning "makes sense for host OS" from code into data we can check.
> 
> 3a. Likewise.
> 
> 3b. Likewise.
> 
> 3c. If qemu-options.hx declares the QemuOptsList name, we can check
> whether the named list exists.  Could also be used to factor the
> qemu_opt_parse() out of the option switch.
> 
> We may not be able to get this wrapped in time for 2.0.  I'm not opposed
> to a partial solution in 2.0, but let's think through the full solution,
> to ensure the partial solution doesn't conflict with it.

We're no worse than we were for 1.5 when query-command-line-options was
first introduced - at this point, since we're not fixing a regression
and since the bug is longstanding, it may be wiser to just leave 2.0
as-is and save all the work for 2.1, rather than rushing in a partial
fix for 2.0 only to have to redo it again later.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Mike Latimer]

On Friday, March 28, 2014 12:26:26 PM Daniel P. Berrange wrote:
> A bunch of tests currently attempt to kickstart a full Fedora
> OS image install. Everytime I try to update this kickstart to
> a new version of Fedora it causes no end of pain. Switch the
> tests over to use Richard Jones' virt-builder command which
> is part of libguestfs. This makes it trivial to deploy and
> customize full OS images from pre-built templates.
> 
> Mike - does Suse include a new enough version of libguestfs
> to get access to the 'virt-builder' tool yet ?

Yes - in openSUSE 13.1 and the upcoming SLE-12 we do have virt-builder. You 
are completely right about these tests being rather painful. We have some 
local modifications that use autoyast to install a openSUSE 12.3 guest. This 
works, but is not without complication. Switching to an openSUSE 13.1 guest 
solves some issues and introduces others (just for the installation itself). 
For the machines I'm immediately testing with, I hacked through the issues and 
created a static 13.1 image that I deploy as part of my provisioning. ;)

I probably won't get to testing these changes today. If not, I'll go through 
them on Monday. (When I do, I'll think about adding some type of option to 
choose between guest types, rather than maintaining a local patch which 
replaces Fedora with SUSE.)

Thanks for doing this, btw. This was next on my list of things to deal with as 
well!

-Mike

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

Re: [libvirt] [Libguestfs] ANNOUNCE: libguestfs 1.26 released

[Richard W.M. Jones]

On Thu, Mar 27, 2014 at 10:26:42PM +, Richard W.M. Jones wrote:
> I'm pleased to announce libguestfs 1.26, a library and set of tools
> for accessing and modifying virtual machine disk images.  This release
> took more than 6 months of work by a considerable number of people,
> and has many new features (see release notes below).
> 
> You can get libguestfs 1.26 here:
> 
> Main website: http://libguestfs.org/
> 
>   Source: http://libguestfs.org/download/1.26-stable/
>   You will also need latest supermin from here:
>   http://libguestfs.org/download/supermin/
> 
> Fedora 20/21: 
> http://koji.fedoraproject.org/koji/packageinfo?packageID=8391
>   It will appear as an update for F20 in about a week.

Fedora 20 users can test and give feedback here:

https://admin.fedoraproject.org/updates/libguestfs-1.26.0-1.fc20,supermin-5.1.6-3.fc20

> Debian/experimental coming soon, see:
>   https://packages.debian.org/experimental/libguestfs0
> 
> The Fedora and Debian packages have split dependencies so you can
> download just the features you need.
> 
> From http://libguestfs.org/guestfs-release-notes.1.html :
> 
> RELEASE NOTES FOR LIBGUESTFS 1.26
> 
>  New features
> 
>   Tools
> 
> virt-customize(1) is a new tool for customizing virtual machine disk
> images. It lets you install packages, edit configuration files, run
> scripts, set passwords and so on. virt-builder(1) and virt-sysprep(1)
> use virt-customize, and command line options across all these tools are
> now identical.
> 
> virt-diff(1) is a new tool for showing the differences between the
> filesystems of two virtual machines. It is mainly useful when showing
> what files have been changed between snapshots.
> 
> virt-builder(1) has been greatly enhanced. There are many more ways to
> customize the virtual machine. It can pull templates from multiple
> repositories. A parallelized internal xzcat implementation speeds up
> template decompression. Virt-builder uses an optimizing planner to
> choose the fastest way to build the VM. It is now easier to use
> virt-builder from other programs. Internationalization support has been
> added to metadata. More efficient SELinux relabelling of files. Can
> build guests for multiple architectures. Error messages have been
> improved. (Pino Toscano)
> 
> virt-sparsify(1) has a new --in-place option. This sparsifies an image
> in place (without copying it) and is also much faster. (Lots of help
> provided by Paolo Bonzini)
> 
> virt-sysprep(1) can delete and scrub files under user control. You can
> lock user accounts or set random passwords on accounts. Can remove more
> log files. Can unsubscribe a guest from Red Hat Subscription Manager.
> New flexible way to enable and disable operations. (Wanlong Gao, Pino
> Toscano)
> 
> virt-win-reg(1) allows you to use URIs to specify remote disk images.
> 
> virt-format(1) can now pass the extra space that it recovers back to
> the host.
> 
> guestfish(1) has additional environment variables to give fine control
> over the > prompt. Guestfish reads its (rarely used) configuration
> file in a different order now so that local settings override global
> settings. (Pino Toscano)
> 
> virt-make-fs(1) was rewritten in C, but is unchanged in terms of
> functionality and command line usage.
> 
>   Language bindings
> 
> The OCaml bindings have a new Guestfs.Errno module, used to check the
> error number returned by Guestfs.last_errno.
> 
> PHP tests now work. (Pino Toscano)
> 
>   Inspection
> 
> Inspection can recognize Debian live images.
> 
>   Architectures
> 
> ARMv7 (32 bit) now supports KVM acceleration.
> 
> Aarch64 (ARM 64 bit) is supported, but the appliance part does not work
> yet.
> 
> PPC64 support has been fixed and enhanced.
> 
>  Security
> 
> Denial of service when inspecting disk images with corrupt btrfs
> volumes
> 
>   It was possible to crash libguestfs (and programs that use libguestfs
>   as a library) by presenting a disk image containing a corrupt btrfs
>   volume.
> 
>   This was caused by a NULL pointer dereference causing a denial of
>   service, and is not thought to be exploitable any further.
> 
>   See commit d70ceb4cbea165c960710576efac5a5716055486 for the fix. This
>   fix is included in libguestfs stable branches ≥ 1.26.0, ≥ 1.24.6 and
>   ≥ 1.22.8, and also in RHEL ≥ 7.0. Earlier versions of libguestfs are
>   not vulnerable.
> 
> Better generation of random root passwords and random seeds
> 
>   When generating random root passwords and random seeds, two bugs were
>   fixed which are possibly security related. Firstly we no longer read
>   excessive bytes from /dev/urandom (most of which were just thrown
>   away). Secondly we changed

Re: [libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Daniel P. Berrange]

On Fri, Mar 28, 2014 at 08:27:45AM -0600, Mike Latimer wrote:
> On Friday, March 28, 2014 12:26:26 PM Daniel P. Berrange wrote:
> > A bunch of tests currently attempt to kickstart a full Fedora
> > OS image install. Everytime I try to update this kickstart to
> > a new version of Fedora it causes no end of pain. Switch the
> > tests over to use Richard Jones' virt-builder command which
> > is part of libguestfs. This makes it trivial to deploy and
> > customize full OS images from pre-built templates.
> > 
> > Mike - does Suse include a new enough version of libguestfs
> > to get access to the 'virt-builder' tool yet ?
> 
> Yes - in openSUSE 13.1 and the upcoming SLE-12 we do have virt-builder. You 
> are completely right about these tests being rather painful. We have some 
> local modifications that use autoyast to install a openSUSE 12.3 guest. This 
> works, but is not without complication. Switching to an openSUSE 13.1 guest 
> solves some issues and introduces others (just for the installation itself). 
> For the machines I'm immediately testing with, I hacked through the issues 
> and 
> created a static 13.1 image that I deploy as part of my provisioning. ;)
> 
> I probably won't get to testing these changes today. If not, I'll go through 
> them on Monday. (When I do, I'll think about adding some type of option to 
> choose between guest types, rather than maintaining a local patch which 
> replaces Fedora with SUSE.)

I already made it configurable - just change the OS name in the config
file to any that virt-builder supports.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH tck] 300-vsitype.t: skip earlier if lldptool is not available

[Daniel P. Berrange]

On Fri, Mar 28, 2014 at 08:11:20AM -0600, Mike Latimer wrote:
> On Friday, March 28, 2014 12:28:43 PM Daniel P. Berrange wrote:
> > On Thu, Mar 27, 2014 at 04:12:16PM -0600, Mike Latimer wrote:
> > > Move the test for /usr/sbin/lldptool up so libvirt-tck will report the
> > > skip
> > > and reason, rather than passing the test as 'ok'.
> > 
> > So we're talking about the difference between the test
> > indicating "OK 3 out of 3 tests skipped" vs "SKIPPED" ?
> > If so, that sounds like a nice improvement
> 
> Yes, but the real reason for the change is when you are running the full 
> libvirt-tck suite. This patch changes the following output:
> 
> .../230-no-mac-broadcast.t .. ok
> .../240-no-arp-spoofing.t ... ok
> .../300-vsitype.t ... ok
> .../100-disk-encryption.t ... ok
> .../200-qcow2-single-backing-file.t . ok
> 
> To this:
> 
> .../230-no-mac-broadcast.t .. ok
> .../240-no-arp-spoofing.t ... ok
> .../300-vsitype.t ... skipped: lldptool is not available
> .../100-disk-encryption.t ... ok
> .../200-qcow2-single-backing-file.t . ok

Yep. that makes sense.  Will push this, and rebase mine change on
top of it.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 0/2] Fix virHostdev with session libvirt

[Ján Tomko]

On 03/28/2014 11:17 AM, Richard W.M. Jones wrote:
> On Fri, Mar 28, 2014 at 10:20:29AM +0100, Ján Tomko wrote:
>> Reported by Richard W.M. Jones:
>> https://www.redhat.com/archives/libvir-list/2014-March/msg01780.html
>>
>> Ján Tomko (2):
>>   Remove double free in virHostdevManagerDispose
>>   Create hostdevmgr in UserRuntimeDirectory for session libvirt
>>
>>  src/util/virhostdev.c | 42 +-
>>  1 file changed, 33 insertions(+), 9 deletions(-)
> 
> I applied these patches to libvirt, and the segfault went away.
> 
> Therefore: ACK.
> 

Thanks, pushed now.

Jan




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

[Leandro Dorileo]

Hi Amos,

On Thu, Mar 27, 2014 at 09:04:31PM +0800, Amos Kong wrote:
> All the options are defined in qemu-options.hx. If we can't find a
> matched option definition by group name of option table, then the
> group name doesn't match with defined option name, it's not allowed
> from 2.0
> 
> Signed-off-by: Amos Kong 
> ---
>  qemu-options.h | 12 
>  util/qemu-config.c | 28 
>  vl.c   | 19 ++-
>  3 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/qemu-options.h b/qemu-options.h
> index 89a009e..4024487 100644
> --- a/qemu-options.h
> +++ b/qemu-options.h
> @@ -28,9 +28,21 @@
>  #ifndef _QEMU_OPTIONS_H_
>  #define _QEMU_OPTIONS_H_
>  
> +#include "sysemu/arch_init.h"
> +
>  enum {
>  #define QEMU_OPTIONS_GENERATE_ENUM
>  #include "qemu-options-wrapper.h"
>  };
>  
> +#define HAS_ARG 0x0001
> +
> +typedef struct QEMUOption {
> +const char *name;
> +int flags;
> +int index;
> +uint32_t arch_mask;
> +} QEMUOption;
> +
> +extern const QEMUOption qemu_options[];
>  #endif
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index f610101..eba5428 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -6,6 +6,14 @@
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
>  #include "qmp-commands.h"
> +#include "qemu-options.h"
> +
> +const QEMUOption qemu_options[] = {
> +{ "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> +#define QEMU_OPTIONS_GENERATE_OPTIONS
> +#include "qemu-options-wrapper.h"
> +{ NULL },
> +};
>  
>  static QemuOptsList *vm_config_groups[32];
>  static QemuOptsList *drive_config_groups[4];
> @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list)
>  abort();
>  }
>  
> +/* check if the option is defined in qemu-options.hx */
> +static bool opt_is_defined(const char *name)
> +{
> +int i;
> +
> +for (i = 0; qemu_options[i].name; i++) {
> +if (!strcmp(qemu_options[i].name, name)) {
> +return true;
> +}
> +}
> +
> +return false;
> +}
> +
>  void qemu_add_opts(QemuOptsList *list)
>  {
>  int entries, i;
> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list)
>  for (i = 0; i < entries; i++) {
>  if (vm_config_groups[i] == NULL) {
>  vm_config_groups[i] = list;
> +if (!opt_is_defined(list->name)) {
> +error_report("Didn't find a matched option definition, "
> + "group name (%s) of option table must match 
> with "
> + "defined option name (Since 2.0)", list->name);


I'm not a native English speaker but I don't fell comfortable with this
message output, what about "option table's group name (%s) must match with
predefined option name (Since 2.0)"?

The patch looks good, I tested it and it works properly, so

Reviewed-by: Leandro Dorileo 



> +abort();
> +}
>  return;
>  }
>  }
> diff --git a/vl.c b/vl.c
> index 0464494..bd44c52 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -111,7 +111,6 @@ int main(int argc, char **argv)
>  #include "trace/control.h"
>  #include "qemu/queue.h"
>  #include "sysemu/cpus.h"
> -#include "sysemu/arch_init.h"
>  #include "qemu/osdep.h"
>  
>  #include "ui/qemu-spice.h"
> @@ -2082,22 +2081,6 @@ static void help(int exitcode)
>  exit(exitcode);
>  }
>  
> -#define HAS_ARG 0x0001
> -
> -typedef struct QEMUOption {
> -const char *name;
> -int flags;
> -int index;
> -uint32_t arch_mask;
> -} QEMUOption;
> -
> -static const QEMUOption qemu_options[] = {
> -{ "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> -#define QEMU_OPTIONS_GENERATE_OPTIONS
> -#include "qemu-options-wrapper.h"
> -{ NULL },
> -};
> -
>  static bool vga_available(void)
>  {
>  return object_class_by_name("VGA") || object_class_by_name("isa-vga");
> @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char 
> **argv,
>  return popt;
>  }
>  
> +#undef HAS_ARG
> +
>  static gpointer malloc_and_trace(gsize n_bytes)
>  {
>  void *ptr = malloc(n_bytes);
> -- 
> 1.8.5.3
> 
> 

-- 
Leandro Dorileo

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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Richard W.M. Jones]

On Fri, Mar 28, 2014 at 02:17:50PM +0200, Laine Stump wrote:
> On 03/28/2014 01:03 PM, Richard W.M. Jones wrote:
> > On Fri, Mar 28, 2014 at 12:16:43PM +0200, Laine Stump wrote:
> >> On 03/28/2014 10:47 AM, Richard W.M. Jones wrote:
> >>> On Fri, Mar 28, 2014 at 10:33:39AM +0200, Laine Stump wrote:
> >> * address should be mandatory
> > I don't understand why this should be mandatory.
[...]
> >  Actually I don't
> > think any of them should be mandatory (the same as qemu), but the only
> > one which libguestfs would specify is the network.  The guest can
> > choose any IP address on the network >= .5 and things will work.  The
> > libguestfs appliance arbitrarily uses .10 since DHCP would be too
> > slow.
> 
> But if the guest is going to choose an IP address itself rather than
> getting one from DHCP, it should know that it's choosing an address on
> the correct network, and I don't like the idea of depending on qemu's
> current choice of default network/host IP/dns IP to remain constant.

That's fair enough, although it seems unlikely it would change
especially as no one is really touching the SLIRP code beyond urgent
security fixes.

However I'm not clear if when you said "address should be mandatory"
you meant the network address or the guest DHCP start address.  It
seemed to me you meant the DHCP start address, but now I think perhaps
you meant the network address?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

[libvirt] [PATCH tck 5/6] Convert nwfilter and domain balloon tests to virtbuilder images

[Daniel P. Berrange]

Change tests which need a full OS image over to use the new
virtbuilder images instead of provisioning from a kickstart
file. This should make them much more reliable to run.

Signed-off-by: Daniel P. Berrange 
---
 scripts/domain/110-memory-balloon.t   | 10 ++--
 scripts/nwfilter/090-install-image.t  | 55 -
 scripts/nwfilter/100-ping-still-working.t | 54 ++---
 scripts/nwfilter/210-no-mac-spoofing.t| 81 +--
 scripts/nwfilter/220-no-ip-spoofing.t | 78 +
 scripts/nwfilter/230-no-mac-broadcast.t   | 53 +++-
 scripts/nwfilter/240-no-arp-spoofing.t| 60 +--
 scripts/nwfilter/300-vsitype.t| 40 ---
 scripts/nwfilter/nwfilter_concurrent.sh   |  4 +-
 9 files changed, 212 insertions(+), 223 deletions(-)
 delete mode 100644 scripts/nwfilter/090-install-image.t

diff --git a/scripts/domain/110-memory-balloon.t 
b/scripts/domain/110-memory-balloon.t
index 6d7df3a..1f90698 100644
--- a/scripts/domain/110-memory-balloon.t
+++ b/scripts/domain/110-memory-balloon.t
@@ -28,10 +28,9 @@ its value of current memory, max memory.
 use strict;
 use warnings;
 
-use Test::More tests => 15;
+use Test::More tests => 16;
 
 use Sys::Virt::TCK;
-use Sys::Virt::TCK::NetworkHelpers;
 use Test::Exception;
 use File::Spec::Functions qw(catfile catdir rootdir);
 
@@ -41,7 +40,6 @@ BAIL_OUT "failed to setup test harness: $@" if $@;
 END { $tck->cleanup if $tck; }
 
 diag "Define a new real domain, default memory is 1048576";
-my $dom_name ="tckmemballoon";
 my $default_mem = 1048576;
 my $max_mem1 = 1572864;
 my $max_mem2 = 1148576;
@@ -50,7 +48,9 @@ my $live_mem = 824288;
 my $current_mem = 724288;
 
 # Install a guest with default memory size
-my $dom = prepare_test_disk_and_vm($tck, $conn, $dom_name);
+my $xml = $tck->generic_domain(name => "tck", fullos => 1)->as_xml;
+my $dom;
+ok_domain(sub { $dom = $conn->define_domain($xml) }, "created persistent 
domain object");
 
 
 diag "Set max memory for inactive domain";
@@ -62,6 +62,8 @@ is($dom->get_max_memory(), $max_mem1, "Get max memory 
$max_mem1");
 diag "Start domain";
 $dom->create;
 ok($dom->get_id() > 0, "running domain has an ID > 0");
+
+diag "Waiting 30 seconds for guest to finish booting";
 sleep(30);
 
 diag "Get max memory for domain when domain is active";
diff --git a/scripts/nwfilter/090-install-image.t 
b/scripts/nwfilter/090-install-image.t
deleted file mode 100644
index 6fd8a0c..000
--- a/scripts/nwfilter/090-install-image.t
+++ /dev/null
@@ -1,55 +0,0 @@
-# -*- perl -*-
-#
-# Copyright (C) 2010 IBM Corp.
-#
-# This program is free software; You can redistribute it and/or modify
-# it under the GNU General Public License as published by the Free
-# Software Foundation; either version 2, or (at your option) any
-# later version
-#
-# The file "LICENSE" distributed along with this file provides full
-# details of the terms and conditions
-#
-
-=pod
-
-=head1 NAME
-
-network/000-install-image.t - install network test image
-
-=head1 DESCRIPTION
-
-The test case creates and install a 2GB fedora virtual 
-disk via kickstart file from the network.
-
-=cut
-
-use strict;
-use warnings;
-
-use Test::More tests => 1;
-
-use Sys::Virt::TCK;
-use Sys::Virt::TCK::NetworkHelpers;
-
-
-my $tck = Sys::Virt::TCK->new();
-my $conn = eval { $tck->setup(); };
-BAIL_OUT "failed to setup test harness: $@" if $@;
-END { $tck->cleanup if $tck; }
-
-use File::Spec::Functions qw(catfile catdir rootdir);
-
-# variables which may need to be adapted
-my $dom_name ="tcknwtest";
-
-my $testdom = prepare_test_disk_and_vm($tck, $conn, $dom_name);
-$testdom->create();
-ok($testdom->get_id() > 0, "running domain has an ID > 0");
-sleep(20);
-
-shutdown_vm_gracefully($testdom);
-
-exit 0;
-
-
diff --git a/scripts/nwfilter/100-ping-still-working.t 
b/scripts/nwfilter/100-ping-still-working.t
index a263cf9..0bfdc00 100644
--- a/scripts/nwfilter/100-ping-still-working.t
+++ b/scripts/nwfilter/100-ping-still-working.t
@@ -27,7 +27,7 @@ the host.
 use strict;
 use warnings;
 
-use Test::More tests => 3;
+use Test::More tests => 4;
 
 use Sys::Virt::TCK;
 use Sys::Virt::TCK::NetworkHelpers;
@@ -44,40 +44,38 @@ END {
 }
 
 # create first domain and start it
-diag "Trying domain lookup by name";
-my $dom1;
-my $dom_name ="tcknwtest";
-
-$dom1 = prepare_test_disk_and_vm($tck, $conn, $dom_name);
-$dom1->create();
-
-my $xml = $dom1->get_xml_description;
-diag $xml;
-ok($dom1->get_id() > 0, "running domain has an ID > 0");
-#my $mac1 = get_macaddress($xml);
-#diag $mac1;
-#my $result = xpath($dom1, "/domain/devices/interface/mac/\@address");
-#my @macaddrs = map { $_->getNodeValue} $result->get_nodelist;
-# we want the first mac
-#my $mac1 =  $macaddrs[0];
-my $mac1 =  get_first_macaddress($dom1);
-diag "mac is $mac1";
+my $xml = $tck->generic_domain(name => "tck", fullos => 1,
+  netmode => "network")->as_xml();

[libvirt] [PATCH tck 2/6] Fix path to dnsmasq leases file

[Daniel P. Berrange]

The tests run with the libvirt managed dnsmasq instance,
not the global dnsmasq instance.

Signed-off-by: Daniel P. Berrange 
---
 lib/Sys/Virt/TCK/NetworkHelpers.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Sys/Virt/TCK/NetworkHelpers.pm 
b/lib/Sys/Virt/TCK/NetworkHelpers.pm
index a2a8251..8bd3802 100644
--- a/lib/Sys/Virt/TCK/NetworkHelpers.pm
+++ b/lib/Sys/Virt/TCK/NetworkHelpers.pm
@@ -11,7 +11,7 @@ sub get_first_macaddress {
 
 sub get_ip_from_leases{
 my $mac = shift;
-my $tmp = `grep $mac /var/lib/dnsmasq/dnsmasq.leases`;
+my $tmp = `grep $mac /var/lib/libvirt/dnsmasq/default.leases`;
 my @fields = split(/ /, $tmp);
 my $ip = $fields[2];
 return $ip;
-- 
1.8.5.3

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

[libvirt] [PATCH tck 0/6] Fix tests which need a full OS image

[Daniel P. Berrange]

A bunch of tests currently attempt to kickstart a full Fedora
OS image install. Everytime I try to update this kickstart to
a new version of Fedora it causes no end of pain. Switch the
tests over to use Richard Jones' virt-builder command which
is part of libguestfs. This makes it trivial to deploy and
customize full OS images from pre-built templates.

Mike - does Suse include a new enough version of libguestfs
to get access to the 'virt-builder' tool yet ?

Daniel P. Berrange (6):
  Add ability to provision full OS using virt-builder
  Fix path to dnsmasq leases file
  Add ability to setup NIC when creating guest XML
  Force destroy guests if graceful shutdown fails
  Convert nwfilter and domain balloon tests to virtbuilder images
  Remove obsolete kickstart provisioning helper methods

 Build.PL  |   1 -
 conf/default.cfg  |  25 +++
 conf/ks.cfg   |  30 
 lib/Sys/Virt/TCK.pm   | 280 +-
 lib/Sys/Virt/TCK/DomainBuilder.pm |  20 ++-
 lib/Sys/Virt/TCK/NetworkHelpers.pm| 140 +--
 perl-Sys-Virt-TCK.spec.PL |   1 -
 scripts/domain/110-memory-balloon.t   |  10 +-
 scripts/nwfilter/090-install-image.t  |  55 --
 scripts/nwfilter/100-ping-still-working.t |  54 +++---
 scripts/nwfilter/210-no-mac-spoofing.t|  81 +
 scripts/nwfilter/220-no-ip-spoofing.t |  78 +
 scripts/nwfilter/230-no-mac-broadcast.t   |  53 +++---
 scripts/nwfilter/240-no-arp-spoofing.t|  60 ---
 scripts/nwfilter/300-vsitype.t|  40 +++--
 scripts/nwfilter/nwfilter_concurrent.sh   |   4 +-
 16 files changed, 493 insertions(+), 439 deletions(-)
 delete mode 100644 conf/ks.cfg
 delete mode 100644 scripts/nwfilter/090-install-image.t

-- 
1.8.5.3

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

[libvirt] [PATCH tck 6/6] Remove obsolete kickstart provisioning helper methods

[Daniel P. Berrange]

Remove the Sys::Virt::TCK::NetworkHelpers::prepare_test_disk_and_vm
method and its helpers. All tests now use the virtbuilder based
images.

Signed-off-by: Daniel P. Berrange 
---
 Build.PL   |   1 -
 conf/ks.cfg|  30 -
 lib/Sys/Virt/TCK/NetworkHelpers.pm | 132 -
 perl-Sys-Virt-TCK.spec.PL  |   1 -
 4 files changed, 164 deletions(-)
 delete mode 100644 conf/ks.cfg

diff --git a/Build.PL b/Build.PL
index e682c3a..50f7499 100644
--- a/Build.PL
+++ b/Build.PL
@@ -110,7 +110,6 @@ my $b = $class->new(
 },
 conf_files => {
'conf/default.cfg' => 'conf/default.cfg',
-   'conf/ks.cfg' => 'conf/ks.cfg',
 },
 PL_files => [ 'perl-Sys-Virt-TCK.spec.PL' ],
 );
diff --git a/conf/ks.cfg b/conf/ks.cfg
deleted file mode 100644
index b6269e9..000
--- a/conf/ks.cfg
+++ /dev/null
@@ -1,30 +0,0 @@
-install
-text
-url 
--url=http://ftp-stud.hs-esslingen.de/Mirrors/fedora.redhat.com/linux/releases/17/Fedora/i386/os/
-lang en_US.UTF-8
-keyboard de-latin1-nodeadkeys
-network --device eth0 --bootproto dhcp
-rootpw  --iscrypted 
$6$AHEMvpa2rx3n/DON$toWNA/ainpreIRC1g2L9yuil7bS.2hIf8DomTluFGulQtN3KstPeVrmwFMhkwhsW7ud7DANsWycGEL5ZOU50e.
-firewall --service=ssh
-authconfig --enableshadow --passalgo=sha512 --enablefingerprint
-selinux --enforcing
-timezone --utc Europe/Berlin
-bootloader --location=mbr --driveorder=vda --append=" LANG=en_US.UTF-8 
SYSFONT=latarcyrheb-sun16 KEYBOARDTYPE=pc KEYTABLE=de-latin1-nodeadkeys rhgb 
quiet"
-# The following is the partition information you requested
-# Note that any partitions you deleted are not expressed
-# here so unless you clear all partitions first, this is
-# not guaranteed to work
-clearpart --all --drives=vda --initlabel
-
-part /boot --fstype=ext4 --size=200
-part swap --grow --maxsize=256 --asprimary --size=1
-part / --fstype=ext3 --grow --size=200
-
-poweroff
-
-%packages
-@admin-tools
-@base
-@core
-@hardware-support
-%end
diff --git a/lib/Sys/Virt/TCK/NetworkHelpers.pm 
b/lib/Sys/Virt/TCK/NetworkHelpers.pm
index 5d19736..133064b 100644
--- a/lib/Sys/Virt/TCK/NetworkHelpers.pm
+++ b/lib/Sys/Virt/TCK/NetworkHelpers.pm
@@ -17,107 +17,7 @@ sub get_ip_from_leases{
 return $ip;
 }
 
-sub build_cdrom_ks_image {
-my $tck = shift;
 
-my $ks = $tck->config("ks");
-
-# Where we put the source files for the ISO
-my $bucket1 = "nwfilter-install-ks";
-# Where we put the ISO itself
-my $bucket2 = "nwfilter-install-iso";
-
-my $isoimage = catfile($tck->bucket_dir($bucket2), "boot.iso");
-
-unless (-e $isoimage) {
-   my $isofiledir = $tck->bucket_dir($bucket1);
-   my $ksfile = $tck->get_scratch_resource($ks, $bucket1, "ks.cfg");
-   my @progs = `which mkisofs genisoimage`;
-   chomp(@progs);
-
-   `$progs[0] -o "$isoimage" $isofiledir`;
-}
-
-return ($isoimage, "cdrom:/ks.cfg");
-}
-
-sub build_domain{
-my $tck = shift;
-my $domain_name = shift;
-my $mode = @_ ? shift : "bridge";
-
-my $guest;
-my $mac = "52:54:00:11:11:11";
-my $model = "virtio";
-#my $filterref = "no-spoofing";
-my $filterref = "clean-traffic";
-my $network = "network";
-my $source = "default";
-my $dev = "eth2";
-my $virtualport;
-
-my ($cdrom, $ksurl) = build_cdrom_ks_image($tck);
-
-my $guest = $tck->generic_domain(name => $domain_name);
-
-# change the type of network connection for 802.1Qbg tests
-if ($mode eq  "vepa") {
-   $network ="direct";
-   $virtualport = "802.1Qbg";
-   }
-
-# We want a bigger disk than normal
-$guest->rmdisk();
-my $diskpath = $tck->create_sparse_disk("nwfilter", "main.img", 5120);
-$guest->disk(src => $diskpath,
-dst => "vda",
-type=> "file");
-
-my $diskalloc = (stat $diskpath)[12];
-
-# No few blocks are allocated, then it likely hasn't been installed yet
-my $install = 0;
-if ($diskalloc < 10) {
-   $install = 1;
-   diag "Add cdrom";
-   $guest->disk(src => $cdrom, dst=>"hdc",
-type=> "file", device => "cdrom");
-   my $cmdline = "ip=dhcp gateway=192.168.122.1 ks=$ksurl";
-   $guest->boot_cmdline($cmdline);
-   $guest->interface(type => $network,
- source => $source,
- model => $model,
- mac => $mac);
-} else {
-   diag "Do normal boot";
-   $guest->clear_kernel_initrd_cmdline();
-   if ($mode eq "vepa") {
-   $guest->interface(type => $network,
- source => $source,
- model => $model,
- mac => $mac,
- dev => $dev,
- mode => $mode,
- virtualport => $virtualport);
-   } else {
-   $guest->interface(type => $network,
-

Re: [libvirt] [PATCH tck] 300-vsitype.t: skip earlier if lldptool is not available

[Daniel P. Berrange]

On Thu, Mar 27, 2014 at 04:12:16PM -0600, Mike Latimer wrote:
> Move the test for /usr/sbin/lldptool up so libvirt-tck will report the skip
> and reason, rather than passing the test as 'ok'.

So we're talking about the difference between the test
indicating "OK 3 out of 3 tests skipped" vs "SKIPPED" ?
If so, that sounds like a nice improvement 


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[libvirt] [PATCH tck 3/6] Add ability to setup NIC when creating guest XML

[Daniel P. Berrange]

Enhance the 'generic_domain' method to allow it to setup
guest NICs, defaulting to the default network, but also
allowing use of vepa to a host NIC.

Signed-off-by: Daniel P. Berrange 
---
 conf/default.cfg  |  6 ++
 lib/Sys/Virt/TCK.pm   | 39 ---
 lib/Sys/Virt/TCK/DomainBuilder.pm | 13 +++--
 3 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/conf/default.cfg b/conf/default.cfg
index 0da118c..209e40d 100644
--- a/conf/default.cfg
+++ b/conf/default.cfg
@@ -178,3 +178,9 @@ host_block_devices = (
 #  }
 # Can list more than on block device if many are available
 )
+
+# List of host NIC devices that the test suite can screw
+# around with for testing purposes
+host_network_devices = (
+#   eth0
+)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index 9981c0f..f8fa75d 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -851,6 +851,7 @@ sub generic_domain {
 my $name = exists $params{name} ? $params{name} : "tck";
 my $ostype = exists $params{ostype} ? $params{ostype} : "hvm";
 my $fullos = exists $params{fullos} ? $params{fullos} : 0;
+my $netmode = exists $params{netmode} ? $params{netmode} : undef;
 
 my $caps = Sys::Virt::TCK::Capabilities->new(xml => 
$self->conn->get_capabilities);
 
@@ -859,18 +860,35 @@ sub generic_domain {
 $container = $self->best_container_domain($caps)
unless $ostype && $ostype ne "exe";
 
+my $b;
 if ($container) {
die "Full provisioned OS not supported with containers yet" if $fullos;
 
-   return $self->generic_container_domain(name => $name,
-  caps => $caps,
-  domain => $container);
-} else {
-   return $self->generic_machine_domain(name => $name,
+   $b = $self->generic_container_domain(name => $name,
 caps => $caps,
-ostype => $ostype,
-fullos => $fullos);
+domain => $container);
+} else {
+   $b = $self->generic_machine_domain(name => $name,
+  caps => $caps,
+  ostype => $ostype,
+  fullos => $fullos);
+}
+if ($netmode) {
+   if ($netmode eq "vepa") {
+   $b->interface(type => "direct",
+ source => "default",
+ mac => "52:54:00:11:11:11",
+ dev => $self->get_host_network_device(),
+ mode => "vepa",
+ virtualport => "802.1Qbg");
+   } else {
+   $b->interface(type => "network",
+ source => "default",
+ mac => "52:54:00:11:11:11",
+ filterref => "clean-traffic");
+   }
 }
+return $b;
 }
 
 sub generic_pool {
@@ -1132,4 +1150,11 @@ sub get_host_block_device {
 return $match ? $device : undef;
 }
 
+sub get_host_network_device {
+my $self = shift;
+my $devindex = @_ ? shift : 0;
+
+return $self->config("host_network_devices/[$devindex]", undef);
+}
+
 1;
diff --git a/lib/Sys/Virt/TCK/DomainBuilder.pm 
b/lib/Sys/Virt/TCK/DomainBuilder.pm
index 5308dc9..7a20008 100644
--- a/lib/Sys/Virt/TCK/DomainBuilder.pm
+++ b/lib/Sys/Virt/TCK/DomainBuilder.pm
@@ -277,7 +277,6 @@ sub interface {
 
 die "type parameter is required" unless $params{type};
 die "source parameter is required" unless $params{source};
-die "model parameter is required" unless $params{model};
 
 push @{$self->{interfaces}}, \%params;
 
@@ -430,7 +429,7 @@ sub as_xml {
$w->emptyTag("mac",
 address =>  $interface->{mac});
 
-   if( $interface->{dev}) {
+   if ($interface->{dev}) {
$w->emptyTag("source",
 dev => $interface->{dev},
 mode => $interface->{mode});
@@ -438,7 +437,7 @@ sub as_xml {
$w->emptyTag("source",
 network => $interface->{source});
}
-   if( $interface->{virtualport}) {
+   if ($interface->{virtualport}) {
$w->startTag("virtualport",
 type => $interface->{virtualport});
$w->emptyTag("parameters",
@@ -448,9 +447,11 @@ sub as_xml {
 instanceid => '4000----');
$w->endTag("virtualport");
}
-   $w->emptyTag("model",
-type => $interface->{model});
-   if( $interface->{filterref}) {
+   if ($interface->{model}) {
+   $w->emptyTag("model",
+type => $interface->{model});
+   }
+   if ($interface->{filterref}) {
$w->emptyTag("filterref",
 

[libvirt] [PATCH tck 1/6] Add ability to provision full OS using virt-builder

[Daniel P. Berrange]

Some of the tests require a full operating system, not
merely a living dead zombie. Add the ability to bootstrap
a full OS image using the 'virt-builder' tool, defaulting
to 'fedora-20' OS image for x86_64.

Signed-off-by: Daniel P. Berrange 
---
 conf/default.cfg  |  19 +++
 lib/Sys/Virt/TCK.pm   | 245 --
 lib/Sys/Virt/TCK/DomainBuilder.pm |   7 +-
 3 files changed, 233 insertions(+), 38 deletions(-)

diff --git a/conf/default.cfg b/conf/default.cfg
index 143fba9..0da118c 100644
--- a/conf/default.cfg
+++ b/conf/default.cfg
@@ -40,6 +40,25 @@
 # over the interwebs
 ks = /etc/libvirt-tck/ks.cfg
 
+# Some of the scripts need to be able to login to the
+# guest OS images. The TCK will pick a random root
+# password, but if you want to login to the guest OS
+# images to debug things, then set a fixed password
+# here
+#rootpassword = 123456
+
+# List the virt-builder images to use for each arch
+images = (
+  {
+arch = x86_64
+ostype = (
+  hvm
+  xen
+)
+osname = fedora-20
+  }
+)
+
 #
 # Where the kernel/initrd files needed by tests are to be
 # found. These can be URLS or local file paths.
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index 57eb08c..9981c0f 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -67,6 +67,12 @@ sub new {
 return $self;
 }
 
+sub root_password {
+my $self = shift;
+
+return $self->{config}->get("rootpassword", "123456");
+}
+
 sub setup_conn {
 my $self = shift;
 my $uri = shift;
@@ -299,6 +305,7 @@ sub download_scratch {
 my $target = shift;
 my $uncompress = shift;
 
+print "# downloading $source\n";
 my $ua = LWP::UserAgent->new;
 $ua->timeout(10);
 $ua->env_proxy;
@@ -332,6 +339,7 @@ sub copy_scratch {
 my $target = shift;
 my $uncompress = shift;
 
+print "# copying $source\n";
 if (defined $uncompress) {
if ($uncompress eq "gzip") {
gunzip $source => $target;
@@ -366,6 +374,28 @@ sub create_sparse_disk {
 }
 
 
+sub create_virt_builder_disk {
+my $self = shift;
+my $bucket = shift;
+my $name = shift;
+my $osname = shift;
+
+my $dir = $self->bucket_dir($bucket);
+
+my $target = catfile($dir, $name);
+
+my $password = $self->root_password;
+
+if (-f $target) {
+   return $target;
+}
+
+print "# running virt-builder $osname\n";
+`virt-builder --root-password 'password:$password' --output $target 
$osname`;
+
+return $target;
+}
+
 sub create_empty_dir {
 my $self = shift;
 my $bucket = shift;
@@ -489,7 +519,31 @@ EOF
 return ($target, catfile(rootdir, "sbin", "init"));
 }
 
-sub match_kernel {
+sub best_domain {
+my $self = shift;
+my $caps = shift;
+my $ostype = shift;
+
+for (my $i = 0 ; $i < $caps->num_guests ; $i++) {
+   if ($caps->guest_os_type($i) eq $ostype &&
+   $caps->guest_arch_name($i) eq $caps->host_cpu_arch()) {
+
+   my @domains = $caps->guest_domain_types($i);
+   next unless int(@domains);
+
+   # Prefer kvm if multiple domain types are returned
+   my $domain = (grep /^kvm$/, @domains) ? "kvm" : $domains[0];
+
+   return ($domain,
+   $caps->host_cpu_arch());
+   }
+}
+
+return ();
+}
+
+
+sub match_guest_domain {
 my $self = shift;
 my $caps = shift;
 my $arch = shift;
@@ -521,12 +575,15 @@ sub best_kernel {
 my $wantostype = shift;
 
 my $kernels = $self->config("kernels", []);
+my $hostarch = $caps->host_cpu_arch();
 
 for (my $i = 0 ; $i <= $#{$kernels} ; $i++) {
my $arch = $kernels->[$i]->{arch};
my $ostype = $kernels->[$i]->{ostype};
my @ostype = ref($ostype) ? @{$ostype} : ($ostype);
 
+   next unless $arch eq $hostarch;
+
foreach $ostype (@ostype) {
if ((defined $wantostype) &&
($wantostype ne $ostype)) {
@@ -534,7 +591,7 @@ sub best_kernel {
}
 
my ($domain, $emulator, $loader) =
-   $self->match_kernel($caps, $arch, $ostype);
+   $self->match_guest_domain($caps, $arch, $ostype);
 
if (defined $domain) {
return ($i, $domain, $arch, $ostype, $emulator, $loader)
@@ -545,6 +602,64 @@ sub best_kernel {
 return ();
 }
 
+
+# Find an image matching the host arch and requested ostype
+sub best_image {
+my $self = shift;
+my $caps = shift;
+my $wantostype = shift;
+
+my $images = $self->config("images", []);
+my $hostarch = $caps->host_cpu_arch();
+
+for (my $i = 0 ; $i <= $#{$images} ; $i++) {
+   my $arch = $images->[$i]->{arch};
+   my $ostype = $images->[$i]->{ostype};
+   my @ostype = ref($ostype) ? @{$ostype} : ($ostype);
+
+   next unless $arch eq $hostarch;
+
+   foreach $ostype (@ostype) {
+   if ((defined $wantostype) &&
+   ($wantostype ne $ostype)) {
+   next;

[libvirt] [PATCH tck 4/6] Force destroy guests if graceful shutdown fails

[Daniel P. Berrange]

Change shutdown_vm_gracefully method so that it invokes
'destroy' if 'shutdown' has not completed after 30 seconds.

Signed-off-by: Daniel P. Berrange 
---
 lib/Sys/Virt/TCK/NetworkHelpers.pm | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/Sys/Virt/TCK/NetworkHelpers.pm 
b/lib/Sys/Virt/TCK/NetworkHelpers.pm
index 8bd3802..5d19736 100644
--- a/lib/Sys/Virt/TCK/NetworkHelpers.pm
+++ b/lib/Sys/Virt/TCK/NetworkHelpers.pm
@@ -118,13 +118,15 @@ sub build_domain{
 
 return ($guest, $install);
 }
-sub shutdown_vm_gracefully{
+sub shutdown_vm_gracefully {
 my $dom = shift;
 
+my $target = time() + 30;
 $dom->shutdown;
-while($dom->is_active()) {
+while ($dom->is_active()) {
sleep(1);
diag ".. waiting for virtual machine to shutdown.. ";
+   $dom->destroy() if time() > $target;
 }
 sleep(1);
 diag ".. shutdown complete.. ";
-- 
1.8.5.3

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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Laine Stump]

On 03/28/2014 01:03 PM, Richard W.M. Jones wrote:
> On Fri, Mar 28, 2014 at 12:16:43PM +0200, Laine Stump wrote:
>> On 03/28/2014 10:47 AM, Richard W.M. Jones wrote:
>>> On Fri, Mar 28, 2014 at 10:33:39AM +0200, Laine Stump wrote:
 Beyond that, a question not with your patch, but with qemu's
 implemenation - does it always assume that the gateway address is
 $network.1 ?
>>> Actually network.2.  The default addresses are:
>>>
>>>network: 10.0.2.0/24 (ie. mask 255.255.255.0)
>>>default gateway: 10.0.2.2
>>> dns server: 10.0.2.3
>>>  dhcp start / normal guest address: 10.0.2.15
>>>
>>> It _is_ possible to change the gateway address, by specifying the
>>> (confusingly named) 'host=' parameter.  As you suggested I think this
>>> could be mapped to a gateway XML attribute, although libguestfs would
>>> not need to use it.
>> Ah, good. I didn't see that in the parameters that I found (why is my
>> first reaction now to do a google search instead of looking at the man
>> page? What is wrong with me?? :-P).
>>
>> In that case, I think that the XML should be
>>
>>   >   gateway='host IP address according to guest'
>>   dns='DNS server IP given in DHCP response (and answering requests)'/>
>>
>> I think that
>>
>> * address should be mandatory
> I don't understand why this should be mandatory.

Well, if the  element is present, there should be *something* in it,
otherwise why bother? But I do see the point that someone might want to
specify the network (or rather the gateway), but not care which IP on
that network was used by the guest.

>  Actually I don't
> think any of them should be mandatory (the same as qemu), but the only
> one which libguestfs would specify is the network.  The guest can
> choose any IP address on the network >= .5 and things will work.  The
> libguestfs appliance arbitrarily uses .10 since DHCP would be too
> slow.

But if the guest is going to choose an IP address itself rather than
getting one from DHCP, it should know that it's choosing an address on
the correct network, and I don't like the idea of depending on qemu's
current choice of default network/host IP/dns IP to remain constant.
That's why I think that, even if the attributes aren't required (or
present) in the XML, we should define defaults for anything that could
affect the correctness of what *is* specified, and always fill them in
on the qemu commandline.


>> * prefix should default to the natural prefix for that particular
>> address (we have a function somewhere in the network conf or driver code
>> that already does this)
> Right, qemu does this if you don't specify it on the command line.
>
>> * gateway should default to
>>
>>   address & ~(0x<<(32-prefix)) + 1
> qemu will choose .2 unless specified.


Yes, currently that's what it will do, and it's not likely to change,
but it might. And if we support this config for a different network
implementation, their default probably *won't* be .2.


>
> [...]
>
> For libguestfs we only care about setting the network address + prefix,
> and to fix this bug I'm quite happy for the patch to only do that, and
> we can worry about the other things another time.

Sure, that's fine with me too, as long as we don't do it in a way that
paints us into a corner such that we *can't* add the other things in a
logical manner. (/me has gotten in the habit of considering patches as
one would consider moves in a chess game, which sometimes has the
unfortunate side effect of leading to gridlock)

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

Re: [libvirt] [Qemu-devel] [PATCH] migration: Fix possible bug for migrate cancel

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonz...@redhat.com) wrote:
> Il 28/03/2014 10:18, Gonglei (Arei) ha scritto:
> >>> > Can you please give more details at how you are triggering the problem
> >>> > with libvirt?  I think Paolo is probably right - the bug is more likely
> >>> > to be in libvirt not expecting the race and not recovering correctly
> >>> > when the race occurs, than it is to be in changing qemu's state 
> >>> > algorithm.
> >>> >
> >>When the migration progress reaches 100%, and the migration status becomes
> >>MIG_STATE_COMPLETED in Qemu.
> >>It will take some time which from MIG_STATE_COMPLETED to the migration
> >>thread resources are recovered.
> >>If we cancel the migration at this moment, the migrate_fd_cancel function 
> >>will
> >>break directly without reporting
> >>error code. Then, libvirt considers the cancle operation a success, contrary
> >>facts.
> 
> There is no error, once migration is completed you can still
> shutdown on the destination and continue on the source.  Libvirt
> should either:

(I've rewritten my reply below about 4 times - swinging between
different answers, this stuff really isn't obvious, and certainly
not documented)

I think I agree that it's not an error; but I think migrate_fd_cancel
knows what the outcome will be.

If it was MIG_STATE_ERROR on entry to migrate_fd_cancel, then yes it
could tell you that the cancel failed because you were already in error.

If it was MIG_STATE_COMPLETED on entry to migrate_fd_cancel, then yes it
could tell you that the cancel failed because you already finished.

If it was MIG_STATE_ACTIVE on entry to migrate_fd_cancel - it will go to
MIG_STATE_CANCELLING and I believe eventually to MIG_STATE_CANCELLED;
I don't believe it can get to MIG_STATE_ERROR from that point, since
all of the places in the migrate_thread that transition to error
do explicit ACTIVE->ERROR transitions.  I don't believe it can get to
MIG_STATE_COMPLETED for the same reason.

So migrate_fd_cancel knows that the eventual outcome will be Error
or Cancelled or completed, even if the state isn't there yet, and it
could reply to say that.

> 1) poll with "query-migrate" after migrate_cancel, and report an
> error there if it's the desired semantics;
> 2) toggle a "cancelled" flag before asking QEMU to cancel migration,
> check it in the migration functions after "query-migrate" reported
> completion; if it is true, do not resume on the destination.

I think you're right you have to poll with query-migrate until you
get one of cancelled/failed/completed.

However it's a bit odd; prior to the introduction of 'CANCELLING', the
state that you would get by a query-migrate after migrate_fd_cancel
returned would in principal be the state you ended up in - i.e.
cancelled/failed/completed.  With cancelling added, query-migrate
might lie to you and say 'active' (when it's really hiding the
fact that cancelling is happening).So while 'cancelling' apparently
didn't alter the API it did, in that query-migrate after a cancel
can now return active where it couldn't before.

> Another reason for doing it in libvirt is that the serialization
> between cancellation and completion of migration ultimately is
> controlled by libvirt's lock.  Doing this in QEMU makes it harder to
> reason about concurrency.

I think you have to be careful when you talk about 'cancellation and completion
of migration' - in that paragraph I don't think you mean the same thing
as MIG_STATE_CANCELLED and MIG_STATE_COMPLETED, I think you're talking
about the larger scale idea of completion after you take into account
that the VM might be paused after qemu has gone to MIG_STATE_COMPLETED and
libvirt might still decide it wants to give up and use the version on
the source that's still paused.

Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

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

Re: [libvirt] [Qemu-devel] [PATCH] migration: Fix possible bug for migrate cancel

[Paolo Bonzini]

Il 28/03/2014 12:30, Dr. David Alan Gilbert ha scritto:

> Another reason for doing it in libvirt is that the serialization
> between cancellation and completion of migration ultimately is
> controlled by libvirt's lock.  Doing this in QEMU makes it harder to
> reason about concurrency.

I think you have to be careful when you talk about 'cancellation and completion
of migration' - in that paragraph I don't think you mean the same thing
as MIG_STATE_CANCELLED and MIG_STATE_COMPLETED, I think you're talking
about the larger scale idea of completion after you take into account
that the VM might be paused after qemu has gone to MIG_STATE_COMPLETED and
libvirt might still decide it wants to give up and use the version on
the source that's still paused.


Yes, exactly.  This is why I considered the possibility of adding a 
"cancelled" flag within libvirt.


Libvirt always uses -S on the destination, so it's always possible to 
cancel migration even after MIG_STATE_COMPLETED.


Paolo

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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Laine Stump]

On 03/28/2014 12:26 PM, Richard W.M. Jones wrote:
> On Fri, Mar 28, 2014 at 08:47:48AM +, Richard W.M. Jones wrote:
>> On Fri, Mar 28, 2014 at 10:33:39AM +0200, Laine Stump wrote:
>>> Beyond that, a question not with your patch, but with qemu's
>>> implemenation - does it always assume that the gateway address is
>>> $network.1 ?
>> Actually network.2.  The default addresses are:
>>
>>network: 10.0.2.0/24 (ie. mask 255.255.255.0)
>>default gateway: 10.0.2.2
>> dns server: 10.0.2.3
>>  dhcp start / normal guest address: 10.0.2.15
>>
>> It _is_ possible to change the gateway address, by specifying the
>> (confusingly named) 'host=' parameter.  As you suggested I think this
>> could be mapped to a gateway XML attribute, although libguestfs would
>> not need to use it.
> Another couple of thoughts on this patch.
>
> (1) Qemu rejects impossible network configurations -- for example, if
> you specify a default gateway address which is outside the network
> address range.  However it does so without giving any specific error
> messages, see:
>
> http://git.qemu.org/?p=qemu.git;a=blob;f=net/slirp.c;h=cce026bf12bbead8a2bc8b5d0a1af67877266dd9;hb=HEAD#l208
>
> Is there a case for making libvirt do the same checks and give proper
> error messages (and/or should we fix qemu)?

Yes, I agree we should check for things like that. (We should also check
for that in the  config, as someone once pointed out in a BZ; I
just keep forgetting to do it)


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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Laine Stump]

On 03/28/2014 12:34 PM, Michal Privoznik wrote:
> On 28.03.2014 09:33, Laine Stump wrote:
>> On 03/27/2014 07:17 PM, Michal Privoznik wrote:
>>> We allow users to use SLIRP stack. However, there are some knobs
>>> which are not exposed to users, such as host network address, DNS
>>> server, smb, and others.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>   docs/formatdomain.html.in  |  7 +-
>>>   docs/schemas/domaincommon.rng  | 23 +-
>>>   src/conf/domain_conf.c | 88
>>> ++
>>>   src/conf/domain_conf.h |  6 ++
>>>   src/qemu/qemu_command.c| 19 +
>>>   .../qemuxml2argvdata/qemuxml2argv-net-user-ip.args |  7 ++
>>>   .../qemuxml2argvdata/qemuxml2argv-net-user-ip.xml  | 33 
>>>   tests/qemuxml2argvtest.c   |  1 +
>>>   8 files changed, 180 insertions(+), 4 deletions(-)
>>>   create mode 100644
>>> tests/qemuxml2argvdata/qemuxml2argv-net-user-ip.args
>>>   create mode 100644
>>> tests/qemuxml2argvdata/qemuxml2argv-net-user-ip.xml
>>
>> It is essential that this new  element be rejected, preferably at
>> parse time in the qemu-specific post-parse callback, for any interface
>> type that doesn't support it. Since it is the most obvious way of
>> specifying an IP address for a guest (and it is a way that has been
>> requested in the past) we are otherwise certain to have a lot of support
>> questions asking why the IP address setting isn't being used.
>
> Well, I think rejecting it goes against current policy 'silently
> ignore elements unknown to the libvirt'. But I can live with that here.

It's no longer unknown :-)

>
>>
>> Also, the attribute names seem confusing. The "address" attribute is the
>> address of the *network*, not of the interface, and "dhcpstart" is the
>> address of the interface. Even though qemu specifies the network address
>> and interface address separately, the network address is really
>> pointless, as it can/should be derived from the interface address.
>
> It is, so what XML schema do you propose? I'm not pleased with 
> either. But it was the best I could come up with so far. Maybe we are
> aiming for more structured XML here:
>
>   
> 
> 
> 
> 
> 
>   
>
> Or even more neting:
>
>   
> 
>   
>   
>   
> 

Interesting idea - a mini  inside the . If we were
going to go this far, it should be in order to replicate the schema of
 as much as possible though. Otherwise it could lead to confusion.

For that reason, I think just a single  element is fine, I just
think the attribute names should follow function rather than following
qemu (see my response to Rich). This will make it more likely that we
can add support for  on some other network type in the future. (For
example, I could see us possibly adding the capability to use the 
inside an interface to automatically populate the static IP list of a
libvirt network's dnsmasq instance before the domain is started, then
removing it once the domain is stopped - it would really only be
interested in the guest IP address, but would fail if any other info was
present and didn't match the network's config).

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

Re: [libvirt] [PATCH tck] Remove /128 from ip6tables output

[Daniel P. Berrange]

On Thu, Mar 27, 2014 at 05:55:06PM -0600, Mike Latimer wrote:
> Due to iptables commit 945353a2 (in iptables v1.4.20 and higher), ip6tables
> no longer prints out /128. This patch removes /128 from output files, and
> replaces '/128' in command output with '' to remain compatible with
> older versions of ip6tables.
> 
> ---
>  scripts/nwfilter/nwfilterxml2fwallout/ah-ipv6-test.fwall| 24 
> +++---
>  scripts/nwfilter/nwfilterxml2fwallout/all-ipv6-test.fwall   | 24 
> +++---
>  scripts/nwfilter/nwfilterxml2fwallout/comment-test.fwall| 12 
> +--
>  scripts/nwfilter/nwfilterxml2fwallout/esp-ipv6-test.fwall   | 24 
> +++---
>  scripts/nwfilter/nwfilterxml2fwallout/hex-data-test.fwall   | 12 
> +--
>  scripts/nwfilter/nwfilterxml2fwallout/icmpv6-test.fwall | 14 
> ++---
>  scripts/nwfilter/nwfilterxml2fwallout/sctp-ipv6-test.fwall  | 24 
> +++---
>  scripts/nwfilter/nwfilterxml2fwallout/tcp-ipv6-test.fwall   | 24 
> +++---
>  scripts/nwfilter/nwfilterxml2fwallout/udp-ipv6-test.fwall   | 24 
> +++---
>  scripts/nwfilter/nwfilterxml2fwallout/udplite-ipv6-test.fwall   | 24 
> +++---
>  10 files changed, 103 insertions(+), 103 deletions(-)

ACK and pushed


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[libvirt] [PATCH] Fix snapshot-create-as in man page

[shyu]

Fix bug https://bugzilla.redhat.com/show_bug.cgi?id=1080859 about
snapshot type in man page. Snapshot type should be no, internal, or
external
---
 tools/virsh.pod | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 20352cb..ba2da20 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3070,7 +3070,7 @@ is specified, the snapshot will not include vm state.
 The I<--memspec> option can be used to control whether a checkpoint
 is internal or external.  The I<--memspec> flag is mandatory, followed
 by a B of the form B<[file=]name[,snapshot=type]>, where
-type can be B, B, or B.  To include a literal
+type can be B, B, or B.  To include a literal
 comma in B, escape it with a second comma. I<--memspec> cannot
 be used together with I<--disk-only>.
 
-- 
1.8.5.3

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

Re: [libvirt] [PATCH tck] Adapt network tests to changed cli tool formats

[Daniel P. Berrange]

On Thu, Mar 27, 2014 at 02:52:31PM -0600, Mike Latimer wrote:
> On Thursday, March 27, 2014 02:49:25 PM Daniel P. Berrange wrote:
> > The network tests invoke various ifconfig and route commands
> > to test network setup, and also grep for dnsmasq/radvd args.
> > Switch to use 'ip' since ifconfig and route commands are not
> > installed by default on recent distros any more and their
> > output formats have also changed. Remove grepping for dnsmasq
> > args since libvirt uses a config file now too. Also avoid
> > looking for radvd, since we let dnsmasq handle IPv6 too now.
> > ---
> >  .../networks/networkxml2hostout/tck-testnet-1.dat  | 20 -
> >  .../networks/networkxml2hostout/tck-testnet-2.dat  | 16 +++
> >  .../networks/networkxml2hostout/tck-testnet-3.dat  | 51
> > +++--- 3 files changed, 33 insertions(+), 54 deletions(-)
> 
> ACK. This works in my environment and obsoletes a local patch I was using. 
> There are some differences between the two patches, but nothing critical. The 
> main difference is that my version greps through the tck-testnet.conf file to 
> validate the settings.
> 
> For example:
> 
> > diff --git a/scripts/networks/networkxml2hostout/tck-testnet-1.dat
> > b/scripts/networks/networkxml2hostout/tck-testnet-1.dat index
> ...
> > -#ps aux | sed -n '/dnsmasq .*tck-testnet/ s|.*\(listen-address
> > 10\.1\.2\.1*\).*|\1|p'
> > -listen-address 10.1.2.1
> 
> Instead of eliminating the above test, I checked for the bind-dynamic setting 
> in the conf file as follows:
> #grep bind-dynamic `ps aux | sed -n '0,/dnsmasq .*tck-testnet/ s|.*--conf-
> file=\(.*tck-testnet.conf\).*|\1|p'`
> bind-dynamic
> 
> > -#ps aux | sed -n '/dnsmasq .*tck-testnet/ s|.*\(dhcp-range
> > 10\.1\.2\.2\,10\.1\.2\.254*\).*|\1|p'
> > -dhcp-range 10.1.2.2,10.1.2.254
> 
> I used the same approach here also, only grepping for 'dhcp-range'.:
> #grep dhcp-range `ps aux | sed -n '0,/dnsmasq .*tck-testnet/ s|.*--conf-
> file=\(.*tck-testnet.conf\).*|\1|p'`
> dhcp-range=10.1.2.2,10.1.2.254
> 
> The above two checks exist in all three .dat files. The test results match 
> across the three test until the final dhcp-range check, where the test 
> produces 
> the following results (in my environment):
> 
> #grep dhcp-range `ps aux | sed -n '0,/dnsmasq .*tck-testnet/ s|.*--conf-
> file=\(.*tck-testnet.conf\).*|\1|p'`
> dhcp-range=10.1.2.2,10.1.2.254
> dhcp-range=2001:db8:ac10:fe01::1,ra-only
> dhcp-range=2001:db8:ac10:fd01::1,ra-only
> 
> Is there any reason to include these checks, or is it safe to just verify the 
> existence of the conf file itself?

Yes, you are right. I was being lazy by just deleting this stuff. I'll
add your examples into the patch before pushing.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH]execute netdev_del after receive DEVICE_DELETED event

[xiexiangyou]

Thanks for your reply.

On 2014/3/27 22:14, Jiri Denemark wrote:

> On Thu, Mar 27, 2014 at 20:51:24 +0800, x00221466 wrote:
>> Hi,
>>
>> When live detaching the virtual net device, such as virtio nic、
>> RTL8139、E1000, there are some problems:
>>
>> (1)If the Guest OS don't support the hot plugging pci device, detach
>> the virtual network device by Libvirt, the "net device" in Qemu will
>> still exist, but "hostnet"(tap) in Qemu will be removed. so the net device
>> in Guest OS will be of no effect.
>>
>> (2)If reject the nic in Guest OS, Qemu will remove the "net device",
>> then Qemu send DEVICE_DELETED to Libvirt, Libvirt receive the event
>> in event-loop thread and release info of the net device in
>> qemuDomainRemoveNetDevice func. but "hostnet" in Qemu still exist.
>> So next live attaching virtual net device will be failed because of
>> "Duplicate ID".
>>
>> #virsh attach-device win2008_st_r2_64 net.xml --live
>> error: Failed to attach device from net.xml
>> error: internal error: unable to execute QEMU command 'netdev_add':
>> Duplicate ID 'hostnet0' for netdev
>>
>> (3)In addition, in qemuDomainDetachNetDevice, detach net device func,
>> "netdev_del" command will be sent after sending "device_del" command
>> at once. So it is violent to remove the tap device before the net device
>> is completely removed.
>>
>> So I think it's more logical that doing the work of sending Qemu command
>> "netdev_del" after receive the DEVICE_DELETED event. It can avoid the 
>> conflict
>> of device info between Libvirt side and Qemu side.
> 
> This sounds like it could be correct, although I'd prefer Laine to
> express his opinion on this since he knows the corners in network device
> assignment...
> 
>> I create a thread in qemuDomainRemoveDevice,the handle of DEVICE_DELETED 
>> event,
>> to execute QEMU command "netdev_del".
> 
> Hmm, it took me some time to realize why you'd need to do this. It's
> because qemuDomainRemoveDevice is run from a DEVICE_DELETED event
> handler and thus it cannot talk back to the monitor, right? In that


Yep! Sending the Qemu monitor command in event handler is no allowed, so I 
create
a new thread to do this.

> case, I suggest spawning a thread for qemuDomainRemoveDevice itself
> within the event handler (qemuProcessHandleDeviceDeleted) so that all
> qemuDomainRemove* methods can talk to monitor if they need to.


I will modify it as your suggest

> 
> To make the changes easier to follow, please do the change in two
> patches. The first one to move qemuDomainRemoveDevice into a new thread
> and the second one to move qemuMonitorRemoveNetdev and
> qemuMonitorRemoveHostNetwork calls inside qemuDomainRemoveNetDevice.
> 
> But first, wait for Laine's input, please.


Regards,
-xie


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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Daniel P. Berrange]

On Thu, Mar 27, 2014 at 11:55:45AM -0600, Eric Blake wrote:
> On 03/27/2014 11:17 AM, Michal Privoznik wrote:
> > We allow users to use SLIRP stack. However, there are some knobs
> > which are not exposed to users, such as host network address, DNS
> > server, smb, and others.
> > 
> > Signed-off-by: Michal Privoznik 
> > ---
> >  docs/formatdomain.html.in  |  7 +-
> >  docs/schemas/domaincommon.rng  | 23 +-
> >  src/conf/domain_conf.c | 88 
> > ++
> >  src/conf/domain_conf.h |  6 ++
> >  src/qemu/qemu_command.c| 19 +
> >  .../qemuxml2argvdata/qemuxml2argv-net-user-ip.args |  7 ++
> >  .../qemuxml2argvdata/qemuxml2argv-net-user-ip.xml  | 33 
> >  tests/qemuxml2argvtest.c   |  1 +
> >  8 files changed, 180 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-user-ip.args
> >  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-user-ip.xml
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 7f90455..0a353ca 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -3225,7 +3225,11 @@
> >starting from 10.0.2.15. The default router will be
> >10.0.2.2 and the DNS server will be 
> > 10.0.2.3.
> >This networking is the only option for unprivileged users who need 
> > their
> > -  VMs to have outgoing access.
> > +  VMs to have outgoing access. Since 1.2.3 
> > the
> 
> We've already frozen for 1.2.3, and this feels like new features rather
> than bug fix.  Is it better to wait until after the release?

Given the ongoing comments, I think 1.2.4 is most suitable at this time.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Richard W.M. Jones]

On Fri, Mar 28, 2014 at 12:16:43PM +0200, Laine Stump wrote:
> On 03/28/2014 10:47 AM, Richard W.M. Jones wrote:
> > On Fri, Mar 28, 2014 at 10:33:39AM +0200, Laine Stump wrote:
> >> Beyond that, a question not with your patch, but with qemu's
> >> implemenation - does it always assume that the gateway address is
> >> $network.1 ?
> > Actually network.2.  The default addresses are:
> >
> >network: 10.0.2.0/24 (ie. mask 255.255.255.0)
> >default gateway: 10.0.2.2
> > dns server: 10.0.2.3
> >  dhcp start / normal guest address: 10.0.2.15
> >
> > It _is_ possible to change the gateway address, by specifying the
> > (confusingly named) 'host=' parameter.  As you suggested I think this
> > could be mapped to a gateway XML attribute, although libguestfs would
> > not need to use it.
> 
> Ah, good. I didn't see that in the parameters that I found (why is my
> first reaction now to do a google search instead of looking at the man
> page? What is wrong with me?? :-P).
> 
> In that case, I think that the XML should be
> 
>  gateway='host IP address according to guest'
>   dns='DNS server IP given in DHCP response (and answering requests)'/>
> 
> I think that
> 
> * address should be mandatory

I don't understand why this should be mandatory.  Actually I don't
think any of them should be mandatory (the same as qemu), but the only
one which libguestfs would specify is the network.  The guest can
choose any IP address on the network >= .5 and things will work.  The
libguestfs appliance arbitrarily uses .10 since DHCP would be too
slow.

> * prefix should default to the natural prefix for that particular
> address (we have a function somewhere in the network conf or driver code
> that already does this)

Right, qemu does this if you don't specify it on the command line.

> * gateway should default to
> 
>   address & ~(0x<<(32-prefix)) + 1

qemu will choose .2 unless specified.

[...]

For libguestfs we only care about setting the network address + prefix,
and to fix this bug I'm quite happy for the patch to only do that, and
we can worry about the other things another time.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

Re: [libvirt] Device attach / detach problem for passthrough/SR-IOV in libvirt

[Mohsen Ghaemi]

Hi

thanks for your reply

Since I am working on my master thesis and seriously I am running out of
time I decided to downgrade my libvirt from 1.1.1 to 1.0.2 to be able to
continue working on my thesis. Now it works !

But for your information , I already checked the XML file and the interface was
really removed. I checked my host interfaces using ifconfig -a and I found
however I released the device but it is not there and it is not possible to
assign it to other VM so I though it means there is some missed procedure
or some thing like this to unbind pci device ! as i mentioned if I
restarted the libvirt service I was able to attach it again but still it
was not shown in ifconfig -a list. The other point that i noticed was the
number of "irq"s in my logs for MSI/MSI-X while attaching device. I am not
a programmer and I have no clue about interrupts but the point was that in
1.1.1 i cloud see only 2 or 3 "irq" in my logs but now I see 7 or 8 .

regards
Mohsen



On Fri, Mar 28, 2014 at 10:34 AM, Michal Privoznik wrote:

> On 27.03.2014 13:53, Mohsen Ghaemi wrote:
>
>> Hi all
>>
>> I just encountered a problem in libvirt while was trying to attach and
>> detach a sr-iov vf to my VM
>>
>>
>> The version of my libvirt is 1.1.1-0ubuntu8.5
>>
>>
>> I tried to attach the device using following xml
>>
>>
>> 
>>
>> 
>>
>> 
>>
>> > function='0x1'/>
>>
>> 
>>
>> 
>>
>>
>> When i attached device it worked properly and i managed to ping another
>> host.
>>
>>
>> virsh # attach-device t5 1.xml
>>
>> Device attached successfully
>>
>>
>> But when i detached it still i had network traffic in my VM but libvirt
>> (virsh) said that
>>
>>
>> virsh # detach-device t5 1.xml
>>
>> Device detached successfully
>>
>>
>> The next time i tried to attach it libvirt said that
>>
>>
>> virsh # attach-device t5 1.xml
>>
>> error: Failed to attach device from 1.xml
>>
>> error: Requested operation is not valid: PCI device :04:01.1 is in
>> use by domain t5
>>
>>
>> and when i continue this action after the 'domain' i receive some
>> strange characters which might come from the memory space (memory
>> content) like some addresses, links or some characters
>>
>>
>> I tried it with different VMs and different guest OSs that the same
>> happend.
>>
>>
>> The same action was done with another host running libvirt version
>>
>>
>> Installed: 1.0.2-0ubuntu11.13.04.5~cloud1
>>
>> Candidate: 1.0.2-0ubuntu11.13.04.5~cloud1
>>
>
> This is probably a bug in libvirt. There's been a rework of hostdev
> plugging and unplugging recently. Can you try the 1.2.3-rc1 and see if the
> problem is gone?
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Michal Privoznik]

On 28.03.2014 09:33, Laine Stump wrote:

On 03/27/2014 07:17 PM, Michal Privoznik wrote:

We allow users to use SLIRP stack. However, there are some knobs
which are not exposed to users, such as host network address, DNS
server, smb, and others.

Signed-off-by: Michal Privoznik 
---
  docs/formatdomain.html.in  |  7 +-
  docs/schemas/domaincommon.rng  | 23 +-
  src/conf/domain_conf.c | 88 ++
  src/conf/domain_conf.h |  6 ++
  src/qemu/qemu_command.c| 19 +
  .../qemuxml2argvdata/qemuxml2argv-net-user-ip.args |  7 ++
  .../qemuxml2argvdata/qemuxml2argv-net-user-ip.xml  | 33 
  tests/qemuxml2argvtest.c   |  1 +
  8 files changed, 180 insertions(+), 4 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-user-ip.args
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-user-ip.xml


It is essential that this new  element be rejected, preferably at
parse time in the qemu-specific post-parse callback, for any interface
type that doesn't support it. Since it is the most obvious way of
specifying an IP address for a guest (and it is a way that has been
requested in the past) we are otherwise certain to have a lot of support
questions asking why the IP address setting isn't being used.


Well, I think rejecting it goes against current policy 'silently ignore 
elements unknown to the libvirt'. But I can live with that here.




Also, the attribute names seem confusing. The "address" attribute is the
address of the *network*, not of the interface, and "dhcpstart" is the
address of the interface. Even though qemu specifies the network address
and interface address separately, the network address is really
pointless, as it can/should be derived from the interface address.


It is, so what XML schema do you propose? I'm not pleased with  
either. But it was the best I could come up with so far. Maybe we are 
aiming for more structured XML here:


  





  

Or even more neting:

  

  
  
  



  




(I realize that "dhcpstart" isn't *exactly* the interface IP, since qemu
allows for multiple IP leases to be acquired from it's internal dhcp
server, but I think that usage is very rare, and unlikely to be possible
for any other backend we might support).

Beyond that, a question not with your patch, but with qemu's
implemenation - does it always assume that the gateway address is
$network.1 ?  If that's the case, then I think definitely we should just
have . If there is support for specifying
the gateway address, then it should be named "gateway", as is already
done in the network xml for static routes.


That's good question, but my qemu code base reading skills are just too 
weak to answer it.


Michal

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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Richard W.M. Jones]

On Fri, Mar 28, 2014 at 08:47:48AM +, Richard W.M. Jones wrote:
> On Fri, Mar 28, 2014 at 10:33:39AM +0200, Laine Stump wrote:
> > Beyond that, a question not with your patch, but with qemu's
> > implemenation - does it always assume that the gateway address is
> > $network.1 ?
> 
> Actually network.2.  The default addresses are:
> 
>network: 10.0.2.0/24 (ie. mask 255.255.255.0)
>default gateway: 10.0.2.2
> dns server: 10.0.2.3
>  dhcp start / normal guest address: 10.0.2.15
> 
> It _is_ possible to change the gateway address, by specifying the
> (confusingly named) 'host=' parameter.  As you suggested I think this
> could be mapped to a gateway XML attribute, although libguestfs would
> not need to use it.

Another couple of thoughts on this patch.

(1) Qemu rejects impossible network configurations -- for example, if
you specify a default gateway address which is outside the network
address range.  However it does so without giving any specific error
messages, see:

http://git.qemu.org/?p=qemu.git;a=blob;f=net/slirp.c;h=cce026bf12bbead8a2bc8b5d0a1af67877266dd9;hb=HEAD#l208

Is there a case for making libvirt do the same checks and give proper
error messages (and/or should we fix qemu)?

(2) IPv6 is impossible with qemu's SLIRP.  SLIRP is ancient BSD code
and was written long before IPv6 existed, and not updated since.  It
looks as if this patch would allow you to do specify IPv6 addresses,
but it should probably reject them at the libvirt level.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Richard W.M. Jones]

On Thu, Mar 27, 2014 at 06:48:24PM +, Richard W.M. Jones wrote:
> On Thu, Mar 27, 2014 at 06:17:46PM +0100, Michal Privoznik wrote:
> > We allow users to use SLIRP stack. However, there are some knobs
> > which are not exposed to users, such as host network address, DNS
> > server, smb, and others.
> 
> The XML looks good.  I have posted a patch to libguestfs to consume
> this XML:
> 
> https://www.redhat.com/archives/libguestfs/2014-March/thread.html#00254
> 
> I wasn't able to try it because of an unrelated segfault in
> libvirt.git at the moment, but will try it out later when that is
> fixed.

I am now able to test this patch, and it works for me.

Not ACKing because AIUI Laine wants you to make some further changes.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

Re: [libvirt] [PATCH 0/2] Fix virHostdev with session libvirt

[Richard W.M. Jones]

On Fri, Mar 28, 2014 at 10:20:29AM +0100, Ján Tomko wrote:
> Reported by Richard W.M. Jones:
> https://www.redhat.com/archives/libvir-list/2014-March/msg01780.html
> 
> Ján Tomko (2):
>   Remove double free in virHostdevManagerDispose
>   Create hostdevmgr in UserRuntimeDirectory for session libvirt
> 
>  src/util/virhostdev.c | 42 +-
>  1 file changed, 33 insertions(+), 9 deletions(-)

I applied these patches to libvirt, and the segfault went away.

Therefore: ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Laine Stump]

On 03/28/2014 10:47 AM, Richard W.M. Jones wrote:
> On Fri, Mar 28, 2014 at 10:33:39AM +0200, Laine Stump wrote:
>> Beyond that, a question not with your patch, but with qemu's
>> implemenation - does it always assume that the gateway address is
>> $network.1 ?
> Actually network.2.  The default addresses are:
>
>network: 10.0.2.0/24 (ie. mask 255.255.255.0)
>default gateway: 10.0.2.2
> dns server: 10.0.2.3
>  dhcp start / normal guest address: 10.0.2.15
>
> It _is_ possible to change the gateway address, by specifying the
> (confusingly named) 'host=' parameter.  As you suggested I think this
> could be mapped to a gateway XML attribute, although libguestfs would
> not need to use it.

Ah, good. I didn't see that in the parameters that I found (why is my
first reaction now to do a google search instead of looking at the man
page? What is wrong with me?? :-P).

In that case, I think that the XML should be

  

I think that

* address should be mandatory

* prefix should default to the natural prefix for that particular
address (we have a function somewhere in the network conf or driver code
that already does this)

* gateway should default to

  address & ~(0x<<(32-prefix)) + 1

* dns should default to gateway (I know this doesn't match qemu's
default, but we shouldn't be designing our XML based on qemu's default,
nor should we be assuming that their default will remain unchanged in
the future - we've been burned by that before. Rather, we should make
the defaults as similar to what we do in other parts of libvirt as
possible).

As for the setting of qemu's options:

* qemu's "network" option should be filled in with

  $(gateway & ~(0x<<(32-prefix)) / $prefix

* qemu's "host" should be filled in with $gateway

* qemu's "dns" should be filled in with $dns

* qemu's "dhcpstart" should be filled in with $address

(do we also want to add a "domain" attribute, which would default to
empty, and be put into qemu's "dnssearch" option?)


When I saw the list of options, I briefly considered that we should
support restricted, but on reading the details it turns out that option
*completely* shuts down network traffic, even to the host, and you need
to add forwarding rules to individually enable what you want. Although
that might be useful in some cases, it is very different from libvirt
networks' "isolated" vs. forward modes, meaning I don't have a clear
idea of a way to make it fit nicely with existing paradigms, so I won't
pursue it for now (or later, unless someone specifically requests it :-)

>
> Rich.
>
> --
>
> Full parameters:
>
>-netdev user,id=id[,option][,option][,...]
>-net user[,option][,option][,...]
>Use the user mode network stack which requires no administrator
>privilege to run. Valid options are:
>
>vlan=n
>Connect user mode stack to VLAN n (n = 0 is the default).
>
>id=id
>name=name
>Assign symbolic name for use in monitor commands.
>
>net=addr[/mask]
>Set IP network address the guest will see. Optionally specify
>the netmask, either in the form a.b.c.d or as number of valid
>top-most bits. Default is 10.0.2.0/24.
>
>host=addr
>Specify the guest-visible address of the host. Default is the
>2nd IP in the guest network, i.e. x.x.x.2.
>
>restrict=on|off
>If this option is enabled, the guest will be isolated, i.e. it
>will not be able to contact the host and no guest IP packets
>will be routed over the host to the outside. This option does
>not affect any explicitly set forwarding rules.
>
>hostname=name
>Specifies the client hostname reported by the built-in DHCP
>server.
>
>dhcpstart=addr
>Specify the first of the 16 IPs the built-in DHCP server can
>assign. Default is the 15th to 31st IP in the guest network,
>i.e. x.x.x.15 to x.x.x.31.
>
>dns=addr
>Specify the guest-visible address of the virtual nameserver.
>The address must be different from the host address. Default is
>the 3rd IP in the guest network, i.e. x.x.x.3.
>
>dnssearch=domain
>Provides an entry for the domain-search list sent by the built-
>in DHCP server. More than one domain suffix can be transmitted
>by specifying this option multiple times. If supported, this
>will cause the guest to automatically try to append the given
>domain suffix(es) in case a domain name can not be resolved.
>
>Example:
>
>   

Re: [libvirt] Device attach / detach problem for passthrough/SR-IOV in libvirt

[Jiri Denemark]

On Fri, Mar 28, 2014 at 10:34:59 +0100, Michal Privoznik wrote:
> On 27.03.2014 13:53, Mohsen Ghaemi wrote:
> > Hi all
> >
> > I just encountered a problem in libvirt while was trying to attach and
> > detach a sr-iov vf to my VM
> >
> >
> > The version of my libvirt is 1.1.1-0ubuntu8.5
> >
> >
> > I tried to attach the device using following xml
> >
> >
> > 
> >
> > 
> >
> > 
> >
> >  > function='0x1'/>
> >
> > 
> >
> > 
> >
> >
> > When i attached device it worked properly and i managed to ping another
> > host.
> >
> >
> > virsh # attach-device t5 1.xml
> >
> > Device attached successfully
> >
> >
> > But when i detached it still i had network traffic in my VM but libvirt
> > (virsh) said that
> >
> >
> > virsh # detach-device t5 1.xml
> >
> > Device detached successfully

Could you run "virsh dumpxml t5" at this point so that we can see if the
interface was really removed? The message from detach-device is not
always correct (and I'm working on fixing it) since some devices need
guest cooperation to be detached and thus this "Device detached
successfully" may just mean that we successfully asked the guest to
remove the device but it was not in fact removed yet.

Jirka

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

Re: [libvirt] Device attach / detach problem for passthrough/SR-IOV in libvirt

[Michal Privoznik]

On 27.03.2014 13:53, Mohsen Ghaemi wrote:

Hi all

I just encountered a problem in libvirt while was trying to attach and
detach a sr-iov vf to my VM


The version of my libvirt is 1.1.1-0ubuntu8.5


I tried to attach the device using following xml















When i attached device it worked properly and i managed to ping another
host.


virsh # attach-device t5 1.xml

Device attached successfully


But when i detached it still i had network traffic in my VM but libvirt
(virsh) said that


virsh # detach-device t5 1.xml

Device detached successfully


The next time i tried to attach it libvirt said that


virsh # attach-device t5 1.xml

error: Failed to attach device from 1.xml

error: Requested operation is not valid: PCI device :04:01.1 is in
use by domain t5


and when i continue this action after the ‘domain’ i receive some
strange characters which might come from the memory space (memory
content) like some addresses, links or some characters


I tried it with different VMs and different guest OSs that the same happend.


The same action was done with another host running libvirt version


Installed: 1.0.2-0ubuntu11.13.04.5~cloud1

Candidate: 1.0.2-0ubuntu11.13.04.5~cloud1


This is probably a bug in libvirt. There's been a rework of hostdev 
plugging and unplugging recently. Can you try the 1.2.3-rc1 and see if 
the problem is gone?


Michal

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

Re: [libvirt] [PATCH] migration: Fix possible bug for migrate cancel

[Paolo Bonzini]

Il 28/03/2014 10:18, Gonglei (Arei) ha scritto:

> > Can you please give more details at how you are triggering the problem
> > with libvirt?  I think Paolo is probably right - the bug is more likely
> > to be in libvirt not expecting the race and not recovering correctly
> > when the race occurs, than it is to be in changing qemu's state algorithm.
> >
When the migration progress reaches 100%, and the migration status becomes
MIG_STATE_COMPLETED in Qemu.
It will take some time which from MIG_STATE_COMPLETED to the migration
thread resources are recovered.
If we cancel the migration at this moment, the migrate_fd_cancel function will
break directly without reporting
error code. Then, libvirt considers the cancle operation a success, contrary
facts.


There is no error, once migration is completed you can still shutdown on 
the destination and continue on the source.  Libvirt should either:


1) poll with "query-migrate" after migrate_cancel, and report an error 
there if it's the desired semantics;


2) toggle a "cancelled" flag before asking QEMU to cancel migration, 
check it in the migration functions after "query-migrate" reported 
completion; if it is true, do not resume on the destination.


Another reason for doing it in libvirt is that the serialization between 
cancellation and completion of migration ultimately is controlled by 
libvirt's lock.  Doing this in QEMU makes it harder to reason about 
concurrency.


Paolo

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

[libvirt] [PATCH 2/2] Create hostdevmgr in UserRuntimeDirectory for session libvirt

[Ján Tomko]

Without this, session libvirt won't start if it fails to create
the directory.
---

So far the directory is only used by SRIOV, which I believe does not work
for unprivileged libvirt, but I found always creating the state dir nicer
than possibly crashing on NULL derefernce.

 src/util/virhostdev.c | 40 +---
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index d80dbf7..7936289 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -89,6 +89,7 @@ static virHostdevManagerPtr
 virHostdevManagerNew(void)
 {
 virHostdevManagerPtr hostdevMgr;
+bool privileged = geteuid() == 0;
 
 if (!(hostdevMgr = virObjectNew(virHostdevManagerClass)))
 return NULL;
@@ -105,14 +106,39 @@ virHostdevManagerNew(void)
 if ((hostdevMgr->activeSCSIHostdevs = virSCSIDeviceListNew()) == NULL)
 goto error;
 
-if (VIR_STRDUP(hostdevMgr->stateDir, HOSTDEV_STATE_DIR) < 0)
-goto error;
+if (privileged) {
+if (VIR_STRDUP(hostdevMgr->stateDir, HOSTDEV_STATE_DIR) < 0)
+goto error;
 
-if (virFileMakePath(hostdevMgr->stateDir) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _("Failed to create state dir '%s'"),
-   hostdevMgr->stateDir);
-goto error;
+if (virFileMakePath(hostdevMgr->stateDir) < 0) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("Failed to create state dir '%s'"),
+   hostdevMgr->stateDir);
+goto error;
+}
+} else {
+char *rundir = NULL;
+mode_t old_umask;
+
+if (!(rundir = virGetUserRuntimeDirectory()))
+goto error;
+
+if (virAsprintf(&hostdevMgr->stateDir, "%s/hostdevmgr", rundir) < 0) {
+VIR_FREE(rundir);
+goto error;
+}
+VIR_FREE(rundir);
+
+old_umask = umask(077);
+
+if (virFileMakePath(hostdevMgr->stateDir) < 0) {
+umask(old_umask);
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("Failed to create state dir '%s'"),
+   hostdevMgr->stateDir);
+goto error;
+}
+umask(old_umask);
 }
 
 return hostdevMgr;
-- 
1.8.3.2

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

[libvirt] [PATCH 1/2] Remove double free in virHostdevManagerDispose

[Ján Tomko]

The object itself is freed by virObjectUnref.
---
 src/util/virhostdev.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 54023bd..d80dbf7 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -83,8 +83,6 @@ virHostdevManagerDispose(void *obj)
 virObjectUnref(hostdevMgr->activeUSBHostdevs);
 virObjectUnref(hostdevMgr->activeSCSIHostdevs);
 VIR_FREE(hostdevMgr->stateDir);
-
-VIR_FREE(hostdevMgr);
 }
 
 static virHostdevManagerPtr
-- 
1.8.3.2

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

[libvirt] [PATCH 0/2] Fix virHostdev with session libvirt

[Ján Tomko]

Reported by Richard W.M. Jones:
https://www.redhat.com/archives/libvir-list/2014-March/msg01780.html

Ján Tomko (2):
  Remove double free in virHostdevManagerDispose
  Create hostdevmgr in UserRuntimeDirectory for session libvirt

 src/util/virhostdev.c | 42 +-
 1 file changed, 33 insertions(+), 9 deletions(-)

-- 
1.8.3.2

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

Re: [libvirt] [PATCH] migration: Fix possible bug for migrate cancel

[Gonglei (Arei)]

> > >> Return error for migrate cancel, when migration status is not
> > >> MIG_STATE_SETUP or MIG_STATE_ACTIVE. Thus, libvirt can can
> > >> perceive the operation fails.
> > >>
> > >> Signed-off-by: zengjunliang 
> > >> Signed-off-by: Gonglei 
> > >
> > > I think this is done on purpose, because canceling migration is racy.
> > > Instead, libvirt should do "query-migrate" and check if the migration
> > > was completed or canceled.
> >
> > Can you please give more details at how you are triggering the problem
> > with libvirt?  I think Paolo is probably right - the bug is more likely
> > to be in libvirt not expecting the race and not recovering correctly
> > when the race occurs, than it is to be in changing qemu's state algorithm.
> >
> When the migration progress reaches 100%, and the migration status becomes
> MIG_STATE_COMPLETED in Qemu.
> It will take some time which from MIG_STATE_COMPLETED to the migration
> thread resources are recovered.
> If we cancel the migration at this moment, the migrate_fd_cancel function will
> break directly without reporting
> error code. Then, libvirt considers the cancle operation a success, contrary
> facts.
> 

Ping... 


Best regards,
-Gonglei

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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Richard W.M. Jones]

On Fri, Mar 28, 2014 at 10:33:39AM +0200, Laine Stump wrote:
> Beyond that, a question not with your patch, but with qemu's
> implemenation - does it always assume that the gateway address is
> $network.1 ?

Actually network.2.  The default addresses are:

   network: 10.0.2.0/24 (ie. mask 255.255.255.0)
   default gateway: 10.0.2.2
dns server: 10.0.2.3
 dhcp start / normal guest address: 10.0.2.15

It _is_ possible to change the gateway address, by specifying the
(confusingly named) 'host=' parameter.  As you suggested I think this
could be mapped to a gateway XML attribute, although libguestfs would
not need to use it.

Rich.

--

Full parameters:

   -netdev user,id=id[,option][,option][,...]
   -net user[,option][,option][,...]
   Use the user mode network stack which requires no administrator
   privilege to run. Valid options are:

   vlan=n
   Connect user mode stack to VLAN n (n = 0 is the default).

   id=id
   name=name
   Assign symbolic name for use in monitor commands.

   net=addr[/mask]
   Set IP network address the guest will see. Optionally specify
   the netmask, either in the form a.b.c.d or as number of valid
   top-most bits. Default is 10.0.2.0/24.

   host=addr
   Specify the guest-visible address of the host. Default is the
   2nd IP in the guest network, i.e. x.x.x.2.

   restrict=on|off
   If this option is enabled, the guest will be isolated, i.e. it
   will not be able to contact the host and no guest IP packets
   will be routed over the host to the outside. This option does
   not affect any explicitly set forwarding rules.

   hostname=name
   Specifies the client hostname reported by the built-in DHCP
   server.

   dhcpstart=addr
   Specify the first of the 16 IPs the built-in DHCP server can
   assign. Default is the 15th to 31st IP in the guest network,
   i.e. x.x.x.15 to x.x.x.31.

   dns=addr
   Specify the guest-visible address of the virtual nameserver.
   The address must be different from the host address. Default is
   the 3rd IP in the guest network, i.e. x.x.x.3.

   dnssearch=domain
   Provides an entry for the domain-search list sent by the built-
   in DHCP server. More than one domain suffix can be transmitted
   by specifying this option multiple times. If supported, this
   will cause the guest to automatically try to append the given
   domain suffix(es) in case a domain name can not be resolved.

   Example:

   qemu -net 
user,dnssearch=mgmt.example.org,dnssearch=example.org [...]

   tftp=dir
   When using the user mode network stack, activate a built-in
   TFTP server. The files in dir will be exposed as the root of a
   TFTP server.  The TFTP client on the guest must be configured
   in binary mode (use the command "bin" of the Unix TFTP client).

   bootfile=file
   When using the user mode network stack, broadcast file as the
   BOOTP filename. In conjunction with tftp, this can be used to
   network boot a guest from a local directory.

   Example (using pxelinux):
   qemu-system-i386 -hda linux.img -boot n -net 
user,tftp=/path/to/tftp/files,bootfile=/pxelinux.0

   smb=dir[,smbserver=addr]
   When using the user mode network stack, activate a built-in SMB
   server so that Windows OSes can access to the host files in dir
   transparently. The IP address of the SMB server can be set to
   addr. By default the 4th IP in the guest network is used, i.e.
   x.x.x.4.

   In the guest Windows OS, the line:

   10.0.2.4 smbserver

   must be added in the file C:\WINDOWS\LMHOSTS (for windows
   9x/Me) or C:\WINNT\SYSTEM32\DRIVERS\ETC\LMHOSTS (Windows
   NT/2000).

   Then dir can be accessed in \\smbserver\qemu.

   Note that a SAMBA server must be installed on the host OS.
   QEMU was tested successfully with smbd versions from Red Hat 9,
   Fedora Core 3 and OpenSUSE 11.x.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

Re: [libvirt] [PATCH] Expose SLIRP attributes

[Laine Stump]

On 03/27/2014 07:17 PM, Michal Privoznik wrote:
> We allow users to use SLIRP stack. However, there are some knobs
> which are not exposed to users, such as host network address, DNS
> server, smb, and others.
>
> Signed-off-by: Michal Privoznik 
> ---
>  docs/formatdomain.html.in  |  7 +-
>  docs/schemas/domaincommon.rng  | 23 +-
>  src/conf/domain_conf.c | 88 
> ++
>  src/conf/domain_conf.h |  6 ++
>  src/qemu/qemu_command.c| 19 +
>  .../qemuxml2argvdata/qemuxml2argv-net-user-ip.args |  7 ++
>  .../qemuxml2argvdata/qemuxml2argv-net-user-ip.xml  | 33 
>  tests/qemuxml2argvtest.c   |  1 +
>  8 files changed, 180 insertions(+), 4 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-user-ip.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-user-ip.xml

It is essential that this new  element be rejected, preferably at
parse time in the qemu-specific post-parse callback, for any interface
type that doesn't support it. Since it is the most obvious way of
specifying an IP address for a guest (and it is a way that has been
requested in the past) we are otherwise certain to have a lot of support
questions asking why the IP address setting isn't being used.

Also, the attribute names seem confusing. The "address" attribute is the
address of the *network*, not of the interface, and "dhcpstart" is the
address of the interface. Even though qemu specifies the network address
and interface address separately, the network address is really
pointless, as it can/should be derived from the interface address.

(I realize that "dhcpstart" isn't *exactly* the interface IP, since qemu
allows for multiple IP leases to be acquired from it's internal dhcp
server, but I think that usage is very rare, and unlikely to be possible
for any other backend we might support).

Beyond that, a question not with your patch, but with qemu's
implemenation - does it always assume that the gateway address is
$network.1 ?  If that's the case, then I think definitely we should just
have . If there is support for specifying
the gateway address, then it should be named "gateway", as is already
done in the network xml for static routes.

>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 7f90455..0a353ca 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3225,7 +3225,11 @@
>starting from 10.0.2.15. The default router will be
>10.0.2.2 and the DNS server will be 10.0.2.3.
>This networking is the only option for unprivileged users who need 
> their
> -  VMs to have outgoing access.
> +  VMs to have outgoing access. Since 1.2.3 the
> +  user network can have the  element to override the default
> +  network of 10.0.2.0/24. For example it can be set to
> +  192.168.2.0/24. The whole element and its attributes are
> +  optional.
>  
>  
>  
> @@ -3235,6 +3239,7 @@
>  ...
>  
>
> +   dhcpstart="192.168.2.9"/>
>  
>
>...
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index bcd8142..5745ce7 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2152,9 +2152,26 @@
>
>
>  
> -  
> -
> -  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +
> +  
>
>  
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6fb216e..aec14ed 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1539,6 +1539,11 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
>  break;
>  
>  case VIR_DOMAIN_NET_TYPE_USER:
> +VIR_FREE(def->data.user.ipaddr);
> +VIR_FREE(def->data.user.dns);
> +VIR_FREE(def->data.user.dhcpstart);
> +break;
> +
>  case VIR_DOMAIN_NET_TYPE_LAST:
>  break;
>  }
> @@ -6669,6 +6674,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>  char *mode = NULL;
>  char *linkstate = NULL;
>  char *addrtype = NULL;
> +char *prefix = NULL;
> +char *dns = NULL;
> +char *dhcpstart = NULL;
>  virNWFilterHashTablePtr filterparams = NULL;
>  virDomainActualNetDefPtr actual = NULL;
>  xmlNodePtr oldnode = ctxt->node;
> @@ -6750,6 +6758,13 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>  def->type == VIR_DOMAIN_NET_TY