[libvirt] [PATCH 2/3] storage: Introduce iscsi_direct pool type

[clem]

From: Clementine Hayat 

Signed-off-by: Clementine Hayat 
---
 configure.ac   |  6 ++-
 m4/virt-storage-iscsi-direct.m4| 41 +
 src/conf/domain_conf.c |  1 +
 src/conf/storage_conf.c| 31 ++--
 src/conf/storage_conf.h|  1 +
 src/conf/virstorageobj.c   |  2 +
 src/storage/Makefile.inc.am| 21 +++
 src/storage/storage_backend.c  |  6 +++
 src/storage/storage_backend_iscsi_direct.c | 43 ++
 src/storage/storage_backend_iscsi_direct.h |  6 +++
 src/storage/storage_driver.c   |  1 +
 tools/virsh-pool.c |  3 ++
 12 files changed, 157 insertions(+), 5 deletions(-)
 create mode 100644 m4/virt-storage-iscsi-direct.m4
 create mode 100644 src/storage/storage_backend_iscsi_direct.c
 create mode 100644 src/storage/storage_backend_iscsi_direct.h

diff --git a/configure.ac b/configure.ac
index c668630a79..87ac4dc2c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -564,6 +564,7 @@ LIBVIRT_STORAGE_ARG_DIR
 LIBVIRT_STORAGE_ARG_FS
 LIBVIRT_STORAGE_ARG_LVM
 LIBVIRT_STORAGE_ARG_ISCSI
+LIBVIRT_STORAGE_ARG_ISCSI_DIRECT
 LIBVIRT_STORAGE_ARG_SCSI
 LIBVIRT_STORAGE_ARG_MPATH
 LIBVIRT_STORAGE_ARG_DISK
@@ -578,6 +579,7 @@ if test "$with_libvirtd" = "no"; then
   with_storage_fs=no
   with_storage_lvm=no
   with_storage_iscsi=no
+  with_storage_iscsi_direct=no
   with_storage_scsi=no
   with_storage_mpath=no
   with_storage_disk=no
@@ -598,6 +600,7 @@ LIBVIRT_STORAGE_CHECK_DIR
 LIBVIRT_STORAGE_CHECK_FS
 LIBVIRT_STORAGE_CHECK_LVM
 LIBVIRT_STORAGE_CHECK_ISCSI
+LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT
 LIBVIRT_STORAGE_CHECK_SCSI
 LIBVIRT_STORAGE_CHECK_MPATH
 LIBVIRT_STORAGE_CHECK_DISK
@@ -608,7 +611,7 @@ LIBVIRT_STORAGE_CHECK_ZFS
 LIBVIRT_STORAGE_CHECK_VSTORAGE
 
 with_storage=no
-for backend in dir fs lvm iscsi scsi mpath rbd disk; do
+for backend in dir fs lvm iscsi iscsi_direct scsi mpath rbd disk; do
 if eval test \$with_storage_$backend = yes; then
 with_storage=yes
 break
@@ -936,6 +939,7 @@ LIBVIRT_STORAGE_RESULT_DIR
 LIBVIRT_STORAGE_RESULT_FS
 LIBVIRT_STORAGE_RESULT_LVM
 LIBVIRT_STORAGE_RESULT_ISCSI
+LIBVIRT_STORAGE_RESULT_ISCSI_DIRECT
 LIBVIRT_STORAGE_RESULT_SCSI
 LIBVIRT_STORAGE_RESULT_MPATH
 LIBVIRT_STORAGE_RESULT_DISK
diff --git a/m4/virt-storage-iscsi-direct.m4 b/m4/virt-storage-iscsi-direct.m4
new file mode 100644
index 00..cc2d490352
--- /dev/null
+++ b/m4/virt-storage-iscsi-direct.m4
@@ -0,0 +1,41 @@
+dnl Iscsi-direct storage
+dnl
+dnl Copyright (C) 2018 Clementine Hayat.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+
+AC_DEFUN([LIBVIRT_STORAGE_ARG_ISCSI_DIRECT], [
+  LIBVIRT_ARG_WITH_FEATURE([STORAGE_ISCSI_DIRECT],
+   [iscsi-direct backend for the storage driver],
+   [check])
+])
+
+AC_DEFUN([LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT], [
+  AC_REQUIRE([LIBVIRT_CHECK_LIBISCSI])
+  if test "$with_storage_iscsi_direct" = "check"; then
+with_storage_iscsi_direct=$with_libiscsi
+  fi
+  if test "$with_storage_iscsi_direct" = "yes"; then
+AC_DEFINE_UNQUOTED([WITH_STORAGE_ISCSI_DIRECT], [1],
+   [whether iSCSI backend for storage driver is enabled])
+  fi
+  AM_CONDITIONAL([WITH_STORAGE_ISCSI_DIRECT],
+ [test "$with_storage_iscsi_direct" = "yes"])
+])
+
+AC_DEFUN([LIBVIRT_STORAGE_RESULT_ISCSI_DIRECT], [
+  LIBVIRT_RESULT([iscsi-direct], [$with_storage_iscsi_direct])
+])
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7396616eda..0a9509de0b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30163,6 +30163,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr 
def)
 
 break;
 
+case VIR_STORAGE_POOL_ISCSI_DIRECT:
 case VIR_STORAGE_POOL_ISCSI:
 if (def->startupPolicy) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5036ab9ef8..7a4b00ad8c 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -62,9 +62,9 @@ VIR_ENUM_IMPL(virStoragePool,
   VIR_STORAGE_POOL_LAST,
   "dir", "fs", "netfs",
   "logical", "disk", "iscsi",
-  "scsi", "mpath", "rbd",

[libvirt] [PATCH 1/3] configure: Introduce libiscsi in build system

[clem]

From: Clementine Hayat 

The minimal required version is 1.18.0 because the synchrounous function
needed were introduced here.

Signed-off-by: Clementine Hayat 
---
 configure.ac|  3 +++
 m4/virt-libiscsi.m4 | 30 ++
 2 files changed, 33 insertions(+)
 create mode 100644 m4/virt-libiscsi.m4

diff --git a/configure.ac b/configure.ac
index a87ca06854..c668630a79 100644
--- a/configure.ac
+++ b/configure.ac
@@ -250,6 +250,7 @@ LIBVIRT_ARG_FIREWALLD
 LIBVIRT_ARG_FUSE
 LIBVIRT_ARG_GLUSTER
 LIBVIRT_ARG_HAL
+LIBVIRT_ARG_LIBISCSI
 LIBVIRT_ARG_LIBPCAP
 LIBVIRT_ARG_LIBSSH
 LIBVIRT_ARG_LIBXML
@@ -290,6 +291,7 @@ LIBVIRT_CHECK_FUSE
 LIBVIRT_CHECK_GLUSTER
 LIBVIRT_CHECK_GNUTLS
 LIBVIRT_CHECK_HAL
+LIBVIRT_CHECK_LIBISCSI
 LIBVIRT_CHECK_LIBNL
 LIBVIRT_CHECK_LIBPARTED
 LIBVIRT_CHECK_LIBPCAP
@@ -970,6 +972,7 @@ LIBVIRT_RESULT_FUSE
 LIBVIRT_RESULT_GLUSTER
 LIBVIRT_RESULT_GNUTLS
 LIBVIRT_RESULT_HAL
+LIBVIRT_RESULT_LIBISCSI
 LIBVIRT_RESULT_LIBNL
 LIBVIRT_RESULT_LIBPCAP
 LIBVIRT_RESULT_LIBSSH
diff --git a/m4/virt-libiscsi.m4 b/m4/virt-libiscsi.m4
new file mode 100644
index 00..2747f00ec4
--- /dev/null
+++ b/m4/virt-libiscsi.m4
@@ -0,0 +1,30 @@
+dnl Libiscsi library
+dnl
+dnl Copyright (C) 2018 Clementine Hayat.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+
+AC_DEFUN([LIBVIRT_ARG_LIBISCSI],[
+  LIBVIRT_ARG_WITH_FEATURE([LIBISCSI], [libiscsi], [check], [1.18.0])
+])
+
+AC_DEFUN([LIBVIRT_CHECK_LIBISCSI],[
+  LIBVIRT_CHECK_PKG([LIBISCSI], [libiscsi], [1.18.0])
+])
+
+AC_DEFUN([LIBVIRT_RESULT_LIBISCSI],[
+  LIBVIRT_RESULT_LIB(LIBISCSI)
+])
-- 
2.17.1

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

[libvirt] [PATCH 3/3] storage: Implement iscsi_direct pool backend

[clem]

From: Clementine Hayat 

We need here libiscsi for the storgae pool backend.
For the iscsi-direct storage pool, only checkPool and refreshPool should
be necessary.
The pool is state-less and just need the informations within the volume
to work.

Signed-off-by: Clementine Hayat 
---
 m4/virt-storage-iscsi-direct.m4|   3 +
 src/storage/Makefile.inc.am|   3 +
 src/storage/storage_backend_iscsi_direct.c | 407 -
 3 files changed, 410 insertions(+), 3 deletions(-)

diff --git a/m4/virt-storage-iscsi-direct.m4 b/m4/virt-storage-iscsi-direct.m4
index cc2d490352..dab4414169 100644
--- a/m4/virt-storage-iscsi-direct.m4
+++ b/m4/virt-storage-iscsi-direct.m4
@@ -29,6 +29,9 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT], [
 with_storage_iscsi_direct=$with_libiscsi
   fi
   if test "$with_storage_iscsi_direct" = "yes"; then
+if test "$with_libiscsi" = "no"; then
+  AC_MSG_ERROR([Need libiscsi for iscsi-direct storage driver])
+fi
 AC_DEFINE_UNQUOTED([WITH_STORAGE_ISCSI_DIRECT], [1],
[whether iSCSI backend for storage driver is enabled])
   fi
diff --git a/src/storage/Makefile.inc.am b/src/storage/Makefile.inc.am
index d81864f5b9..bd5ea06f8b 100644
--- a/src/storage/Makefile.inc.am
+++ b/src/storage/Makefile.inc.am
@@ -202,6 +202,8 @@ endif WITH_STORAGE_ISCSI
 if WITH_STORAGE_ISCSI_DIRECT
 libvirt_storage_backend_iscsi_direct_la_SOURCES = 
$(STORAGE_DRIVER_ISCSI_DIRECT_SOURCES)
 libvirt_storage_backend_iscsi_direct_la_CFLAGS = \
+   -I$(srcdir)/conf \
+   -I$(srcdir)/secret \
$(LIBISCSI_CFLAGS) \
$(AM_CFLAGS) \
$(NULL)
@@ -210,6 +212,7 @@ storagebackend_LTLIBRARIES += 
libvirt_storage_backend_iscsi-direct.la
 libvirt_storage_backend_iscsi_direct_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 libvirt_storage_backend_iscsi_direct_la_LIBADD = \
libvirt.la \
+   $(LIBISCSI_LIBS) \
../gnulib/lib/libgnu.la \
$(NULL)
 endif WITH_STORAGE_ISCSI_DIRECT
diff --git a/src/storage/storage_backend_iscsi_direct.c 
b/src/storage/storage_backend_iscsi_direct.c
index e3c1f75b42..f93c8e1e67 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -1,34 +1,435 @@
 #include 
 #include 
+#include 
+#include 
+#include 
 #include 
 #include 
 #include 
 
 #include "datatypes.h"
 #include "driver.h"
+#include "secret_util.h"
 #include "storage_backend_iscsi_direct.h"
 #include "storage_util.h"
+#include "viralloc.h"
+#include "vircommand.h"
+#include "virerror.h"
+#include "virfile.h"
 #include "virlog.h"
 #include "virobject.h"
+#include "virstring.h"
+#include "virtime.h"
+#include "viruuid.h"
 
 #define VIR_FROM_THIS VIR_FROM_STORAGE
 
+#define ISCSI_DEFAULT_TARGET_PORT 3260
+#define VIR_ISCSI_TEST_UNIT_TIMEOUT 30 * 1000
+
 VIR_LOG_INIT("storage.storage_backend_iscsi_direct");
 
+static struct iscsi_context *
+virISCSIDirectCreateContext(const char* initiator_iqn)
+{
+struct iscsi_context *iscsi = NULL;
+
+iscsi = iscsi_create_context(initiator_iqn);
+if (!iscsi)
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to create iscsi context for %s"),
+   initiator_iqn);
+return iscsi;
+}
+
+static char *
+virStorageBackendISCSIDirectPortal(virStoragePoolSourcePtr source)
+{
+char *portal = NULL;
+
+if (source->nhost != 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Expected exactly 1 host for the storage pool"));
+return NULL;
+}
+if (source->hosts[0].port == 0) {
+ignore_value(virAsprintf(, "%s:%d",
+ source->hosts[0].name,
+ ISCSI_DEFAULT_TARGET_PORT));
+} else if (strchr(source->hosts[0].name, ':')) {
+ignore_value(virAsprintf(, "[%s]:%d",
+ source->hosts[0].name,
+ source->hosts[0].port));
+} else {
+ignore_value(virAsprintf(, "%s:%d",
+ source->hosts[0].name,
+ source->hosts[0].port));
+}
+return portal;
+}
+
+static int
+virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi,
+virStoragePoolSourcePtr source)
+{
+unsigned char *secret_value = NULL;
+size_t secret_size;
+virStorageAuthDefPtr authdef = source->auth;
+int ret = -1;
+virConnectPtr conn = NULL;
+
+if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE)
+return 0;
+
+VIR_DEBUG("username='%s' authType=%d seclookupdef.type=%d",
+  authdef->username, authdef->authType, 
authdef->seclookupdef.type);
+
+if (authdef->authType != VIR_STORAGE_AUTH_TYPE_CHAP) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("iscsi-direct pool only supports 'chap' auth type"));
+return ret;
+}
+
+if (!(conn = 

[libvirt] [PATCH 0/3] iscsi-direct: first part

[clem]

From: Clementine Hayat 

Hello,

This is the implementation of the iscsi-direct backend storage pool.
The documentation, some API calls and tests are still missing and will
be comming in a second part.

Best Regards,

-- 
Clementine Hayat


Clementine Hayat (3):
  configure: Introduce libiscsi in build system
  storage: Introduce iscsi_direct pool type
  storage: Implement iscsi_direct pool backend

 configure.ac   |   9 +-
 m4/virt-libiscsi.m4|  30 ++
 m4/virt-storage-iscsi-direct.m4|  44 ++
 src/conf/domain_conf.c |   1 +
 src/conf/storage_conf.c|  31 +-
 src/conf/storage_conf.h|   1 +
 src/conf/virstorageobj.c   |   2 +
 src/storage/Makefile.inc.am|  24 ++
 src/storage/storage_backend.c  |   6 +
 src/storage/storage_backend_iscsi_direct.c | 444 +
 src/storage/storage_backend_iscsi_direct.h |   6 +
 src/storage/storage_driver.c   |   1 +
 tools/virsh-pool.c |   3 +
 13 files changed, 597 insertions(+), 5 deletions(-)
 create mode 100644 m4/virt-libiscsi.m4
 create mode 100644 m4/virt-storage-iscsi-direct.m4
 create mode 100644 src/storage/storage_backend_iscsi_direct.c
 create mode 100644 src/storage/storage_backend_iscsi_direct.h

-- 
2.17.1

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

[libvirt] [PATCH v4 23/31] util: bitmap: remove redundant include directive

[Sukrit Bhatnagar]

viralloc.h is pulled in by virbitmap.h

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virbitmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 0cc5292..ef18dad 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -31,7 +31,6 @@
 #include 
 
 #include "virbitmap.h"
-#include "viralloc.h"
 #include "virbuffer.h"
 #include "c-ctype.h"
 #include "count-one-bits.h"
-- 
1.8.3.1

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

[libvirt] [PATCH v4 24/31] util: bitmap: use VIR_AUTOPTR for aggregate types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virbitmap.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index ef18dad..5b6e55f 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -1202,15 +1202,12 @@ char *
 virBitmapDataFormat(const void *data,
 int len)
 {
-virBitmapPtr map = NULL;
-char *ret = NULL;
+VIR_AUTOPTR(virBitmap) map = NULL;
 
 if (!(map = virBitmapNewData(data, len)))
 return NULL;
 
-ret = virBitmapFormat(map);
-virBitmapFree(map);
-return ret;
+return virBitmapFormat(map);
 }
 
 
-- 
1.8.3.1

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

[libvirt] [PATCH v4 25/31] util: iohelper: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/iohelper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/util/iohelper.c b/src/util/iohelper.c
index bb8a8dd..f7794dc 100644
--- a/src/util/iohelper.c
+++ b/src/util/iohelper.c
@@ -46,7 +46,7 @@
 static int
 runIO(const char *path, int fd, int oflags)
 {
-void *base = NULL; /* Location to be freed */
+VIR_AUTOFREE(void *) base = NULL; /* Location to be freed */
 char *buf = NULL; /* Aligned location within base */
 size_t buflen = 1024*1024;
 intptr_t alignMask = 64*1024 - 1;
@@ -174,8 +174,6 @@ runIO(const char *path, int fd, int oflags)
 virReportSystemError(errno, _("Unable to close %s"), path);
 ret = -1;
 }
-
-VIR_FREE(base);
 return ret;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH v4 19/31] util: json: remove redundant include directive

[Sukrit Bhatnagar]

viralloc.h is pulled in by virjson.h

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virjson.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/virjson.c b/src/util/virjson.c
index 0559d40..92f3994 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -24,7 +24,6 @@
 #include 
 
 #include "virjson.h"
-#include "viralloc.h"
 #include "virerror.h"
 #include "virlog.h"
 #include "virstring.h"
-- 
1.8.3.1

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

[libvirt] [PATCH v4 18/31] util: json: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Sukrit Bhatnagar]

Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope.

When a variable of type virJSONValuePtr is declared using
VIR_AUTOPTR, the function virJSONValueFree will be run
automatically on it when it goes out of scope.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virjson.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util/virjson.h b/src/util/virjson.h
index e4a82bd..75f7f17 100644
--- a/src/util/virjson.h
+++ b/src/util/virjson.h
@@ -26,6 +26,7 @@
 
 # include "internal.h"
 # include "virbitmap.h"
+# include "viralloc.h"
 
 # include 
 
@@ -156,4 +157,6 @@ char *virJSONStringReformat(const char *jsonstr, bool 
pretty);
 
 virJSONValuePtr virJSONValueObjectDeflatten(virJSONValuePtr json);
 
+VIR_DEFINE_AUTOPTR_FUNC(virJSONValue, virJSONValueFree)
+
 #endif /* __VIR_JSON_H_ */
-- 
1.8.3.1

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

[libvirt] [PATCH v4 31/31] util: identity: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/viridentity.c | 52 +-
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/src/util/viridentity.c b/src/util/viridentity.c
index 2f4307b..c621444 100644
--- a/src/util/viridentity.c
+++ b/src/util/viridentity.c
@@ -133,8 +133,8 @@ int virIdentitySetCurrent(virIdentityPtr ident)
  */
 virIdentityPtr virIdentityGetSystem(void)
 {
-char *username = NULL;
-char *groupname = NULL;
+VIR_AUTOFREE(char *) username = NULL;
+VIR_AUTOFREE(char *) groupname = NULL;
 unsigned long long startTime;
 virIdentityPtr ret = NULL;
 #if WITH_SELINUX
@@ -154,14 +154,14 @@ virIdentityPtr virIdentityGetSystem(void)
 goto error;
 
 if (!(username = virGetUserName(geteuid(
-goto cleanup;
+return ret;
 if (virIdentitySetUNIXUserName(ret, username) < 0)
 goto error;
 if (virIdentitySetUNIXUserID(ret, getuid()) < 0)
 goto error;
 
 if (!(groupname = virGetGroupName(getegid(
-goto cleanup;
+return ret;
 if (virIdentitySetUNIXGroupName(ret, groupname) < 0)
 goto error;
 if (virIdentitySetUNIXGroupID(ret, getgid()) < 0)
@@ -172,7 +172,7 @@ virIdentityPtr virIdentityGetSystem(void)
 if (getcon() < 0) {
 virReportSystemError(errno, "%s",
  _("Unable to lookup SELinux process 
context"));
-goto cleanup;
+return ret;
 }
 if (virIdentitySetSELinuxContext(ret, con) < 0) {
 freecon(con);
@@ -182,15 +182,11 @@ virIdentityPtr virIdentityGetSystem(void)
 }
 #endif
 
- cleanup:
-VIR_FREE(username);
-VIR_FREE(groupname);
 return ret;
 
  error:
 virObjectUnref(ret);
-ret = NULL;
-goto cleanup;
+return NULL;
 }
 
 
@@ -461,15 +457,14 @@ int virIdentitySetUNIXUserName(virIdentityPtr ident,
 int virIdentitySetUNIXUserID(virIdentityPtr ident,
  uid_t uid)
 {
-char *val;
-int ret;
+VIR_AUTOFREE(char *) val = NULL;
+
 if (virAsprintf(, "%d", (int)uid) < 0)
 return -1;
-ret = virIdentitySetAttr(ident,
+
+return virIdentitySetAttr(ident,
  VIR_IDENTITY_ATTR_UNIX_USER_ID,
  val);
-VIR_FREE(val);
-return ret;
 }
 
 
@@ -485,45 +480,42 @@ int virIdentitySetUNIXGroupName(virIdentityPtr ident,
 int virIdentitySetUNIXGroupID(virIdentityPtr ident,
   gid_t gid)
 {
-char *val;
-int ret;
+VIR_AUTOFREE(char *) val = NULL;
+
 if (virAsprintf(, "%d", (int)gid) < 0)
 return -1;
-ret = virIdentitySetAttr(ident,
+
+return virIdentitySetAttr(ident,
  VIR_IDENTITY_ATTR_UNIX_GROUP_ID,
  val);
-VIR_FREE(val);
-return ret;
 }
 
 
 int virIdentitySetUNIXProcessID(virIdentityPtr ident,
 pid_t pid)
 {
-char *val;
-int ret;
+VIR_AUTOFREE(char *) val = NULL;
+
 if (virAsprintf(, "%lld", (long long) pid) < 0)
 return -1;
-ret = virIdentitySetAttr(ident,
+
+return virIdentitySetAttr(ident,
  VIR_IDENTITY_ATTR_UNIX_PROCESS_ID,
  val);
-VIR_FREE(val);
-return ret;
 }
 
 
 int virIdentitySetUNIXProcessTime(virIdentityPtr ident,
   unsigned long long timestamp)
 {
-char *val;
-int ret;
+VIR_AUTOFREE(char *) val = NULL;
+
 if (virAsprintf(, "%llu", timestamp) < 0)
 return -1;
-ret = virIdentitySetAttr(ident,
+
+return virIdentitySetAttr(ident,
  VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME,
  val);
-VIR_FREE(val);
-return ret;
 }
 
 
-- 
1.8.3.1

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

[libvirt] [PATCH v4 20/31] util: json: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virjson.c | 49 ++---
 1 file changed, 14 insertions(+), 35 deletions(-)

diff --git a/src/util/virjson.c b/src/util/virjson.c
index 92f3994..82f539f 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -496,65 +496,50 @@ virJSONValueNewNumber(const char *data)
 virJSONValuePtr
 virJSONValueNewNumberInt(int data)
 {
-virJSONValuePtr val = NULL;
-char *str;
+VIR_AUTOFREE(char *) str = NULL;
 if (virAsprintf(, "%i", data) < 0)
 return NULL;
-val = virJSONValueNewNumber(str);
-VIR_FREE(str);
-return val;
+return virJSONValueNewNumber(str);
 }
 
 
 virJSONValuePtr
 virJSONValueNewNumberUint(unsigned int data)
 {
-virJSONValuePtr val = NULL;
-char *str;
+VIR_AUTOFREE(char *) str = NULL;
 if (virAsprintf(, "%u", data) < 0)
 return NULL;
-val = virJSONValueNewNumber(str);
-VIR_FREE(str);
-return val;
+return virJSONValueNewNumber(str);
 }
 
 
 virJSONValuePtr
 virJSONValueNewNumberLong(long long data)
 {
-virJSONValuePtr val = NULL;
-char *str;
+VIR_AUTOFREE(char *) str = NULL;
 if (virAsprintf(, "%lld", data) < 0)
 return NULL;
-val = virJSONValueNewNumber(str);
-VIR_FREE(str);
-return val;
+return virJSONValueNewNumber(str);
 }
 
 
 virJSONValuePtr
 virJSONValueNewNumberUlong(unsigned long long data)
 {
-virJSONValuePtr val = NULL;
-char *str;
+VIR_AUTOFREE(char *) str = NULL;
 if (virAsprintf(, "%llu", data) < 0)
 return NULL;
-val = virJSONValueNewNumber(str);
-VIR_FREE(str);
-return val;
+return virJSONValueNewNumber(str);
 }
 
 
 virJSONValuePtr
 virJSONValueNewNumberDouble(double data)
 {
-virJSONValuePtr val = NULL;
-char *str;
+VIR_AUTOFREE(char *) str = NULL;
 if (virDoubleToStr(, data) < 0)
 return NULL;
-val = virJSONValueNewNumber(str);
-VIR_FREE(str);
-return val;
+return virJSONValueNewNumber(str);
 }
 
 
@@ -1171,10 +1156,9 @@ int
 virJSONValueGetArrayAsBitmap(const virJSONValue *val,
  virBitmapPtr *bitmap)
 {
-int ret = -1;
 virJSONValuePtr elem;
 size_t i;
-unsigned long long *elems = NULL;
+VIR_AUTOFREE(unsigned long long *) elems = NULL;
 unsigned long long maxelem = 0;
 
 *bitmap = NULL;
@@ -1191,25 +1175,20 @@ virJSONValueGetArrayAsBitmap(const virJSONValue *val,
 
 if (elem->type != VIR_JSON_TYPE_NUMBER ||
 virStrToLong_ullp(elem->data.number, NULL, 10, [i]) < 0)
-goto cleanup;
+return -1;
 
 if (elems[i] > maxelem)
 maxelem = elems[i];
 }
 
 if (!(*bitmap = virBitmapNewQuiet(maxelem + 1)))
-goto cleanup;
+return -1;
 
 /* second pass sets the correct bits in the map */
 for (i = 0; i < val->data.array.nvalues; i++)
 ignore_value(virBitmapSetBit(*bitmap, elems[i]));
 
-ret = 0;
-
- cleanup:
-VIR_FREE(elems);
-
-return ret;
+return 0;
 }
 
 
-- 
1.8.3.1

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

[libvirt] [PATCH v4 15/31] util: auth: remove redundant include directive

[Sukrit Bhatnagar]

viralloc.h is pulled in by virauthconfig.h

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virauth.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/virauth.c b/src/util/virauth.c
index adb093e..c6a2ce7 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -26,7 +26,6 @@
 
 #include "virauth.h"
 #include "virutil.h"
-#include "viralloc.h"
 #include "virlog.h"
 #include "datatypes.h"
 #include "virerror.h"
-- 
1.8.3.1

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

[libvirt] [PATCH v4 28/31] util: fcp: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virfcp.c | 20 ++--
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/src/util/virfcp.c b/src/util/virfcp.c
index 7660ba7..b703744 100644
--- a/src/util/virfcp.c
+++ b/src/util/virfcp.c
@@ -40,16 +40,12 @@
 bool
 virFCIsCapableRport(const char *rport)
 {
-bool ret = false;
-char *path = NULL;
+VIR_AUTOFREE(char *) path = NULL;
 
 if (virBuildPath(, SYSFS_FC_RPORT_PATH, rport) < 0)
 return false;
 
-ret = virFileExists(path);
-VIR_FREE(path);
-
-return ret;
+return virFileExists(path);
 }
 
 int
@@ -57,8 +53,8 @@ virFCReadRportValue(const char *rport,
 const char *entry,
 char **result)
 {
-int ret = -1;
-char *buf = NULL, *p = NULL;
+VIR_AUTOFREE(char *) buf = NULL;
+char *p = NULL;
 
 if (virFileReadValueString(, "%s/%s/%s",
SYSFS_FC_RPORT_PATH, rport, entry) < 0) {
@@ -69,13 +65,9 @@ virFCReadRportValue(const char *rport,
 *p = '\0';
 
 if (VIR_STRDUP(*result, buf) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
-
- cleanup:
-VIR_FREE(buf);
-return ret;
+return 0;
 }
 
 #else
-- 
1.8.3.1

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

[libvirt] [PATCH v4 26/31] util: arptable: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virarptable.c | 14 +++---
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/src/util/virarptable.c b/src/util/virarptable.c
index c0e90dc..04a6f35 100644
--- a/src/util/virarptable.c
+++ b/src/util/virarptable.c
@@ -71,9 +71,8 @@ virArpTableGet(void)
 {
 int num = 0;
 int msglen;
-void *nlData = NULL;
+VIR_AUTOFREE(void *) nlData = NULL;
 virArpTablePtr table = NULL;
-char *ipstr = NULL;
 struct nlmsghdr* nh;
 struct rtattr * tb[NDA_MAX+1];
 
@@ -108,7 +107,7 @@ virArpTableGet(void)
 continue;
 
 if (nh->nlmsg_type == NLMSG_DONE)
-goto end_of_netlink_messages;
+return table;
 
 VIR_WARNINGS_NO_CAST_ALIGN
 parse_rtattr(tb, NDA_MAX, NDA_RTA(r),
@@ -119,6 +118,7 @@ virArpTableGet(void)
 continue;
 
 if (tb[NDA_DST]) {
+VIR_AUTOFREE(char *) ipstr = NULL;
 virSocketAddr virAddr;
 if (VIR_REALLOC_N(table->t, num + 1) < 0)
 goto cleanup;
@@ -134,8 +134,6 @@ virArpTableGet(void)
 
 if (VIR_STRDUP(table->t[num].ipaddr, ipstr) < 0)
 goto cleanup;
-
-VIR_FREE(ipstr);
 }
 
 if (tb[NDA_LLADDR]) {
@@ -154,14 +152,8 @@ virArpTableGet(void)
 }
 }
 
- end_of_netlink_messages:
-VIR_FREE(nlData);
-return table;
-
  cleanup:
 virArpTableFree(table);
-VIR_FREE(ipstr);
-VIR_FREE(nlData);
 return NULL;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH v4 29/31] util: eventpoll: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/vireventpoll.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
index 81ecab4..13d278d 100644
--- a/src/util/vireventpoll.c
+++ b/src/util/vireventpoll.c
@@ -618,7 +618,7 @@ static void virEventPollCleanupHandles(void)
  */
 int virEventPollRunOnce(void)
 {
-struct pollfd *fds = NULL;
+VIR_AUTOFREE(struct pollfd *) fds = NULL;
 int ret, timeout, nfds;
 
 virMutexLock();
@@ -645,7 +645,7 @@ int virEventPollRunOnce(void)
 goto retry;
 virReportSystemError(errno, "%s",
  _("Unable to poll on file handles"));
-goto error_unlocked;
+return -1;
 }
 EVENT_DEBUG("Poll got %d event(s)", ret);
 
@@ -662,13 +662,10 @@ int virEventPollRunOnce(void)
 
 eventLoop.running = 0;
 virMutexUnlock();
-VIR_FREE(fds);
 return 0;
 
  error:
 virMutexUnlock();
- error_unlocked:
-VIR_FREE(fds);
 return -1;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH v4 27/31] util: audit: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/viraudit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/viraudit.c b/src/util/viraudit.c
index 0085dc3..a49d458 100644
--- a/src/util/viraudit.c
+++ b/src/util/viraudit.c
@@ -97,7 +97,7 @@ void virAuditSend(virLogSourcePtr source,
   virAuditRecordType type ATTRIBUTE_UNUSED, bool success,
   const char *fmt, ...)
 {
-char *str = NULL;
+VIR_AUTOFREE(char *) str = NULL;
 va_list args;
 
 /* Duplicate later checks, to short circuit & avoid printf overhead
@@ -144,7 +144,6 @@ void virAuditSend(virLogSourcePtr source,
 }
 }
 #endif
-VIR_FREE(str);
 }
 
 void virAuditClose(void)
-- 
1.8.3.1

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

[libvirt] [PATCH v4 30/31] util: filecache: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virfilecache.c | 35 +++
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c
index 96ae96d..2927c68 100644
--- a/src/util/virfilecache.c
+++ b/src/util/virfilecache.c
@@ -100,18 +100,17 @@ static char *
 virFileCacheGetFileName(virFileCachePtr cache,
 const char *name)
 {
-char *file = NULL;
-char *namehash = NULL;
+VIR_AUTOFREE(char *) namehash = NULL;
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 
 if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, ) < 0)
-goto cleanup;
+return NULL;
 
 if (virFileMakePath(cache->dir) < 0) {
 virReportSystemError(errno,
  _("Unable to create directory '%s'"),
  cache->dir);
-goto cleanup;
+return NULL;
 }
 
 virBufferAsprintf(, "%s/%s", cache->dir, namehash);
@@ -120,13 +119,9 @@ virFileCacheGetFileName(virFileCachePtr cache,
 virBufferAsprintf(, ".%s", cache->suffix);
 
 if (virBufferCheckError() < 0)
-goto cleanup;
+return NULL;
 
-file = virBufferContentAndReset();
-
- cleanup:
-VIR_FREE(namehash);
-return file;
+return virBufferContentAndReset();
 }
 
 
@@ -135,7 +130,7 @@ virFileCacheLoad(virFileCachePtr cache,
  const char *name,
  void **data)
 {
-char *file = NULL;
+VIR_AUTOFREE(char *) file = NULL;
 int ret = -1;
 void *loadData = NULL;
 
@@ -178,7 +173,6 @@ virFileCacheLoad(virFileCachePtr cache,
 
  cleanup:
 virObjectUnref(loadData);
-VIR_FREE(file);
 return ret;
 }
 
@@ -188,20 +182,15 @@ virFileCacheSave(virFileCachePtr cache,
  const char *name,
  void *data)
 {
-char *file = NULL;
-int ret = -1;
+VIR_AUTOFREE(char *) file = NULL;
 
 if (!(file = virFileCacheGetFileName(cache, name)))
-return ret;
+return -1;
 
 if (cache->handlers.saveFile(data, file, cache->priv) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
-
- cleanup:
-VIR_FREE(file);
-return ret;
+return 0;
 }
 
 
@@ -346,7 +335,7 @@ virFileCacheLookupByFunc(virFileCachePtr cache,
  const void *iterData)
 {
 void *data = NULL;
-char *name = NULL;
+VIR_AUTOFREE(char *) name = NULL;
 
 virObjectLock(cache);
 
@@ -356,8 +345,6 @@ virFileCacheLookupByFunc(virFileCachePtr cache,
 virObjectRef(data);
 virObjectUnlock(cache);
 
-VIR_FREE(name);
-
 return data;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH v4 07/31] util: command: use VIR_AUTOPTR for aggregate types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/vircommand.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index d328431..8ba9c35 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -822,12 +822,9 @@ virExec(virCommandPtr cmd)
 int
 virRun(const char *const*argv, int *status)
 {
-int ret;
-virCommandPtr cmd = virCommandNewArgs(argv);
+VIR_AUTOPTR(virCommand) cmd = virCommandNewArgs(argv);
 
-ret = virCommandRun(cmd, status);
-virCommandFree(cmd);
-return ret;
+return virCommandRun(cmd, status);
 }
 
 #else /* WIN32 */
@@ -2960,7 +2957,7 @@ virCommandRunRegex(virCommandPtr cmd,
 int totgroups = 0, ngroup = 0, maxvars = 0;
 char **groups;
 VIR_AUTOFREE(char *) outbuf = NULL;
-char **lines = NULL;
+VIR_AUTOPTR(virString) lines = NULL;
 int ret = -1;
 
 /* Compile all regular expressions */
@@ -3039,7 +3036,6 @@ virCommandRunRegex(virCommandPtr cmd,
 
 ret = 0;
  cleanup:
-virStringListFree(lines);
 if (groups) {
 for (j = 0; j < totgroups; j++)
 VIR_FREE(groups[j]);
-- 
1.8.3.1

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

[libvirt] [PATCH v4 09/31] util: file: remove redundant include directive

[Sukrit Bhatnagar]

viralloc.h is pulled in by virfile.h

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virfile.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 378d03e..2690e2d 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -67,7 +67,6 @@
 
 #include "configmake.h"
 #include "intprops.h"
-#include "viralloc.h"
 #include "vircommand.h"
 #include "virerror.h"
 #include "virfile.h"
-- 
1.8.3.1

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

[libvirt] [PATCH v4 11/31] util: file: use VIR_AUTOPTR for aggregate types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virfile.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 3a7445f..6b94885 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -888,20 +888,19 @@ int virFileNBDDeviceAssociate(const char *file,
 {
 VIR_AUTOFREE(char *) nbddev = NULL;
 VIR_AUTOFREE(char *) qemunbd = NULL;
-virCommandPtr cmd = NULL;
-int ret = -1;
+VIR_AUTOPTR(virCommand) cmd = NULL;
 const char *fmtstr = NULL;
 
 if (!virFileNBDLoadDriver())
-goto cleanup;
+return -1;
 
 if (!(nbddev = virFileNBDDeviceFindUnused()))
-goto cleanup;
+return -1;
 
 if (!(qemunbd = virFindFileInPath("qemu-nbd"))) {
 virReportSystemError(ENOENT, "%s",
  _("Unable to find 'qemu-nbd' binary in $PATH"));
-goto cleanup;
+return -1;
 }
 
 if (fmt > 0)
@@ -926,17 +925,14 @@ int virFileNBDDeviceAssociate(const char *file,
 /* qemu-nbd will daemonize itself */
 
 if (virCommandRun(cmd, NULL) < 0)
-goto cleanup;
+return -1;
 
 VIR_DEBUG("Associated NBD device %s with file %s and format %s",
   nbddev, file, fmtstr);
 *dev = nbddev;
 nbddev = NULL;
-ret = 0;
 
- cleanup:
-virCommandFree(cmd);
-return ret;
+return 0;
 }
 
 #else /* __linux__ */
-- 
1.8.3.1

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

[libvirt] [PATCH v4 21/31] util: json: use VIR_AUTOPTR for aggregate types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virjson.c | 18 +-
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/src/util/virjson.c b/src/util/virjson.c
index 82f539f..29530dc 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1786,7 +1786,7 @@ virJSONValueFromString(const char *jsonstring)
 size_t len = strlen(jsonstring);
 # ifndef WITH_YAJL2
 yajl_parser_config cfg = { 0, 1 }; /* Match yajl 2 default behavior */
-virJSONValuePtr tmp;
+VIR_AUTOPTR(virJSONValue) tmp = NULL;
 # endif
 
 VIR_DEBUG("string=%s", jsonstring);
@@ -1850,7 +1850,6 @@ virJSONValueFromString(const char *jsonstring)
jsonstring);
 else
 ret = virJSONValueArraySteal(tmp, 0);
-virJSONValueFree(tmp);
 # endif
 }
 
@@ -2023,16 +2022,12 @@ char *
 virJSONStringReformat(const char *jsonstr,
   bool pretty)
 {
-virJSONValuePtr json;
-char *ret;
+VIR_AUTOPTR(virJSONValue) json = NULL;
 
 if (!(json = virJSONValueFromString(jsonstr)))
 return NULL;
 
-ret = virJSONValueToString(json, pretty);
-
-virJSONValueFree(json);
-return ret;
+return virJSONValueToString(json, pretty);
 }
 
 
@@ -2121,7 +2116,7 @@ virJSONValueObjectDeflattenWorker(const char *key,
 virJSONValuePtr
 virJSONValueObjectDeflatten(virJSONValuePtr json)
 {
-virJSONValuePtr deflattened;
+VIR_AUTOPTR(virJSONValue) deflattened = NULL;
 virJSONValuePtr ret = NULL;
 
 if (!(deflattened = virJSONValueNewObject()))
@@ -2130,12 +2125,9 @@ virJSONValueObjectDeflatten(virJSONValuePtr json)
 if (virJSONValueObjectForeachKeyValue(json,
   virJSONValueObjectDeflattenWorker,
   deflattened) < 0)
-goto cleanup;
+return NULL;
 
 VIR_STEAL_PTR(ret, deflattened);
 
- cleanup:
-virJSONValueFree(deflattened);
-
 return ret;
 }
-- 
1.8.3.1

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

[libvirt] [PATCH v4 22/31] util: bitmap: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Sukrit Bhatnagar]

Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope.

When a variable of type virBitmapPtr is declared using
VIR_AUTOPTR, the function virBitmapFree will be run
automatically on it when it goes out of scope.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virbitmap.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index 2464814..312e7e2 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -25,6 +25,7 @@
 # define __BITMAP_H__
 
 # include "internal.h"
+# include "viralloc.h"
 
 # include 
 
@@ -155,4 +156,6 @@ void virBitmapSubtract(virBitmapPtr a, virBitmapPtr b)
 
 void virBitmapShrink(virBitmapPtr map, size_t b);
 
+VIR_DEFINE_AUTOPTR_FUNC(virBitmap, virBitmapFree)
+
 #endif
-- 
1.8.3.1

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

[libvirt] [PATCH v4 10/31] util: file: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virfile.c | 310 ++---
 1 file changed, 102 insertions(+), 208 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 2690e2d..3a7445f 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -211,7 +211,7 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int 
flags)
 bool output = false;
 int pipefd[2] = { -1, -1 };
 int mode = -1;
-char *iohelper_path = NULL;
+VIR_AUTOFREE(char *) iohelper_path = NULL;
 
 if (!flags) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -262,8 +262,6 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int 
flags)
 
 ret->cmd = virCommandNewArgList(iohelper_path, name, NULL);
 
-VIR_FREE(iohelper_path);
-
 if (output) {
 virCommandSetInputFD(ret->cmd, pipefd[0]);
 virCommandSetOutputFD(ret->cmd, fd);
@@ -294,7 +292,6 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int 
flags)
 return ret;
 
  error:
-VIR_FREE(iohelper_path);
 VIR_FORCE_CLOSE(pipefd[0]);
 VIR_FORCE_CLOSE(pipefd[1]);
 virFileWrapperFdFree(ret);
@@ -492,7 +489,7 @@ virFileRewrite(const char *path,
virFileRewriteFunc rewrite,
const void *opaque)
 {
-char *newfile = NULL;
+VIR_AUTOFREE(char *) newfile = NULL;
 int fd = -1;
 int ret = -1;
 
@@ -533,10 +530,8 @@ virFileRewrite(const char *path,
 
  cleanup:
 VIR_FORCE_CLOSE(fd);
-if (newfile) {
+if (newfile)
 unlink(newfile);
-VIR_FREE(newfile);
-}
 return ret;
 }
 
@@ -763,7 +758,7 @@ int virFileLoopDeviceAssociate(const char *file,
 int lofd = -1;
 int fsfd = -1;
 struct loop_info64 lo;
-char *loname = NULL;
+VIR_AUTOFREE(char *) loname = NULL;
 int ret = -1;
 
 if ((lofd = virFileLoopDeviceOpen()) < 0)
@@ -801,7 +796,6 @@ int virFileLoopDeviceAssociate(const char *file,
 ret = 0;
 
  cleanup:
-VIR_FREE(loname);
 VIR_FORCE_CLOSE(fsfd);
 if (ret == -1)
 VIR_FORCE_CLOSE(lofd);
@@ -816,8 +810,7 @@ int virFileLoopDeviceAssociate(const char *file,
 static int
 virFileNBDDeviceIsBusy(const char *dev_name)
 {
-char *path;
-int ret = -1;
+VIR_AUTOFREE(char *) path = NULL;
 
 if (virAsprintf(, SYSFS_BLOCK_DIR "/%s/pid",
 dev_name) < 0)
@@ -825,18 +818,14 @@ virFileNBDDeviceIsBusy(const char *dev_name)
 
 if (!virFileExists(path)) {
 if (errno == ENOENT)
-ret = 0;
+return 0;
 else
 virReportSystemError(errno,
  _("Cannot check NBD device %s pid"),
  dev_name);
-goto cleanup;
+return -1;
 }
-ret = 1;
-
- cleanup:
-VIR_FREE(path);
-return ret;
+return 1;
 }
 
 
@@ -881,15 +870,13 @@ virFileNBDLoadDriver(void)
  "administratively prohibited"));
 return false;
 } else {
-char *errbuf = NULL;
+VIR_AUTOFREE(char *) errbuf = NULL;
 
 if ((errbuf = virKModLoad(NBD_DRIVER, true))) {
-VIR_FREE(errbuf);
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Failed to load nbd module"));
 return false;
 }
-VIR_FREE(errbuf);
 }
 return true;
 }
@@ -899,8 +886,8 @@ int virFileNBDDeviceAssociate(const char *file,
   bool readonly,
   char **dev)
 {
-char *nbddev = NULL;
-char *qemunbd = NULL;
+VIR_AUTOFREE(char *) nbddev = NULL;
+VIR_AUTOFREE(char *) qemunbd = NULL;
 virCommandPtr cmd = NULL;
 int ret = -1;
 const char *fmtstr = NULL;
@@ -948,8 +935,6 @@ int virFileNBDDeviceAssociate(const char *file,
 ret = 0;
 
  cleanup:
-VIR_FREE(nbddev);
-VIR_FREE(qemunbd);
 virCommandFree(cmd);
 return ret;
 }
@@ -996,7 +981,6 @@ int virFileDeleteTree(const char *dir)
 {
 DIR *dh;
 struct dirent *de;
-char *filepath = NULL;
 int ret = -1;
 int direrr;
 
@@ -1008,6 +992,7 @@ int virFileDeleteTree(const char *dir)
 return -1;
 
 while ((direrr = virDirRead(dh, , dir)) > 0) {
+VIR_AUTOFREE(char *) filepath = NULL;
 struct stat sb;
 
 if (virAsprintf(, "%s/%s",
@@ -1031,8 +1016,6 @@ int virFileDeleteTree(const char *dir)
 goto cleanup;
 }
 }
-
-VIR_FREE(filepath);
 }
 if (direrr < 0)
 goto cleanup;
@@ -1047,7 +1030,6 @@ int virFileDeleteTree(const char *dir)
 ret = 0;
 
  cleanup:
-VIR_FREE(filepath);
 VIR_DIR_CLOSE(dh);
 return ret;
 }
@@ 

[libvirt] [PATCH v4 08/31] util: file: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Sukrit Bhatnagar]

Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope.

When a variable of type virFileWrapperFdPtr is declared using
VIR_AUTOPTR, the function virFileWrapperFdFree will be run
automatically on it when it goes out of scope.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virfile.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util/virfile.h b/src/util/virfile.h
index 6f1e802..b30a1d3 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -32,6 +32,7 @@
 # include "internal.h"
 # include "virbitmap.h"
 # include "virstoragefile.h"
+# include "viralloc.h"
 
 typedef enum {
 VIR_FILE_CLOSE_PRESERVE_ERRNO = 1 << 0,
@@ -367,4 +368,6 @@ int virFileInData(int fd,
   int *inData,
   long long *length);
 
+VIR_DEFINE_AUTOPTR_FUNC(virFileWrapperFd, virFileWrapperFdFree)
+
 #endif /* __VIR_FILE_H */
-- 
1.8.3.1

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

[libvirt] [PATCH v4 17/31] util: auth: use VIR_AUTOPTR for aggregate types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virauth.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/util/virauth.c b/src/util/virauth.c
index d3a5cc7..8c450b6 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -111,8 +111,7 @@ virAuthGetCredential(const char *servicename,
  const char *path,
  char **value)
 {
-int ret = -1;
-virAuthConfigPtr config = NULL;
+VIR_AUTOPTR(virAuthConfig) config = NULL;
 const char *tmp;
 
 *value = NULL;
@@ -121,23 +120,19 @@ virAuthGetCredential(const char *servicename,
 return 0;
 
 if (!(config = virAuthConfigNew(path)))
-goto cleanup;
+return -1;
 
 if (virAuthConfigLookup(config,
 servicename,
 hostname,
 credname,
 ) < 0)
-goto cleanup;
+return -1;
 
 if (VIR_STRDUP(*value, tmp) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
-
- cleanup:
-virAuthConfigFree(config);
-return ret;
+return 0;
 }
 
 
-- 
1.8.3.1

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

[libvirt] [PATCH v4 16/31] util: auth: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virauth.c | 45 +
 1 file changed, 13 insertions(+), 32 deletions(-)

diff --git a/src/util/virauth.c b/src/util/virauth.c
index c6a2ce7..d3a5cc7 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -41,10 +41,9 @@ int
 virAuthGetConfigFilePathURI(virURIPtr uri,
 char **path)
 {
-int ret = -1;
 size_t i;
 const char *authenv = virGetEnvBlockSUID("LIBVIRT_AUTH_FILE");
-char *userdir = NULL;
+VIR_AUTOFREE(char *) userdir = NULL;
 
 *path = NULL;
 
@@ -53,7 +52,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
 if (authenv) {
 VIR_DEBUG("Using path from env '%s'", authenv);
 if (VIR_STRDUP(*path, authenv) < 0)
-goto cleanup;
+return -1;
 return 0;
 }
 
@@ -63,17 +62,17 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
 uri->params[i].value) {
 VIR_DEBUG("Using path from URI '%s'", uri->params[i].value);
 if (VIR_STRDUP(*path, uri->params[i].value) < 0)
-goto cleanup;
+return -1;
 return 0;
 }
 }
 }
 
 if (!(userdir = virGetUserConfigDirectory()))
-goto cleanup;
+return -1;
 
 if (virAsprintf(path, "%s/auth.conf", userdir) < 0)
-goto cleanup;
+return -1;
 
 VIR_DEBUG("Checking for readability of '%s'", *path);
 if (access(*path, R_OK) == 0)
@@ -82,7 +81,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
 VIR_FREE(*path);
 
 if (VIR_STRDUP(*path, SYSCONFDIR "/libvirt/auth.conf") < 0)
-goto cleanup;
+return -1;
 
 VIR_DEBUG("Checking for readability of '%s'", *path);
 if (access(*path, R_OK) == 0)
@@ -91,13 +90,9 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
 VIR_FREE(*path);
 
  done:
-ret = 0;
-
 VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
- cleanup:
-VIR_FREE(userdir);
 
-return ret;
+return 0;
 }
 
 
@@ -155,7 +150,7 @@ virAuthGetUsernamePath(const char *path,
 {
 unsigned int ncred;
 virConnectCredential cred;
-char *prompt;
+VIR_AUTOFREE(char *) prompt = NULL;
 char *ret = NULL;
 
 if (virAuthGetCredential(servicename, hostname, "username", path, ) < 
0)
@@ -192,8 +187,6 @@ virAuthGetUsernamePath(const char *path,
 break;
 }
 
-VIR_FREE(prompt);
-
 return cred.result;
 }
 
@@ -205,18 +198,13 @@ virAuthGetUsername(virConnectPtr conn,
const char *defaultUsername,
const char *hostname)
 {
-char *ret;
-char *path;
+VIR_AUTOFREE(char *) path = NULL;
 
 if (virAuthGetConfigFilePath(conn, ) < 0)
 return NULL;
 
-ret = virAuthGetUsernamePath(path, auth, servicename,
+return virAuthGetUsernamePath(path, auth, servicename,
  defaultUsername, hostname);
-
-VIR_FREE(path);
-
-return ret;
 }
 
 
@@ -229,7 +217,7 @@ virAuthGetPasswordPath(const char *path,
 {
 unsigned int ncred;
 virConnectCredential cred;
-char *prompt;
+VIR_AUTOFREE(char *) prompt = NULL;
 char *ret = NULL;
 
 if (virAuthGetCredential(servicename, hostname, "password", path, ) < 
0)
@@ -263,8 +251,6 @@ virAuthGetPasswordPath(const char *path,
 break;
 }
 
-VIR_FREE(prompt);
-
 return cred.result;
 }
 
@@ -276,15 +262,10 @@ virAuthGetPassword(virConnectPtr conn,
const char *username,
const char *hostname)
 {
-char *ret;
-char *path;
+VIR_AUTOFREE(char *) path = NULL;
 
 if (virAuthGetConfigFilePath(conn, ) < 0)
 return NULL;
 
-ret = virAuthGetPasswordPath(path, auth, servicename, username, hostname);
-
-VIR_FREE(path);
-
-return ret;
+return virAuthGetPasswordPath(path, auth, servicename, username, hostname);
 }
-- 
1.8.3.1

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

[libvirt] [PATCH v4 14/31] util: authconfig: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virauthconfig.c | 34 --
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c
index 3487cc2..4acdf1d 100644
--- a/src/util/virauthconfig.c
+++ b/src/util/virauthconfig.c
@@ -105,10 +105,9 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
 const char *credname,
 const char **value)
 {
-char *authgroup = NULL;
-char *credgroup = NULL;
+VIR_AUTOFREE(char *) authgroup = NULL;
+VIR_AUTOFREE(char *) credgroup = NULL;
 const char *authcred;
-int ret = -1;
 
 *value = NULL;
 
@@ -118,47 +117,38 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
 hostname = "localhost";
 
 if (virAsprintf(, "auth-%s-%s", service, hostname) < 0)
-goto cleanup;
+return -1;
 
 if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
VIR_FREE(authgroup);
if (virAsprintf(, "auth-%s-%s", service, "default") < 0)
-   goto cleanup;
+return -1;
 }
 
-if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
-ret = 0;
-goto cleanup;
-}
+if (!virKeyFileHasGroup(auth->keyfile, authgroup))
+return 0;
 
 if (!(authcred = virKeyFileGetValueString(auth->keyfile, authgroup, 
"credentials"))) {
 virReportError(VIR_ERR_CONF_SYNTAX,
_("Missing item 'credentials' in group '%s' in '%s'"),
authgroup, auth->path);
-goto cleanup;
+return -1;
 }
 
 if (virAsprintf(, "credentials-%s", authcred) < 0)
-goto cleanup;
+return -1;
 
 if (!virKeyFileHasGroup(auth->keyfile, credgroup)) {
 virReportError(VIR_ERR_CONF_SYNTAX,
_("Missing group 'credentials-%s' referenced from group 
'%s' in '%s'"),
authcred, authgroup, auth->path);
-goto cleanup;
+return -1;
 }
 
-if (!virKeyFileHasValue(auth->keyfile, credgroup, credname)) {
-ret = 0;
-goto cleanup;
-}
+if (!virKeyFileHasValue(auth->keyfile, credgroup, credname))
+return 0;
 
 *value = virKeyFileGetValueString(auth->keyfile, credgroup, credname);
 
-ret = 0;
-
- cleanup:
-VIR_FREE(authgroup);
-VIR_FREE(credgroup);
-return ret;
+return 0;
 }
-- 
1.8.3.1

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

[libvirt] [PATCH v4 13/31] util: authconfig: remove redundant include directive

[Sukrit Bhatnagar]

viralloc.h is pulled in by virauthconfig.h

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virauthconfig.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c
index 91c9c0c..3487cc2 100644
--- a/src/util/virauthconfig.c
+++ b/src/util/virauthconfig.c
@@ -25,7 +25,6 @@
 #include "virauthconfig.h"
 
 #include "virkeyfile.h"
-#include "viralloc.h"
 #include "virlog.h"
 #include "virerror.h"
 #include "virstring.h"
-- 
1.8.3.1

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

[libvirt] [PATCH v4 04/31] util: command: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Sukrit Bhatnagar]

Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope.

When a variable of type virCommandPtr is declared using VIR_AUTOPTR,
the function virCommandFree will be run automatically on it when it
goes out of scope.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/vircommand.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index 883e212..90bcc6c 100644
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -24,6 +24,7 @@
 
 # include "internal.h"
 # include "virbuffer.h"
+# include "viralloc.h"
 
 typedef struct _virCommand virCommand;
 typedef virCommand *virCommandPtr;
@@ -218,5 +219,6 @@ int virCommandRunNul(virCommandPtr cmd,
  virCommandRunNulFunc func,
  void *data);
 
+VIR_DEFINE_AUTOPTR_FUNC(virCommand, virCommandFree)
 
 #endif /* __VIR_COMMAND_H__ */
-- 
1.8.3.1

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

[libvirt] [PATCH v4 12/31] util: authconfig: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Sukrit Bhatnagar]

Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope.

When a variable of type virAuthConfigPtr is declared using
VIR_AUTOPTR, the function virAuthConfigFree will be run
automatically on it when it goes out of scope.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virauthconfig.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util/virauthconfig.h b/src/util/virauthconfig.h
index ac0ceeb..d8a3849 100644
--- a/src/util/virauthconfig.h
+++ b/src/util/virauthconfig.h
@@ -24,6 +24,7 @@
 # define __VIR_AUTHCONFIG_H__
 
 # include "internal.h"
+# include "viralloc.h"
 
 typedef struct _virAuthConfig virAuthConfig;
 typedef virAuthConfig *virAuthConfigPtr;
@@ -42,4 +43,6 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
 const char *credname,
 const char **value);
 
+VIR_DEFINE_AUTOPTR_FUNC(virAuthConfig, virAuthConfigFree)
+
 #endif /* __VIR_AUTHCONFIG_H__ */
-- 
1.8.3.1

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

[libvirt] [PATCH v4 06/31] util: command: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

By making use of GNU C's cleanup attribute handled by the
VIR_AUTOFREE macro for declaring scalar variables, majority
of the VIR_FREE calls can be dropped, which in turn leads to
getting rid of most of our cleanup sections.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/vircommand.c | 40 
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 8681e7b..d328431 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -507,11 +507,11 @@ virExec(virCommandPtr cmd)
 int childout = -1;
 int childerr = -1;
 int tmpfd;
-char *binarystr = NULL;
+VIR_AUTOFREE(char *) binarystr = NULL;
 const char *binary = NULL;
 int ret;
 struct sigaction waxon, waxoff;
-gid_t *groups = NULL;
+VIR_AUTOFREE(gid_t *) groups = NULL;
 int ngroups;
 
 if (cmd->args[0][0] != '/') {
@@ -604,9 +604,6 @@ virExec(virCommandPtr cmd)
 
 cmd->pid = pid;
 
-VIR_FREE(groups);
-VIR_FREE(binarystr);
-
 return 0;
 }
 
@@ -796,9 +793,6 @@ virExec(virCommandPtr cmd)
 /* This is cleanup of parent process only - child
should never jump here on error */
 
-VIR_FREE(binarystr);
-VIR_FREE(groups);
-
 /* NB we don't virReportError() on any failures here
because the code which jumped here already raised
an error condition which we must not overwrite */
@@ -2386,7 +2380,7 @@ int
 virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
 {
 int ret = -1;
-char *str = NULL;
+VIR_AUTOFREE(char *) str = NULL;
 size_t i;
 bool synchronous = false;
 int infd[2] = {-1, -1};
@@ -2511,7 +2505,6 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
 VIR_FORCE_CLOSE(cmd->infd);
 VIR_FORCE_CLOSE(cmd->inpipe);
 }
-VIR_FREE(str);
 return ret;
 }
 
@@ -2588,8 +2581,8 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
 if (exitstatus && (cmd->rawStatus || WIFEXITED(status))) {
 *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status);
 } else if (status) {
-char *str = virCommandToString(cmd);
-char *st = virProcessTranslateStatus(status);
+VIR_AUTOFREE(char *) str = virCommandToString(cmd);
+VIR_AUTOFREE(char *) st = virProcessTranslateStatus(status);
 bool haveErrMsg = cmd->errbuf && *cmd->errbuf && (*cmd->errbuf)[0];
 
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2597,8 +2590,6 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
str ? str : cmd->args[0], NULLSTR(st),
haveErrMsg ? ": " : "",
haveErrMsg ? *cmd->errbuf : "");
-VIR_FREE(str);
-VIR_FREE(st);
 return -1;
 }
 }
@@ -2718,7 +2709,7 @@ int virCommandHandshakeWait(virCommandPtr cmd)
 return -1;
 }
 if (c != '1') {
-char *msg;
+VIR_AUTOFREE(char *) msg = NULL;
 ssize_t len;
 if (VIR_ALLOC_N(msg, 1024) < 0) {
 VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
@@ -2731,7 +2722,6 @@ int virCommandHandshakeWait(virCommandPtr cmd)
 
 if ((len = saferead(cmd->handshakeWait[0], msg, 1024)) < 0) {
 VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
-VIR_FREE(msg);
 virReportSystemError(errno, "%s",
  _("No error message from child failure"));
 return -1;
@@ -2739,7 +2729,6 @@ int virCommandHandshakeWait(virCommandPtr cmd)
 VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
 msg[len-1] = '\0';
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s", msg);
-VIR_FREE(msg);
 return -1;
 }
 VIR_FORCE_CLOSE(cmd->handshakeWait[0]);
@@ -2853,8 +2842,8 @@ virCommandFree(virCommandPtr cmd)
  * This requests asynchronous string IO on @cmd. It is useful in
  * combination with virCommandRunAsync():
  *
- *  virCommandPtr cmd = virCommandNew*(...);
- *  char *buf = NULL;
+ *  VIR_AUTOPTR(virCommand) cmd = virCommandNew*(...);
+ *  VIR_AUTOFREE(char *) buf = NULL;
  *
  *  ...
  *
@@ -2862,21 +2851,18 @@ virCommandFree(virCommandPtr cmd)
  *  virCommandDoAsyncIO(cmd);
  *
  *  if (virCommandRunAsync(cmd, NULL) < 0)
- *  goto cleanup;
+ *  return;
  *
  *  ...
  *
  *  if (virCommandWait(cmd, NULL) < 0)
- *  goto cleanup;
+ *  return;
  *
  *  // @buf now contains @cmd's stdout
  *  VIR_DEBUG("STDOUT: %s", NULLSTR(buf));
  *
  *  ...
  *
- *  cleanup:
- *  VIR_FREE(buf);
- *  virCommandFree(cmd);
  *
  * The libvirt's event loop is used for handling stdios of @cmd.
  * Since current implementation uses strlen to determine length
@@ -2969,11 +2955,11 @@ virCommandRunRegex(virCommandPtr cmd,
 {
 int err;
 regex_t *reg;
-regmatch_t *vars = NULL;
+

[libvirt] [PATCH v4 05/31] util: command: remove redundant include directive

[Sukrit Bhatnagar]

viralloc.h is pulled in by vircommand.h

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/vircommand.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 6dab105..8681e7b 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -44,7 +44,6 @@
 
 #define __VIR_COMMAND_PRIV_H_ALLOW__
 #include "vircommandpriv.h"
-#include "viralloc.h"
 #include "virerror.h"
 #include "virutil.h"
 #include "virlog.h"
-- 
1.8.3.1

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

[libvirt] [PATCH v4 03/31] util: string: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

[Sukrit Bhatnagar]

Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in
src/util/viralloc.h, define a new wrapper around an existing
cleanup function which will be called when a variable declared
with VIR_AUTOPTR macro goes out of scope.

Alias virString to (char *) so that the new cleanup macros
can be used for a list of strings (char **).

When a list of strings (virString *) is declared using VIR_AUTOPTR,
the function virStringListFree will be run automatically on it when
it goes out of scope.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/virstring.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/util/virstring.h b/src/util/virstring.h
index 607ae66..726e02b 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -25,6 +25,9 @@
 # include 
 
 # include "internal.h"
+# include "viralloc.h"
+
+typedef char *virString;
 
 char **virStringSplitCount(const char *string,
const char *delim,
@@ -309,4 +312,6 @@ int virStringParsePort(const char *str,
unsigned int *port)
 ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
 
+VIR_DEFINE_AUTOPTR_FUNC(virString, virStringListFree)
+
 #endif /* __VIR_STRING_H__ */
-- 
1.8.3.1

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

[libvirt] [PATCH v4 01/31] util: alloc: add macros for implementing automatic cleanup functionality

[Sukrit Bhatnagar]

New macros are introduced which help in adding GNU C's cleanup
attribute to variable declarations. Variables declared with these
macros will have their allocated memory freed automatically when
they go out of scope.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 src/util/viralloc.h | 42 ++
 1 file changed, 42 insertions(+)

diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index 69d0f90..a23aa18 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -596,4 +596,46 @@ void virAllocTestInit(void);
 int virAllocTestCount(void);
 void virAllocTestOOM(int n, int m);
 void virAllocTestHook(void (*func)(int, void*), void *data);
+
+# define VIR_AUTOPTR_FUNC_NAME(type) type##AutoPtrFree
+
+/**
+ * VIR_DEFINE_AUTOPTR_FUNC:
+ * @type: type of the variable to be freed automatically
+ * @func: cleanup function to be automatically called
+ *
+ * This macro defines a function for automatic freeing of
+ * resources allocated to a variable of type @type. This newly
+ * defined function works as a necessary wrapper around @func.
+ */
+# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
+static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
+{ \
+if (*_ptr) \
+(func)(*_ptr); \
+*_ptr = NULL; \
+} \
+
+/**
+ * VIR_AUTOFREE:
+ * @type: type of the variable to be freed automatically
+ *
+ * Macro to automatically free the memory allocated to
+ * the variable declared with it by calling virFree
+ * when the variable goes out of scope.
+ */
+# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
+
+/**
+ * VIR_AUTOPTR:
+ * @type: type of the variable to be freed automatically
+ *
+ * Macro to automatically free the memory allocated to
+ * the variable declared with it by calling the function
+ * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
+ * goes out of scope.
+ */
+# define VIR_AUTOPTR(type) \
+__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type *
+
 #endif /* __VIR_MEMORY_H_ */
-- 
1.8.3.1

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

[libvirt] [PATCH v4 00/31] use GNU C's cleanup attribute in src/util (batch I)

[Sukrit Bhatnagar]

This series of patches first introduces a new set of macros which help
in using GNU C's __attribute__((cleanup)) in the code.

Then a few syntax-check rules are added which help in ensuring correct
usage of the newly introduced cleanup macros.

Then the patches modify a few files in src/util to use VIR_AUTOFREE
and VIR_AUTOPTR for automatic freeing of memory and get rid of some
VIR_FREE macro invocations and *Free function calls.

Sukrit Bhatnagar (31):
  util: alloc: add macros for implementing automatic cleanup
functionality
  cfg.mk: variable initialization when declared with cleanup macro
  util: string: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
  util: command: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
  util: command: remove redundant include directive
  util: command: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: command: use VIR_AUTOPTR for aggregate types
  util: file: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
  util: file: remove redundant include directive
  util: file: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: file: use VIR_AUTOPTR for aggregate types
  util: authconfig: define cleanup function using
VIR_DEFINE_AUTOPTR_FUNC
  util: authconfig: remove redundant include directive
  util: authconfig: use VIR_AUTOFREE instead of VIR_FREE for scalar
types
  util: auth: remove redundant include directive
  util: auth: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: auth: use VIR_AUTOPTR for aggregate types
  util: json: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
  util: json: remove redundant include directive
  util: json: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: json: use VIR_AUTOPTR for aggregate types
  util: bitmap: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC
  util: bitmap: remove redundant include directive
  util: bitmap: use VIR_AUTOPTR for aggregate types
  util: iohelper: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: arptable: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: audit: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: fcp: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: eventpoll: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: filecache: use VIR_AUTOFREE instead of VIR_FREE for scalar types
  util: identity: use VIR_AUTOFREE instead of VIR_FREE for scalar types

 cfg.mk   |  11 ++
 src/util/iohelper.c  |   4 +-
 src/util/viralloc.h  |  42 ++
 src/util/virarptable.c   |  14 +-
 src/util/viraudit.c  |   3 +-
 src/util/virauth.c   |  61 +++--
 src/util/virauthconfig.c |  35 ++---
 src/util/virauthconfig.h |   3 +
 src/util/virbitmap.c |   8 +-
 src/util/virbitmap.h |   3 +
 src/util/vircommand.c|  51 +++-
 src/util/vircommand.h|   2 +
 src/util/vireventpoll.c  |   7 +-
 src/util/virfcp.c|  20 +--
 src/util/virfile.c   | 327 ---
 src/util/virfile.h   |   3 +
 src/util/virfilecache.c  |  35 ++---
 src/util/viridentity.c   |  52 
 src/util/virjson.c   |  68 +++---
 src/util/virjson.h   |   3 +
 src/util/virstring.h |   5 +
 21 files changed, 292 insertions(+), 465 deletions(-)

-- 
1.8.3.1

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

[libvirt] [PATCH v4 02/31] cfg.mk: variable initialization when declared with cleanup macro

[Sukrit Bhatnagar]

A variable, which is never assigned a value in the function, might get
passed into the cleanup function which may or may not raise any errors.

To maintain the correct usage, the variable must be initialized, either
with a value or with NULL. This syntax-check rule takes care of that.

Signed-off-by: Sukrit Bhatnagar 
Reviewed-by: Erik Skultety 
---
 cfg.mk | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/cfg.mk b/cfg.mk
index 6bebd0a..609ae86 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1057,6 +1057,17 @@ sc_prohibit_backslash_alignment:
halt='Do not attempt to right-align backslashes' \
  $(_sc_search_regexp)
 
+# Some syntax rules pertaining to the usage of cleanup macros
+# implementing GNU C's cleanup attribute
+
+# Rule to ensure that varibales declared using a cleanup macro are
+# always initialized.
+sc_require_attribute_cleanup_initialization:
+   @prohibit='VIR_AUTO(FREE|PTR)\(.+\) *[^=]+;' \
+   in_vc_files='\.[chx]$$' \
+   halt='variable declared with a cleanup macro must be initialized' \
+ $(_sc_search_regexp)
+
 # We don't use this feature of maint.mk.
 prev_version_file = /dev/null
 
-- 
1.8.3.1

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

Re: [libvirt] [PATCHv2 00/11] BaselineHypervisorCPU using QEMU QMP exchanges

[Jiri Denemark]

On Mon, Jul 09, 2018 at 22:56:44 -0500, Chris Venteicher wrote:
> Some architectures (S390) depend on QEMU to compute baseline CPU model and
> expand a models feature set.
> 
> Interacting with QEMU requires starting the QEMU process and completing one or
> more query-cpu-model-baseline QMP exchanges with QEMU in addition to a
> query-cpu-model-expansion QMP exchange to expand all features in the model.
> 
> See "s390x CPU models: exposing features" patch set on Qemu-devel for 
> discussion
> of QEMU aspects.
> 
> This is part of resolution of: 
> https://bugzilla.redhat.com/show_bug.cgi?id=1511999
> 
> Version 2 address all issues raised by Collin as well as all other issues
> identified with additional testing.

Thanks for the patches Chris. I think they are going in the right
direction.

BTW, I'll be offline next week so don't expect any reviews/comments from
me before Monday 23.

Jirka

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

Re: [libvirt] [PATCHv2 11/11] qemu_driver: BaselineHypervisorCPU supports S390 using QEMU/QMP

[Jiri Denemark]

On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
> Transient S390 configurations require using QEMU to compute CPU Model
> Baseline and to do CPU Feature Expansion.
> 
> Start and use a single QEMU instance to do both the baseline and
> expansion transactions required by BaselineHypervisorCPU.
> 
> CPU Feature Expansion uses true / false to indicate if property is/isn't
> included in model. Baseline only returns property list where all
> enumerated properties are included.

So are you saying on s390 there's no chance there would be a CPU model
with some feature which is included in the CPU model disabled for some
reason? Sounds too good to be true :-) (This is the question I referred
to in one of my replies to the other patches.)

Most of the code you added in this patch is indented by three spaces
while we use four spaces in libvirt.

> ---
>  src/qemu/qemu_driver.c | 74 +-
>  1 file changed, 65 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9a35e04a85..6c6107f077 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>  virArch arch;
>  virDomainVirtType virttype;
>  virDomainCapsCPUModelsPtr cpuModels;
> -bool migratable;
> +bool migratable_only;

Why? The bool says the user specified
VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable
CPU back. What does the "_only" part mean? This API does not return
several CPUs, it only returns a single one and it's either migratable or
not.

>  virCPUDefPtr cpu = NULL;
>  char *cpustr = NULL;
>  char **features = NULL;
> +virQEMUCapsInitQMPCommandPtr cmd = NULL;
> +bool forceTCG = false;
> +qemuMonitorCPUModelInfoPtr modelInfo = NULL;
>  
>  virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES |
>VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL);
> @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>  if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0)
>  goto cleanup;
>  
> -migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
> -
>  if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO)))
>  goto cleanup;
>  
> @@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>  if (!qemuCaps)
>  goto cleanup;
>  
> +/* QEMU can enumerate non-migratable cpu model features for some archs 
> like x86
> + * migratable_only == true:  ask for and include only migratable features
> + * migratable_only == false: ask for and include all features
> + */
> +migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE);
> +
> +if (ARCH_IS_S390(arch)) {
> +   /* QEMU for S390 arch only enumerates migratable features
> +* No reason to explicitly ask QEMU for or include non-migratable 
> features
> +*/
> +   migratable_only = true;
> +}
> +

And what if they come up with some features which are not migratable in
the future? I don't think there's any reason for this API to mess with
the value. The code should just provide the same CPU in both cases for
s390.

>  if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) ||
>  cpuModels->nmodels == 0) {
>  virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> @@ -13437,18 +13451,31 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
>  
>  if (ARCH_IS_X86(arch)) {
>  int rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype,
> -   migratable, );
> +   migratable_only, );
>  if (rc < 0)
>  goto cleanup;
>  if (features && rc == 0) {
>  /* We got only migratable features from QEMU if we asked for 
> them,
>   * no further filtering in virCPUBaseline is desired. */
> -migratable = false;
> +migratable_only = false;
>  }
>  
>  if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels,
> -   (const char **)features, migratable)))
> +   (const char **)features, 
> migratable_only)))
>  goto cleanup;
> +} else if (ARCH_IS_S390(arch)) {
> +

No need for this extra empty line.

> +   const char *binary = virQEMUCapsGetBinary(qemuCaps);
> +   virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +
> +   if (!(cmd = virQEMUCapsNewQMPCommandConnection(binary, cfg->libDir,
> +  cfg->user, cfg->group,
> +  forceTCG)))
> +  goto cleanup;
> +
> +   if ((virQEMUCapsQMPBaselineCPUModel(cmd, cpus, ) < 0) || !cpu)

Hmm, I think you should only call this when the user passed more than
one CPU. 

Re: [libvirt] [PATCHv2 10/11] qemu_capabilities: Introduce virQEMUCapsQMPBaselineCPUModel (baseline using QEMU)

[Jiri Denemark]

Drop "(baseline using QEMU)" from Subject to make it shorter.

On Mon, Jul 09, 2018 at 22:56:54 -0500, Chris Venteicher wrote:
> Baseline cpu model using QEMU/QMP query-cpu-model-baseline
> 
> query-cpu-model-baseline only compares two CPUModels so multiple
> exchanges are needed to evaluate more than two CPUModels.
> ---
>  src/qemu/qemu_capabilities.c | 85 
>  src/qemu/qemu_capabilities.h |  4 ++
>  2 files changed, 89 insertions(+)

This code has nothing to do with QEMU capabilities, it should be
implemented in qemu_driver.c.

> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 6f8983384a..e0bf78fbba 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -5424,3 +5424,88 @@ virQEMUCapsStripMachineAliases(virQEMUCapsPtr qemuCaps)
>  for (i = 0; i < qemuCaps->nmachineTypes; i++)
>  VIR_FREE(qemuCaps->machineTypes[i].alias);
>  }
> +
> +
> +/* in:
> + *  cpus[0]->model = "z13-base";
> + *  cpus[0]->features[0].name = "xxx";
> + *  cpus[0]->features[1].name = "yyy";
> + *  ***
> + *  cpus[n]->model = "s390x";
> + *  cpus[n]->features[0].name = "xxx";
> + *  cpus[n]->features[1].name = "yyy";
> + *
> + * out:
> + *  *baseline->model = "s390x";
> + *  *baseline->features[0].name = "yyy";
> + *
> + * (ret==0) && (*baseline==NULL) if a QEMU rejects model name or baseline 
> command
> + */
> +int
> +virQEMUCapsQMPBaselineCPUModel(virQEMUCapsInitQMPCommandPtr cmd,
> +   virCPUDefPtr *cpus,
> +   virCPUDefPtr *baseline)
> +{
> +qemuMonitorCPUModelInfoPtr  model_baseline = NULL;
> +qemuMonitorCPUModelInfoPtr  new_model_baseline = NULL;
> +qemuMonitorCPUModelInfoPtr  next_model = NULL;

One space between the type and the variable name would be enough.

> +bool migratable_only = true;
> +int ret = -1;
> +size_t i;
> +
> +*baseline = NULL;
> +
> +if (!cpus || !cpus[0] || !cpus[1]) {
> +virReportError(VIR_ERR_INVALID_ARG, "%s", _("less than 2 cpus"));
> +goto cleanup;
> +}

I guess you're going to special case the single CPU use case in the
caller...

> +
> +for (i = 0; !cpus[i]; i++) {  /* last element in cpus == NULL */

As already mentioned by Collin, this can't really work unless you remove
'!'.

> +virCPUDefPtr cpu = cpus[i];
> +
> +VIR_DEBUG("cpu[%lu]->model = %s", i, NULLSTR(cpu->model));
> +
> +if (!(next_model = virQEMUCapsCPUModelInfoFromCPUDef(cpu))) {
> +virReportError(VIR_ERR_INVALID_ARG, "%s", _("cpu without 
> content"));

This would override any error reported by
virQEMUCapsCPUModelInfoFromCPUDef.

> +goto cleanup;
> +}
> +
> +if (i == 0) {
> +model_baseline = next_model;
> +continue;
> +}

Alternatively you could just call virQEMUCapsCPUModelInfoFromCPUDef once
before the for loop to set model_baseline and start the for loop at
index 1.

> +
> +if (qemuMonitorGetCPUModelBaseline(cmd->mon, model_baseline,
> +   next_model, _model_baseline) 
> < 0)
> +goto cleanup;
> +
> +if (!new_model_baseline) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("QEMU doesn't support baseline or recognize 
> model %s or %s"),
> +   model_baseline->name,
> +   next_model->name);
> +ret = 0;

This doesn't make a lot of sense. It's a real error so why you'd return
0? And the error message should be reported by
qemuMonitorGetCPUModelBaseline.

> +goto cleanup;
> +}
> +
> +qemuMonitorCPUModelInfoFree(model_baseline);
> +qemuMonitorCPUModelInfoFree(next_model);
> +
> +next_model = NULL;
> +
> +model_baseline = new_model_baseline;
> +}
> +
> +if (!(*baseline = virQEMUCapsCPUModelInfoToCPUDef(migratable_only, 
> model_baseline)))
> +goto cleanup;
> +
> +VIR_DEBUG("baseline->model = %s", NULLSTR((*baseline)->model));

How could the model name be NULL at this point?

> +
> +ret = 0;
> +
> + cleanup:
> +qemuMonitorCPUModelInfoFree(model_baseline);
> +qemuMonitorCPUModelInfoFree(next_model);
> +
> +return ret;
> +}
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 7be636d739..d49c8b32ec 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -593,6 +593,10 @@ void virQEMUCapsFilterByMachineType(virQEMUCapsPtr 
> qemuCaps,
>  qemuMonitorCPUModelInfoPtr virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef 
> *cpuDef);
>  virCPUDefPtr virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, 
> qemuMonitorCPUModelInfoPtr model);
>  
> +int virQEMUCapsQMPBaselineCPUModel(virQEMUCapsInitQMPCommandPtr cmd,
> +   virCPUDefPtr *cpus,
> +   virCPUDefPtr 

[libvirt] [PATCH] qemu: hotplug: don't overwrite error message in qemuDomainAttachNetDevice

[Katerina Koukiou]

Since commit f14c37, virDomainConfVMNWFilterTeardown is reporting errors
thus any previously reported error gets overwritten.
We need to save the errors in qemuDomainAttachNetDevice before calling
this function when we are in cleanup code.

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

Signed-off-by: Katerina Koukiou 
---
 src/qemu/qemu_hotplug.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3dfa51b0a0..2e13cab233 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -793,6 +793,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 bool charDevPlugged = false;
 bool netdevPlugged = false;
 char *netdev_name;
+virErrorPtr save_error = NULL;
 
 /* preallocate new slot for device */
 if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0)
@@ -1074,7 +1075,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 qemuDomainReleaseDeviceAddress(vm, >info, NULL);
 
 if (iface_connected) {
+virErrorPreserveLast(_error);
 virDomainConfNWFilterTeardown(net);
+virErrorRestore(_error);
 
 if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) {
 ignore_value(virNetDevMacVLanDeleteWithVPortProfile(
-- 
2.15.0

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

Re: [libvirt] [PATCHv2 09/11] qemu_capabilities: Persist QEMU instance over multiple QMP Cmds

[Jiri Denemark]

On Mon, Jul 09, 2018 at 22:56:53 -0500, Chris Venteicher wrote:
> Commit makes starting a single persistent QEMU instance possible for use
> over multiple independent QMP commands without starting and stopping
> QEMU for each QMP command or command type.
> 
> Commit allows functions outside qemu_capabilities
> (ex.  qemuConnectBaselineHypervisorCPU in qemu_driver)
> requiring multiple different QMP messages to be sent to QEMU to issue
> those requests over a single QMP Command Channel without starting and
> stopping QEMU for each independent QMP Command usage.
> 
> Commit moves following to global scope so parent function can
> maintain QEMU instance over multiple QMP commands / command types:
> virQEMUCapsInitQMPCommand
> virQEMUCapsInitQMPCommandFree
> 
> Commit Introduces virQEMUCapsNewQMPCommandConnection to Start and
> connect to QEMU so QMP commands can be performed.
> 
> The new reusable function isolates code for starting QEMU and
> establishing Monitor connections from code for obtaining capabilities so
> that arbitrary QMP commands can be exchanged with QEMU.
> ---
>  src/qemu/qemu_capabilities.c | 61 +++-
>  src/qemu/qemu_capabilities.h | 26 +++
>  2 files changed, 66 insertions(+), 21 deletions(-)

Just a high level review since this patch will require some significant
changes...

Your approach will not work because all callers would end up using the
same monitor socket for to the QEMU process. And there might be several
concurrent callers which would want to call some QMP command to do the
job, each of them spawning their own QEMU process. You'd need to give
each caller a unique monitor socket path.

I think this should be reworked into a general code (ideally placed in
qemu_process.c), which would then be called by the QEMU capabilities
code and from APIs which need to call QEMU.

Jirka

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

Re: [libvirt] [PATCH 2/4] conf: Move hugepage XML validation check out of qemu_command

[Michal Privoznik]

On 07/13/2018 02:02 PM, Pavel Hrdina wrote:
> On Wed, Jul 11, 2018 at 06:03:08PM +0200, Pavel Hrdina wrote:
>> To make it clear I'll summarize all the possible combinations and how it
>> should work so we are on the same page.
> 
> originally: before commit [1]
> now: after commit [1] (current master)
> expect: what this patch series should fix
> 
> 
> === Non-NUMA guests ===
> 
> 
> * Only one hugepage specified without any nodeset
> 
> 
>   
> 
>   
> 
> 
> This is correct, was always working and we should not change it.
> 
> originally: works
> now: works
> expected: works
> 
> 
> * Only one hugapage specified with nodeset
> 
> 
>   
> 
>   
> 
> 
> This is questionable since there is no guest NUMA node specified,
> but on the other hand we can consider non-NUMA guest to have exactly
> one NUMA node.
> 
> This was working but has been broken by commit [1]  which tried to
> fix a case where you are trying to specify non-existing NUMA node.
> 
> Because of that assumption we can consider this as valid XML even
> though there is no NUMA node specified [2].  There are two possible
> solutions:
> 
> - we can leave the XML intact
> 
> - we can silently remove the 'nodeset' attribute to 'fix' the
>   XML definition
> 
> originally: works
> now: fails
> expect: works
> 
> 
> 
>   
> 
>   
> 
> 
> If the nodeset is != 0 it should newer work becuase there is no
> guest NUMA topology and even if we take into account the assumption
> that there is always one NUMA node it is still invalid.
> 
> originally: works
> now: fails
> expect: fails
> 
> 
> * One hugepage with specific nodeset and second default hugepage
> 
> 
>   
> 
> 
>   
> 
> 
> This was working but was 'fixed' by commit [1] because the code
> checks only the first hugepage and uses only the first hugepage.
> 
> It should never worked because it doesn't make any sense, there
> is no guest NUMA node configured and even if we take into account
> the assumption that non-NUMA guest has one NUMA node there is need
> for the default page size.
> 
> originally: works
> now: fails
> expect: fails
> 
> 
> There is yet another issue with the current state in libvirt, if
> you swap the order of pages:
> 
> 
>   
> 
> 
>   
> 
> 
> it will work even with current libvirt with the fix [1].  The reason
> is that code in qemuBuildMemPathStr() function takes into account
> only the first page size so it depends on the order of elements
> which is wrong.
> 
> We should not allow any of these two configurations.  Setting
> nodeset to != 0 will should not make any difference.
> 
> originally: works
> now: works
> expect: fails
> 
> 
> == NUMA guest ==
> 
> 
> * Only one hugepage specified without any nodeset
> 
> 
>   
> 
>   
> 
> ...
> 
>   
>   
> 
> 
>   
> 
> 
> originally: works
> now: works
> expect: works
> 
> 
> * Only one hugapage specified with nodeset
> 
> 
>   
> 
>   
> 
> ...
> 
>   
>   
> 
> 
>   
> 
> 
> All possible combinations for the nodeset attribute are allowed
> if it always corresponds to existing guest NUMA node:
> 
> 
> 
> or
> 
> 
> 
> originally: works
> now: works
> expect: works
> 
> 
> 
>   
> 
>   
> 
> ...
> 
>   
>   
> 
> 
>   
> 
> 
> There is invalid guest NUMA node specified for the hugepage.
> 
> originally: fails
> now: fails
> expect: fails
> 
> * One hugepage with specific nodeset and second default hugepage
> 
> 
>   
> 
> 
>   
> 
> ...
> 
>   
>   
> 
> 
>   
> 
> 
> There are two guest NUMA nodes where we have default hugepage size
> configured and for NUMA node '0' we have a different size.
> 
> originally: works
> now: works
> expect: works
> 
> 
> 
>   
> 
> 
>   
> 
> ...
> 
>   
>   
> 
> 
>   
> 
> 
> originally: works
> now: works
> expect: fails ???
> 
> In this situation all the guest NUMA nodes are covered by the
> hugepage size with specified nodeset attribute.  The default one
> is not used at all so is pointless here.
> 
> The difference between this and non-NUMA guest is that if we change
> the order it will still work as expected, it doesn't depend on the
> order of elements.  However, we might consider is as invalid
> configuration because there is no need to have the default page size
> configured at all.
> 

Re: [libvirt] [PATCHv2 08/11] qemu_capabilities: QMPCommandPtr without maintaining shadow qmperr

[Jiri Denemark]

On Mon, Jul 09, 2018 at 22:56:52 -0500, Chris Venteicher wrote:
> Previously QMPCommandPtr (handle for issuing QMP commands) required an
> external char * qmperr to persist over the lifespan of QMPCommand to
> expose a QMP error string outside of QMPCommand.
> 
> Before this change, an external char *qmperr had to be maintained
> between calls to virQEMUCapsInitQMPCommandNew and
> virQEMUCapsInitQMPCommandAbort.
> 
> This change allows the qmperr pointer to be maintained within the
> QMPCommand structure avoiding the need to track and maintain the qmperr
> across a sometimes multi-function call lifespan of a QMPCommand.
> 
> Q) Should we eliminate external *qmperr completely as there seems to be
> no current use of qmperr outside the lifespan of QMPCommand in which an
> internal qmperr char pointer can persist and be used by anywhere we have
> access to the QMPCommandPtr?
> ---
>  src/qemu/qemu_capabilities.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index d3f2317a1d..f33152ec40 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4272,6 +4272,7 @@ struct _virQEMUCapsInitQMPCommand {
>  uid_t runUid;
>  gid_t runGid;
>  char **qmperr;
> +char *qmperr_internal;

I'd probably call it qmperrBuffer or something similar to make it more
obvious it's the buffer for qmperr.

>  char *monarg;
>  char *monpath;
>  char *pidfile;
> @@ -4349,7 +4350,11 @@ virQEMUCapsInitQMPCommandNew(char *binary,
>  
>  cmd->runUid = runUid;
>  cmd->runGid = runGid;
> -cmd->qmperr = qmperr;
> +
> +if (qmperr)
> +cmd->qmperr = qmperr; /* external storage */
> +else
> +cmd->qmperr = >qmperr_internal; /* cmd internal storage */
>  
>  /* the ".sock" sufix is important to avoid a possible clash with a qemu
>   * domain called "capabilities"

You don't free the internal buffer anywhere.

Jirka

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

Re: [libvirt] [REPOST PATCHv2 1/2] qemu: Check for existing hostdev address for cold attach device

[Michal Privoznik]

On 07/13/2018 02:50 PM, John Ferlan wrote:
> 
> 
> On 07/09/2018 10:32 AM, Michal Privoznik wrote:
>> On 07/06/2018 08:50 PM, John Ferlan wrote:
>>> Prior to the hostdev being inserted in the hostdevs list,
>>> add a check during qemuDomainAttachDeviceConfig to determine
>>> whether the new/incoming  device is providing
>>> the same  as some existing hostdev on the list
>>> and if so fail the cold attach.
>>>
>>> This cannot be done during virDomainHostdevDefPostParse
>>> because that is called after virDomainDefParseXML would
>>> have inserted a hostdev onto the hostdevs list and thus
>>> would have a "conflict" with itself. Therefore, the post
>>> parse processing can only compare if the current hostdev
>>> address conflicts with a SCSI  address.
>>>
>>> By comparison this is similar to the validation phase
>>> checks in virDomainDefCheckDuplicateDriveAddresses that
>>> occur during define/startup processing but are not run
>>> during config attach of a live guest.
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>  src/qemu/qemu_driver.c | 6 ++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 9a35e04a85..ef1abe3f68 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -8007,6 +8007,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
>>> _("device is already in the domain 
>>> configuration"));
>>>  return -1;
>>>  }
>>> +if (dev->data.hostdev->info->type != 
>>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>>> +virDomainDefHasDeviceAddress(vmdef, dev->data.hostdev->info)) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +   _("a device with the same address already 
>>> exists "));
>>> +return -1;
>>> +}
>>>  if (virDomainHostdevInsert(vmdef, hostdev))
>>>  return -1;
>>>  dev->data.hostdev = NULL;
>>>
>>
>> I think hostdevs are not the only type of device suffering from this. In
>> fact, I've just tested disks and libvirt accepts attaching another disk
>> onto same  happily.
>>
>> I wonder if this should go into virDomainDefCompatibleDevice() (now that
>> we have @action there ;-) ).
>>
> 
> It's a case of myopia for the bug I'm working on as listed in patch2.
> 
> What I did is no different than the VIR_DOMAIN_DEVICE_RNG check in the
> same function.
> 
> Still, if I change this patch to add:
> 
>  if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH && !live &&
> data.newInfo &&
> data.newInfo->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> virDomainDefHasDeviceAddress(def, data.newInfo)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>_("a device with the same address already exists "));
> return -1;
> }
> 
> to virDomainDefCompatibleDevice and remove the (new)hostdev and
> (existing)rng checks, then I believe it covers the existing cases.

Yes, this looks reasonable.

Michal

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

Re: [libvirt] [REPOST PATCHv2 2/2] qemu: Use the correct vm def on cold attach

[Michal Privoznik]

On 07/13/2018 02:50 PM, John Ferlan wrote:
> 
> 
> On 07/09/2018 10:32 AM, Michal Privoznik wrote:
>> On 07/06/2018 08:50 PM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1559867
>>>
>>> When attaching a device to the domain we need to be sure
>>> to use the correct domain definition (vm->def or vm->newDef)
>>> when calling virDomainDeviceDefParse because the post parse
>>> processing algorithms that may assign an address for the
>>> device will use whatever domain definition was passed in.
>>>
>>> Additionally, some devices (SCSI hostdev and SCSI disk) use
>>> algorithms that rely on knowing what already exists of the
>>> other type when generating the new device's address. Using
>>> the wrong VM definition could result in duplicated addresses.
>>>
>>> In the case of the bz, two hostdev's with no domain address
>>> provided were added to the running domain's config only.
>>> However, the parsing algorithm used the live domain in
>>> order to figure out the host device address resulting in
>>> the same address being used and a subsequent start failing
>>> due to duplicate address.
>>>
>>> Fix this by separating the checks/code into CONFIG and LIVE
>>> processing using the correct definition for each block and
>>> peforming cleanup for both options as necessary.
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>  src/qemu/qemu_driver.c | 52 
>>> ++
>>>  1 file changed, 23 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index ef1abe3f68..60085befea 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -8469,7 +8469,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr 
>>> vm,
>>>  {
>>>  virDomainDefPtr vmdef = NULL;
>>>  virQEMUDriverConfigPtr cfg = NULL;
>>> -virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
>>> +virDomainDeviceDefPtr devConf = NULL;
>>> +virDomainDeviceDefPtr devLive = NULL;
>>>  int ret = -1;
>>>  virCapsPtr caps = NULL;
>>>  unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
>>> @@ -8483,49 +8484,43 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr 
>>> vm,
>>>  if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>>>  goto cleanup;
>>>  
>>> -dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
>>> - caps, driver->xmlopt,
>>> - parse_flags);
>>> -if (dev == NULL)
>>> -goto cleanup;
>>> -
>>> -if (virDomainDeviceValidateAliasForHotplug(vm, dev, flags) < 0)
>>> -goto cleanup;
>>> -
>>> -if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
>>> -flags & VIR_DOMAIN_AFFECT_LIVE) {
>>> -/* If we are affecting both CONFIG and LIVE
>>> - * create a deep copy of device as adding
>>> - * to CONFIG takes one instance.
>>> - */
>>> -dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, 
>>> driver->xmlopt);
>>> -if (!dev_copy)
>>> -goto cleanup;
>>> -}
>>> -
>>> +/* The config and live post processing address auto-generation 
>>> algorithms
>>> + * rely on the correct vm->def or vm->newDef being passed, so call the
>>> + * device parse based on which definition is in use */
>>>  if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
>>> -/* Make a copy for updated domain. */
>>>  vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
>>>  if (!vmdef)
>>>  goto cleanup;
>>>  
>>> -if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
>>> +if (!(devConf = virDomainDeviceDefParse(xml, vmdef, caps,
>>> +driver->xmlopt, 
>>> parse_flags)))
>>> +goto cleanup;
>>> +
>>> +if (virDomainDefCompatibleDevice(vmdef, devConf, NULL,
>>>   VIR_DOMAIN_DEVICE_ACTION_ATTACH,
>>>   false) < 0)
>>>  goto cleanup;
>>> -if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps,
>>> +
>>> +if ((ret = qemuDomainAttachDeviceConfig(vmdef, devConf, caps,
>>>  parse_flags,
>>>  driver->xmlopt)) < 0)
>>>  goto cleanup;
>>>  }
>>>  
>>>  if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>>> -if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
>>> +if (!(devLive = virDomainDeviceDefParse(xml, vm->def, caps,
>>> +driver->xmlopt, 
>>> parse_flags)))
>>> +goto cleanup;
>>> +
>>
>> In the light of 84de7fbfdb2f528e05d98c09e3260fe0fc0739f9: shouldn't we
>> parse this as live XML?
>>
> 
> Let's see:
> 
> qemuDomainAttachDeviceFlags calls virDomainObjUpdateModificationImpact
> and then calls qemuDomainAttachDeviceLiveAndConfig, so if 

Re: [libvirt] [PATCH] qemu: hotplug: report error when changing rom enabled attr for net iface

[Ján Tomko]

On Fri, Jul 13, 2018 at 03:57:04PM +0200, Katerina Koukiou wrote:

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



The 'Resolves: ' prefix is still pointless, but Andrea might try to disagree


Signed-off-by: Katerina Koukiou 
---
src/qemu/qemu_hotplug.c | 5 +
1 file changed, 5 insertions(+)



If you add yourself to the maintainers list:

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 0/2] Fix libvirt for kernels without DM support

[Ján Tomko]

On Fri, Jul 13, 2018 at 03:48:25PM +0200, Michal Privoznik wrote:

*** BLURB HERE ***

Michal Prívozník (2):
 virDevMapperGetTargetsImpl: Be tolerant to kernels without DM support
 qemu_cgroup: Allow/disallow devmapper control iff available

src/qemu/qemu_cgroup.c  | 36 +++-
src/util/virdevmapper.c |  8 +++-
2 files changed, 26 insertions(+), 18 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

[libvirt] [PATCH] qemu: hotplug: report error when changing rom enabled attr for net iface

[Katerina Koukiou]

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

Signed-off-by: Katerina Koukiou 
---
 src/qemu/qemu_hotplug.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3dfa51b0a0..bb50d19c85 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3222,6 +3222,11 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
_("cannot modify network device boot index setting"));
 goto cleanup;
 }
+if (olddev->info.romenabled != newdev->info.romenabled) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("cannot modify network device rom enabled setting"));
+goto cleanup;
+}
 /* (end of device info checks) */
 
 if (STRNEQ_NULLABLE(olddev->filter, newdev->filter) ||
-- 
2.15.0

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

Re: [libvirt] [PATCHv2 07/11] qemu_capabilities: Introduce virCPUDef / qemuMonitorCPUModelInfo conversions

[Jiri Denemark]

On Mon, Jul 09, 2018 at 22:56:51 -0500, Chris Venteicher wrote:
> Bi-directional conversion functions.
> Converts model / name and features / properties between the two structures.
> 
> Created reusable functions to bridge the internal (virCpuDef) and

s/virCpuDef/virCPUDef/

> QEMU/QMP specific structures for describing CPU Models.
> ---
>  src/qemu/qemu_capabilities.c | 146 +--
>  src/qemu/qemu_capabilities.h |   3 +
>  2 files changed, 127 insertions(+), 22 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 72ab012a78..d3f2317a1d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2758,7 +2758,8 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
>  virCPUDefPtr cpu,
>  bool migratable)
>  {
> -size_t i;
> +virCPUDefPtr tmp = NULL;
> +int ret = -1;
>  
>  if (!modelInfo) {
>  if (type == VIR_DOMAIN_VIRT_KVM) {
> @@ -2771,32 +2772,21 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
>  return 2;
>  }
>  
> -if (VIR_STRDUP(cpu->model, modelInfo->name) < 0 ||
> -VIR_ALLOC_N(cpu->features, modelInfo->nprops) < 0)
> -return -1;
> +if (!(tmp = virQEMUCapsCPUModelInfoToCPUDef(migratable, modelInfo)))
> +goto cleanup;
>  
> -cpu->nfeatures_max = modelInfo->nprops;
> -cpu->nfeatures = 0;
> +/* Free then copy over model, vendor, vendor_id and features */
> +virCPUDefFreeModel(cpu);
>  
> -for (i = 0; i < modelInfo->nprops; i++) {
> -virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
> -qemuMonitorCPUPropertyPtr prop = modelInfo->props + i;
> -
> -if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
> -continue;
> +if (virCPUDefCopyModel(cpu, tmp, true) < 0)
> +goto cleanup;

You can replace the virCPUDefFreeModel/virCPUDefCopyModel combo with a
single virCPUDefStealModel.

>  
> -if (VIR_STRDUP(feature->name, prop->name) < 0)
> -return -1;
> +ret = 0;
>  
> -if (!prop->value.boolean ||
> -(migratable && prop->migratable == VIR_TRISTATE_BOOL_NO))
> -feature->policy = VIR_CPU_FEATURE_DISABLE;
> -else
> -feature->policy = VIR_CPU_FEATURE_REQUIRE;
> -cpu->nfeatures++;
> -}
> + cleanup:
> +virCPUDefFree(tmp);
>  
> -return 0;
> +return ret;
>  }
>  
>  
> @@ -3547,6 +3537,118 @@ virQEMUCapsLoadCache(virArch hostArch,
>  return ret;
>  }
>  
> +/* virCPUDef model=> qemuMonitorCPUModelInfo name
> + * virCPUDef features => qemuMonitorCPUModelInfo boolean properties */
> +qemuMonitorCPUModelInfoPtr
> +virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef)
> +{
> +size_t i;
> +qemuMonitorCPUModelInfoPtr cpuModel = NULL;
> +qemuMonitorCPUModelInfoPtr ret = NULL;
> +
> +if (!cpuDef || (VIR_ALLOC(cpuModel) < 0))
> +goto cleanup;
> +
> +VIR_DEBUG("cpuDef->model = %s", NULLSTR(cpuDef->model));
> +
> +if (VIR_STRDUP(cpuModel->name, cpuDef->model) < 0 ||
> +VIR_ALLOC_N(cpuModel->props, cpuDef->nfeatures) < 0)
> +goto cleanup;
> +
> +/* no visibility on migratability of properties / features */
> +cpuModel->props_migratable_valid = false;

I don't think there's any need to set this explicitly as this is the
default value.

> +
> +cpuModel->nprops = 0;

This is also the default value, but since we actually care about the
value and increment it later, it's useful to explicitly set it anyway.
(In contrast to the migratability bool above)

> +
> +for (i = 0; i < cpuDef->nfeatures; i++) {
> +qemuMonitorCPUPropertyPtr prop = 
> &(cpuModel->props[cpuModel->nprops]);
> +virCPUFeatureDefPtr feature = &(cpuDef->features[i]);
> +
> +if (!feature || !(feature->name))
> +continue;

feature cannot be NULL at this point.

> +
> +if (VIR_STRDUP(prop->name, feature->name) < 0)
> +goto cleanup;
> +
> +prop->migratable = VIR_TRISTATE_BOOL_ABSENT;

No need to set this explicitly.

> +prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
> +
> +switch (feature->policy) {
> +case -1:/* policy undefined */
> +case VIR_CPU_FEATURE_FORCE:
> +case VIR_CPU_FEATURE_REQUIRE:
> +prop->value.boolean = true;
> +break;
> +
> +case VIR_CPU_FEATURE_FORBID:
> +case VIR_CPU_FEATURE_DISABLE:
> +case VIR_CPU_FEATURE_OPTIONAL:
> +case VIR_CPU_FEATURE_LAST:
> +prop->value.boolean = false;
> +break;
> +}

I don't see any value in using switch since the compiler won't warn us
in case of missing case anyway because feature->policy is int. I'd just
write it as

if (feature->policy == -1 ||
feature->policy == VIR_CPU_FEATURE_FORCE ||
feature->policy == 

[libvirt] [PATCH 2/2] qemu_cgroup: Allow/disallow devmapper control iff available

[Michal Privoznik]

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

On kernels without device mapper support there won't be
/dev/mapper/control. Therefore it doesn't make much sense to
put it into devices CGroup.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_cgroup.c | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index c8fba7f9e6..43e17d786e 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -129,6 +129,7 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm,
 }
 
 if (virStoragePRDefIsManaged(src->pr) &&
+virFileExists(DEVICE_MAPPER_CONTROL_PATH) &&
 qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0)
 return -1;
 
@@ -163,28 +164,29 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
 return 0;
 }
 
-for (i = 0; i < vm->def->ndisks; i++) {
-virStorageSourcePtr diskSrc = vm->def->disks[i]->src;
+if (virFileExists(DEVICE_MAPPER_CONTROL_PATH)) {
+for (i = 0; i < vm->def->ndisks; i++) {
+virStorageSourcePtr diskSrc = vm->def->disks[i]->src;
 
-if (src == diskSrc)
-continue;
+if (src == diskSrc)
+continue;
 
-if (virStoragePRDefIsManaged(diskSrc->pr))
-break;
-}
+if (virStoragePRDefIsManaged(diskSrc->pr))
+break;
+}
 
-if (i == vm->def->ndisks) {
-VIR_DEBUG("Disabling device mapper control");
-ret = virCgroupDenyDevicePath(priv->cgroup,
-  DEVICE_MAPPER_CONTROL_PATH, perms, true);
-virDomainAuditCgroupPath(vm, priv->cgroup, "deny",
- DEVICE_MAPPER_CONTROL_PATH,
- virCgroupGetDevicePermsString(perms), ret);
-if (ret < 0)
-return ret;
+if (i == vm->def->ndisks) {
+VIR_DEBUG("Disabling device mapper control");
+ret = virCgroupDenyDevicePath(priv->cgroup,
+  DEVICE_MAPPER_CONTROL_PATH, perms, 
true);
+virDomainAuditCgroupPath(vm, priv->cgroup, "deny",
+ DEVICE_MAPPER_CONTROL_PATH,
+ virCgroupGetDevicePermsString(perms), 
ret);
+if (ret < 0)
+return ret;
+}
 }
 
-
 VIR_DEBUG("Deny path %s", src->path);
 
 ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true);
-- 
2.16.4

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

[libvirt] [PATCH 1/2] virDevMapperGetTargetsImpl: Be tolerant to kernels without DM support

[Michal Privoznik]

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

If kernel is compiled without CONFIG_BLK_DEV_DM enabled, there is
no /dev/mapper/control device and since dm_task_create() actually
does some ioctl() over it creating a task may fail.
To cope with this handle ENOENT and ENODEV gracefully.

Signed-off-by: Michal Privoznik 
---
 src/util/virdevmapper.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
index b365f20145..7da0dba911 100644
--- a/src/util/virdevmapper.c
+++ b/src/util/virdevmapper.c
@@ -87,8 +87,14 @@ virDevMapperGetTargetsImpl(const char *path,
 return ret;
 }
 
-if (!(dmt = dm_task_create(DM_DEVICE_DEPS)))
+if (!(dmt = dm_task_create(DM_DEVICE_DEPS))) {
+if (errno == ENOENT || errno == ENODEV) {
+/* It's okay. Kernel is probably built without
+ * devmapper support. */
+ret = 0;
+}
 return ret;
+}
 
 if (!dm_task_set_name(dmt, path)) {
 if (errno == ENOENT) {
-- 
2.16.4

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

[libvirt] [PATCH 0/2] Fix libvirt for kernels without DM support

[Michal Privoznik]

*** BLURB HERE ***

Michal Prívozník (2):
  virDevMapperGetTargetsImpl: Be tolerant to kernels without DM support
  qemu_cgroup: Allow/disallow devmapper control iff available

 src/qemu/qemu_cgroup.c  | 36 +++-
 src/util/virdevmapper.c |  8 +++-
 2 files changed, 26 insertions(+), 18 deletions(-)

-- 
2.16.4

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

Re: [libvirt] [PATCH v2] virnetdevtap: Don't crash on !ifname in virNetDevTapInterfaceStats

[Andrea Bolognani]

On Fri, 2018-07-13 at 13:59 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1595184
> 
> Some domain  do not have a name (because they are
> not TAP devices). Therefore, if
> virNetDevTapInterfaceStats(net->ifname, ...) is called an instant
> crash occurs. In Linux version of the function strlen() is called
> over the name and in BSD version STREQ() is called.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> diff to v1:
> - changed the error message
> - fixed check in BSD version
> 
>  src/util/virnetdevtap.c | 12 

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [RFC PATCH 10/17] qemu: add qemuSecurityStartVhostUserGPU helper

[marcandre . lureau]

From: Marc-André Lureau 

See function documentation, used in following patch.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_security.c | 48 
 src/qemu/qemu_security.h |  6 +
 2 files changed, 54 insertions(+)

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index af3be42854..c722d19ca4 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -426,6 +426,54 @@ qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver,
 }
 
 
+/*
+ * qemuSecurityStartVhostUserGPU:
+ *
+ * @driver: the QEMU driver
+ * @def: the domain definition
+ * @cmd: the command to run
+ * @existstatus: pointer to int returning exit status of process
+ * @cmdret: pointer to int returning result of virCommandRun
+ *
+ * Start the vhost-user-gpu process with approriate labels.
+ * This function returns -1 on security setup error, 0 if all the
+ * setup was done properly. In case the virCommand failed to run
+ * 0 is returned but cmdret is set appropriately with the process
+ * exitstatus also set.
+ */
+int
+qemuSecurityStartVhostUserGPU(virQEMUDriverPtr driver,
+  virDomainDefPtr def,
+  virCommandPtr cmd,
+  int *exitstatus,
+  int *cmdret)
+{
+int ret = -1;
+
+if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
+   def, cmd) < 0)
+goto cleanup;
+
+if (virSecurityManagerPreFork(driver->securityManager) < 0)
+goto cleanup;
+
+ret = 0;
+
+*cmdret = virCommandRun(cmd, exitstatus);
+
+virSecurityManagerPostFork(driver->securityManager);
+
+if (*cmdret < 0)
+goto cleanup;
+
+return 0;
+
+cleanup:
+
+return ret;
+}
+
+
 /*
  * qemuSecurityStartTPMEmulator:
  *
diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
index a189b63828..75131120b9 100644
--- a/src/qemu/qemu_security.h
+++ b/src/qemu/qemu_security.h
@@ -84,6 +84,12 @@ int qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 virDomainChrDefPtr chr);
 
+int qemuSecurityStartVhostUserGPU(virQEMUDriverPtr driver,
+  virDomainDefPtr def,
+  virCommandPtr cmd,
+  int *exitstatus,
+  int *cmdret);
+
 int qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
  virDomainDefPtr def,
  virCommandPtr cmd,
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 09/17] qemu: validate vhost-user video model

[marcandre . lureau]

From: Marc-André Lureau 

Check qemu capability, and accept 3d acceleration.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_process.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c903a8e5c8..ee4c3445fb 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4973,6 +4973,8 @@ qemuProcessStartValidateVideo(virDomainObjPtr vm,
  !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) ||
 (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
  !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_USER_GPU)) ||
 (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
  video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
  !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW))) {
@@ -4983,7 +4985,9 @@ qemuProcessStartValidateVideo(virDomainObjPtr vm,
 }
 
 if (video->accel) {
-if (video->accel->accel3d == VIR_TRISTATE_SWITCH_ON &&
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER) {
+continue;
+} else if (video->accel->accel3d == VIR_TRISTATE_SWITCH_ON &&
 (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
  !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_VIRGL))) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 17/17] tests: add vhost-user-gpu xml2argv tests

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
 .../vhost-user-gpu-secondary.args | 34 +
 .../vhost-user-gpu-secondary.xml  | 38 +++
 tests/qemuxml2argvdata/vhost-user-vga.args| 31 +++
 tests/qemuxml2argvdata/vhost-user-vga.xml | 35 +
 tests/qemuxml2argvtest.c  | 12 ++
 5 files changed, 150 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/vhost-user-gpu-secondary.args
 create mode 100644 tests/qemuxml2argvdata/vhost-user-gpu-secondary.xml
 create mode 100644 tests/qemuxml2argvdata/vhost-user-vga.args
 create mode 100644 tests/qemuxml2argvdata/vhost-user-vga.xml

diff --git a/tests/qemuxml2argvdata/vhost-user-gpu-secondary.args 
b/tests/qemuxml2argvdata/vhost-user-gpu-secondary.args
new file mode 100644
index 00..0aee4f50ec
--- /dev/null
+++ b/tests/qemuxml2argvdata/vhost-user-gpu-secondary.args
@@ -0,0 +1,34 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-object memory-backend-memfd,id=ram-node,size=224395264 \
+-numa node,nodeid=0,memdev=ram-node \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait
 \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-chardev socket,id=chr-vu-video0,fd=0 \
+-object vhost-user-backend,id=vu-video0,chardev=chr-vu-video0 \
+-chardev socket,id=chr-vu-video1,fd=0 \
+-object vhost-user-backend,id=vu-video1,chardev=chr-vu-video1 \
+-device 
vhost-user-vga,id=video0,max_outputs=1,vhost-user=vu-video0,bus=pci.0,addr=0x2 \
+-device 
vhost-user-gpu-pci,id=video1,max_outputs=1,vhost-user=vu-video1,bus=pci.0,addr=0x4
 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/vhost-user-gpu-secondary.xml 
b/tests/qemuxml2argvdata/vhost-user-gpu-secondary.xml
new file mode 100644
index 00..f91be819f6
--- /dev/null
+++ b/tests/qemuxml2argvdata/vhost-user-gpu-secondary.xml
@@ -0,0 +1,38 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  
+
+  
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-i686
+
+  
+  
+  
+
+
+
+
+
+
+
+
+  
+
+
+  
+
+  
+
diff --git a/tests/qemuxml2argvdata/vhost-user-vga.args 
b/tests/qemuxml2argvdata/vhost-user-vga.args
new file mode 100644
index 00..fbaea7e371
--- /dev/null
+++ b/tests/qemuxml2argvdata/vhost-user-vga.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-object memory-backend-memfd,id=ram-node,size=224395264 \
+-numa node,nodeid=0,memdev=ram-node \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait
 \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-chardev socket,id=chr-vu-video0,fd=0 \
+-object vhost-user-backend,id=vu-video0,chardev=chr-vu-video0 \
+-device 
vhost-user-vga,id=video0,max_outputs=1,vhost-user=vu-video0,bus=pci.0,addr=0x2 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/vhost-user-vga.xml 
b/tests/qemuxml2argvdata/vhost-user-vga.xml
new file mode 100644
index 00..db38b5a45e
--- /dev/null
+++ b/tests/qemuxml2argvdata/vhost-user-vga.xml
@@ -0,0 +1,35 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  
+
+  
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-i686
+
+  
+  
+  
+
+
+
+
+
+
+
+
+  
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 47bd2ab867..d78caa6a37 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2921,6 +2921,18 @@ mymain(void)
 QEMU_CAPS_KVM,
 QEMU_CAPS_SEV_GUEST);
 
+DO_TEST("vhost-user-vga",
+QEMU_CAPS_OBJECT_MEMORY_MEMFD,
+

[libvirt] [RFC PATCH 11/17] qemu: add vhost-user-gpu helper unit

[marcandre . lureau]

From: Marc-André Lureau 

Similar to the qemu_tpm.c, add a unit with a few functions to
start/stop and setup the cgroup of the external vhost-user-gpu
process. See function documentation.

Signed-off-by: Marc-André Lureau 
---
 src/conf/device_conf.h |   1 +
 src/qemu/Makefile.inc.am   |   2 +
 src/qemu/qemu_vhost_user_gpu.c | 318 +
 src/qemu/qemu_vhost_user_gpu.h |  48 +
 4 files changed, 369 insertions(+)
 create mode 100644 src/qemu/qemu_vhost_user_gpu.c
 create mode 100644 src/qemu/qemu_vhost_user_gpu.h

diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index a31ce9c376..544fd33a33 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -175,6 +175,7 @@ struct _virDomainDeviceInfo {
  * cases we might want to prevent that from happening by
  * locking the isolation group */
 bool isolationGroupLocked;
+int vhost_user_fd;
 };
 
 int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 2afa67f195..28daf9d426 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -56,6 +56,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_qapi.h \
qemu/qemu_tpm.c \
qemu/qemu_tpm.h \
+   qemu/qemu_vhost_user_gpu.c \
+   qemu/qemu_vhost_user_gpu.h \
$(NULL)
 
 
diff --git a/src/qemu/qemu_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c
new file mode 100644
index 00..9007614020
--- /dev/null
+++ b/src/qemu/qemu_vhost_user_gpu.c
@@ -0,0 +1,318 @@
+/*
+ * qemu_vhost_user_gpu.c: QEMU vhost-user GPU support
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * 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
+ * .
+ *
+ * Author: Marc-André Lureau 
+ */
+
+#include 
+
+#include "qemu_extdevice.h"
+#include "qemu_domain.h"
+#include "qemu_security.h"
+
+#include "conf/domain_conf.h"
+#include "vircommand.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "virutil.h"
+#include "virfile.h"
+#include "virstring.h"
+#include "virtime.h"
+#include "virpidfile.h"
+#include "qemu_vhost_user_gpu.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("qemu.vhost-user-gpu")
+
+/*
+ * Look up the vhost-user-gpu executable; to be found on the host
+ */
+static char *vhost_user_gpu_path;
+
+static int
+qemuVhostUserGPUInit(void)
+{
+if (!vhost_user_gpu_path) {
+vhost_user_gpu_path = virFindFileInPath("vhost-user-gpu");
+if (!vhost_user_gpu_path) {
+virReportSystemError(ENOENT, "%s",
+ _("Unable to find 'vhost-user-gpu' binary in 
$PATH"));
+return -1;
+}
+if (!virFileIsExecutable(vhost_user_gpu_path)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("vhost-user-gpu %s is not an executable"),
+   vhost_user_gpu_path);
+VIR_FREE(vhost_user_gpu_path);
+return -1;
+}
+}
+
+return 0;
+}
+
+
+static char *
+qemuVhostUserGPUCreatePidFilename(const char *stateDir,
+  const char *shortName,
+  const char *alias)
+{
+char *pidfile = NULL;
+char *devicename = NULL;
+
+if (virAsprintf(, "%s-%s-vhost-user-gpu", shortName, alias) < 0)
+return NULL;
+
+pidfile = virPidFileBuildPath(stateDir, devicename);
+
+VIR_FREE(devicename);
+
+return pidfile;
+}
+
+
+/*
+ * qemuVhostUserGPUGetPid
+ *
+ * @stateDir: the directory where vhost-user-gpu writes the pidfile into
+ * @shortName: short name of the domain
+ * @alias: video device alias
+ * @pid: pointer to pid
+ *
+ * Return -errno upon error, or zero on successful reading of the pidfile.
+ * If the PID was not still alive, zero will be returned, and @pid will be
+ * set to -1;
+ */
+static int
+qemuVhostUserGPUGetPid(const char *stateDir,
+   const char *shortName,
+   const char *alias,
+   pid_t *pid)
+{
+int ret;
+char *pidfile = qemuVhostUserGPUCreatePidFilename(stateDir, shortName, 
alias);
+if (!pidfile)
+return -ENOMEM;
+
+ret = virPidFileReadPathIfAlive(pidfile, pid, vhost_user_gpu_path);
+
+VIR_FREE(pidfile);
+
+return ret;
+}
+
+
+/*
+ * qemuExtVhostUserGPUStart:
+ *
+ * @driver: QEMU driver
+ * @def: 

[libvirt] [RFC PATCH 16/17] qemu: build vhost-user-gpu video device arguments

[marcandre . lureau]

From: Marc-André Lureau 

Change the model name to "vhost-user-gpu-pci" if running on PCI.

Set the "max_outputs" property.

Associate the device with the "vhost-user" backend.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_command.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b3a2bba28e..b4afcd2e8c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4319,7 +4319,7 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
 goto error;
 }
 
-if (STREQ(model, "virtio-gpu")) {
+if (STREQ(model, "virtio-gpu") || STREQ(model, "vhost-user-gpu")) {
 if (video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)
 virBufferAsprintf(, "%s-ccw", model);
 else
@@ -4367,6 +4367,10 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
 if (video->heads)
 virBufferAsprintf(, ",max_outputs=%u", video->heads);
 }
+} else if (video->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER) {
+if (video->heads)
+virBufferAsprintf(, ",max_outputs=%u", video->heads);
+virBufferAsprintf(, ",vhost-user=vu-%s", video->info.alias);
 } else if (video->vram &&
 ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
   virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 13/17] qemu: set default address type on vhost-user video model

[marcandre . lureau]

From: Marc-André Lureau 

It's a virtio device, like virtio-gpu.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_domain_address.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index df98d7384f..476ee2d4a5 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -329,7 +329,8 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
 virDomainVideoDefPtr video = def->videos[i];
 
 if (video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
-video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO)
+(video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
+ video->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER))
 video->info.type = type;
 }
 
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 14/17] qemu: start/stop the vhost-user-gpu external device

[marcandre . lureau]

From: Marc-André Lureau 

Each vhost-user-gpu needs its own helper gpu process.
Start/stop them, and apply the emulator cgroup controller.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_extdevice.c | 47 ++-
 1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index d982922470..8ec703a9b0 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -25,6 +25,7 @@
 #include "qemu_extdevice.h"
 #include "qemu_domain.h"
 #include "qemu_tpm.h"
+#include "qemu_vhost_user_gpu.h"
 
 #include "viralloc.h"
 #include "virlog.h"
@@ -132,11 +133,21 @@ qemuExtDevicesStart(virQEMUDriverPtr driver,
 virDomainDefPtr def,
 qemuDomainLogContextPtr logCtxt)
 {
-int ret = 0;
+int i, ret = 0;
 
 if (qemuExtDevicesInitPaths(driver, def) < 0)
 return -1;
 
+for (i = 0; i < def->nvideos; i++) {
+virDomainVideoDefPtr video = def->videos[i];
+
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER) {
+ret = qemuExtVhostUserGPUStart(driver, def, video, logCtxt);
+if (ret < 0)
+return ret;
+}
+}
+
 if (def->tpm)
 ret = qemuExtTPMStart(driver, def, logCtxt);
 
@@ -148,9 +159,19 @@ void
 qemuExtDevicesStop(virQEMUDriverPtr driver,
virDomainDefPtr def)
 {
+int i;
+
 if (qemuExtDevicesInitPaths(driver, def) < 0)
 return;
 
+for (i = 0; i < def->nvideos; i++) {
+virDomainVideoDefPtr video = def->videos[i];
+
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER) {
+qemuExtVhostUserGPUStop(driver, def, video);
+}
+}
+
 if (def->tpm)
 qemuExtTPMStop(driver, def);
 }
@@ -159,6 +180,13 @@ qemuExtDevicesStop(virQEMUDriverPtr driver,
 bool
 qemuExtDevicesHasDevice(virDomainDefPtr def)
 {
+int i;
+
+for (i = 0; i < def->nvideos; i++) {
+if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER)
+return true;
+}
+
 if (def->tpm && def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR)
 return true;
 
@@ -171,10 +199,19 @@ qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver,
   virDomainDefPtr def,
   virCgroupPtr cgroup)
 {
-int ret = 0;
+int i;
 
-if (def->tpm)
-ret = qemuExtTPMSetupCgroup(driver, def, cgroup);
+for (i = 0; i < def->nvideos; i++) {
+virDomainVideoDefPtr video = def->videos[i];
 
-return ret;
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER &&
+qemuExtVhostUserGPUSetupCgroup(driver, def, video, cgroup) < 0)
+return -1;
+}
+
+if (def->tpm &&
+qemuExtTPMSetupCgroup(driver, def, cgroup) < 0)
+return -1;
+
+return 0;
 }
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 15/17] qemu: build vhost-user-backend for vhost-user-gpu

[marcandre . lureau]

From: Marc-André Lureau 

Pass the vhost-user socket to a chardev, and associate a
vhost-user-backend with it for each vhost-user-gpu.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_command.c | 50 +
 1 file changed, 50 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e85c5c3c1e..b3a2bba28e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4475,6 +4475,38 @@ qemuBuildVgaVideoCommand(virCommandPtr cmd,
 }
 
 
+static char *
+qemuBuildVhostUserBackendStr(virDomainVideoDefPtr video,
+ virCommandPtr cmd,
+ char **chardev)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+if (virAsprintf(chardev, "socket,id=chr-vu-%s,fd=%d",
+video->info.alias, video->info.vhost_user_fd) < 0)
+goto error;
+
+virCommandPassFD(cmd, video->info.vhost_user_fd,
+ VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+
+video->info.vhost_user_fd = -1;
+
+virBufferAsprintf(, "vhost-user-backend,id=vu-%s,chardev=chr-vu-%s",
+  video->info.alias, video->info.alias);
+
+if (virBufferCheckError() < 0)
+goto error;
+
+return virBufferContentAndReset();
+
+error:
+VIR_FREE(*chardev);
+virBufferFreeAndReset();
+return NULL;
+
+}
+
+
 static int
 qemuBuildVideoCommandLine(virCommandPtr cmd,
   const virDomainDef *def,
@@ -4482,6 +4514,24 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
 {
 size_t i;
 
+for (i = 0; i < def->nvideos; i++) {
+char *optstr;
+char *chardev = NULL;
+virDomainVideoDefPtr video = def->videos[i];
+
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER) {
+
+if (!(optstr = qemuBuildVhostUserBackendStr(video, cmd, )))
+return -1;
+
+virCommandAddArgList(cmd, "-chardev", chardev, NULL);
+virCommandAddArgList(cmd, "-object", optstr, NULL);
+
+VIR_FREE(optstr);
+VIR_FREE(chardev);
+}
+}
+
 for (i = 0; i < def->nvideos; i++) {
 char *str = NULL;
 virDomainVideoDefPtr video = def->videos[i];
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 08/17] qemu: vhost-user is valid as non-primary video device

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_domain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 20da58d978..59c63fa2a4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4492,7 +4492,8 @@ qemuDomainDeviceDefValidateVideo(const virDomainVideoDef 
*video)
 
 if (!video->primary &&
 video->type != VIR_DOMAIN_VIDEO_TYPE_QXL &&
-video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
+video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
+video->type != VIR_DOMAIN_VIDEO_TYPE_VHOST_USER) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("video type '%s' is only valid as primary "
  "video device"),
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 05/17] domain: add "vhost-user" video type

[marcandre . lureau]

From: Marc-André Lureau 

Learn to accept "vhost-user" model type:
  

  

(fill the required enum and switches to compile successfully)

Signed-off-by: Marc-André Lureau 
---
 docs/formatdomain.html.in   | 5 +++--
 docs/schemas/domaincommon.rng   | 1 +
 src/conf/domain_conf.c  | 4 +++-
 src/conf/domain_conf.h  | 1 +
 src/qemu/qemu_command.c | 9 ++---
 src/qemu/qemu_domain.c  | 1 +
 src/qemu/qemu_domain_address.c  | 1 +
 tests/domaincapsschemadata/full.xml | 1 +
 8 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index a3afe137bf..8bd0d81dbd 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6620,8 +6620,9 @@ qemu-kvm -net nic,model=? /dev/null
   The model element has a mandatory type
   attribute which takes the value "vga", "cirrus", "vmvga", "xen",
   "vbox", "qxl" (since 0.8.6),
-  "virtio" (since 1.3.0)
-  or "gop" (since 3.2.0)
+  "virtio" (since 1.3.0),
+  "gop" (since 3.2.0) or
+  "vhost-user" (since 4.6.0)
   depending on the hypervisor features available.
 
 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f24a56392a..88dcc5404d 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3451,6 +3451,7 @@
 vbox
 virtio
 gop
+vhost-user
   
 
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7396616eda..93c33263ad 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -590,7 +590,8 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
   "qxl",
   "parallels",
   "virtio",
-  "gop")
+  "gop",
+  "vhost-user")
 
 VIR_ENUM_IMPL(virDomainVideoVGAConf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
   "io",
@@ -15022,6 +15023,7 @@ virDomainVideoDefaultRAM(const virDomainDef *def,
 case VIR_DOMAIN_VIDEO_TYPE_VBOX:
 case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
 case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
+case VIR_DOMAIN_VIDEO_TYPE_VHOST_USER:
 case VIR_DOMAIN_VIDEO_TYPE_GOP:
 case VIR_DOMAIN_VIDEO_TYPE_LAST:
 default:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0f10e242fd..4e608b6ae2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1423,6 +1423,7 @@ typedef enum {
 VIR_DOMAIN_VIDEO_TYPE_PARALLELS, /* pseudo device for VNC in containers */
 VIR_DOMAIN_VIDEO_TYPE_VIRTIO,
 VIR_DOMAIN_VIDEO_TYPE_GOP,
+VIR_DOMAIN_VIDEO_TYPE_VHOST_USER,
 
 VIR_DOMAIN_VIDEO_TYPE_LAST
 } virDomainVideoType;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 45ab1f85ae..e78a534628 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -105,7 +105,8 @@ VIR_ENUM_IMPL(qemuVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
   "qxl",
   "", /* don't support parallels */
   "", /* no need for virtio */
-  "" /* don't support gop */);
+  "", /* don't support gop */
+  "", /* no need for virtio */);
 
 VIR_ENUM_DECL(qemuDeviceVideo)
 
@@ -119,7 +120,8 @@ VIR_ENUM_IMPL(qemuDeviceVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
   "qxl-vga",
   "", /* don't support parallels */
   "virtio-vga",
-  "" /* don't support gop */);
+  "", /* don't support gop */
+  "vhost-user-vga");
 
 VIR_ENUM_DECL(qemuDeviceVideoSecondary)
 
@@ -133,7 +135,8 @@ VIR_ENUM_IMPL(qemuDeviceVideoSecondary, 
VIR_DOMAIN_VIDEO_TYPE_LAST,
   "qxl",
   "", /* don't support parallels */
   "virtio-gpu",
-  "" /* don't support gop */);
+  "", /* don't support gop */
+  "vhost-user-gpu");
 
 VIR_ENUM_DECL(qemuSoundCodec)
 
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ed76495309..b8b5919795 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4485,6 +4485,7 @@ qemuDomainDeviceDefValidateVideo(const virDomainVideoDef 
*video)
 case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
 case VIR_DOMAIN_VIDEO_TYPE_QXL:
 case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
+case VIR_DOMAIN_VIDEO_TYPE_VHOST_USER:
 case VIR_DOMAIN_VIDEO_TYPE_LAST:
 break;
 }
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 6ea80616af..df98d7384f 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -786,6 +786,7 @@ 
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_DEVICE_VIDEO:
 switch ((virDomainVideoType)dev->data.video->type) {
 case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
+case VIR_DOMAIN_VIDEO_TYPE_VHOST_USER:
 return virtioFlags;
 
 

[libvirt] [RFC PATCH 07/17] qemu: check that qemu is vhost-user-vga capable

[marcandre . lureau]

From: Marc-André Lureau 

To support VGA, we need vhost-user-vga device, for "vhost-user" model.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_domain.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b8b5919795..20da58d978 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10488,6 +10488,10 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
 !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_VGA))
 return false;
 
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_VHOST_USER &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_USER_VGA))
+return false;
+
 return true;
 }
 
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 12/17] qemu: restrict 'virgl=' option to 'virtio' video type

[marcandre . lureau]

From: Marc-André Lureau 

vhost-user doesn't have a virgl option, it's given to the
vhost-user-gpu helper process instead.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_command.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e78a534628..e85c5c3c1e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4330,9 +4330,11 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
 
 virBufferAsprintf(, ",id=%s", video->info.alias);
 
-if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON) {
-virBufferAsprintf(, ",virgl=%s",
-  
virTristateSwitchTypeToString(video->accel->accel3d));
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
+if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON) {
+virBufferAsprintf(, ",virgl=%s",
+  
virTristateSwitchTypeToString(video->accel->accel3d));
+}
 }
 
 if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 06/17] qemu: fill the vhost-user video type capability

[marcandre . lureau]

From: Marc-André Lureau 

If vhost-user-gpu is supported, vhost-user video type is.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_capabilities.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a6c00308a2..b517477709 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5036,6 +5036,8 @@ virQEMUCapsFillDomainDeviceVideoCaps(virQEMUCapsPtr 
qemuCaps,
 VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, VIR_DOMAIN_VIDEO_TYPE_QXL);
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU))
 VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, VIR_DOMAIN_VIDEO_TYPE_VIRTIO);
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_USER_GPU))
+VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, 
VIR_DOMAIN_VIDEO_TYPE_VHOST_USER);
 
 return 0;
 }
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 01/17] qemu: setup shared memory without explicit numa configuration

[marcandre . lureau]

From: Marc-André Lureau 

When a domain is configured with 'shared' memory backing:

  

  

But no explicit NUMA configuration, let's configure a shared memory
backend associated with default -numa.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_command.c   | 100 --
 .../fd-memory-no-numa-topology.args   |   4 +
 2 files changed, 73 insertions(+), 31 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 44ae8dcef7..f1235099b2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3254,26 +3254,21 @@ qemuBuildMemoryBackendProps(virJSONValuePtr 
*backendProps,
 
 
 static int
-qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
-  virQEMUDriverConfigPtr cfg,
-  size_t cell,
-  qemuDomainObjPrivatePtr priv,
-  virBufferPtr buf)
+qemuBuildMemoryBackendStr(virDomainDefPtr def,
+  virQEMUDriverConfigPtr cfg,
+  const char *alias,
+  int targetNode,
+  unsigned long long memsize,
+  qemuDomainObjPrivatePtr priv,
+  virBufferPtr buf)
 {
 virJSONValuePtr props = NULL;
-char *alias = NULL;
-int ret = -1;
-int rc;
 virDomainMemoryDef mem = { 0 };
-unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa,
-cell);
-
-if (virAsprintf(, "ram-node%zu", cell) < 0)
-goto cleanup;
+int rc, ret = -1;
 
 mem.size = memsize;
-mem.targetNode = cell;
-mem.info.alias = alias;
+mem.targetNode = targetNode;
+mem.info.alias = (char *)alias;
 
 if ((rc = qemuBuildMemoryBackendProps(, alias, cfg, priv->qemuCaps,
   def, , priv->autoNodeset, 
false)) < 0)
@@ -3284,9 +3279,30 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
 
 ret = rc;
 
+cleanup:
+virJSONValueFree(props);
+return ret;
+}
+
+
+static int
+qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
+  virQEMUDriverConfigPtr cfg,
+  size_t cell,
+  qemuDomainObjPrivatePtr priv,
+  virBufferPtr buf)
+{
+char *alias = NULL;
+int ret = -1;
+unsigned long long memsize = virDomainNumaGetNodeMemorySize(def->numa, 
cell);
+
+if (virAsprintf(, "ram-node%zu", cell) < 0)
+goto cleanup;
+
+ret = qemuBuildMemoryBackendStr(def, cfg, alias, cell, memsize, priv, buf);
+
  cleanup:
 VIR_FREE(alias);
-virJSONValueFree(props);
 
 return ret;
 }
@@ -7590,6 +7606,17 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
 size_t ncells = virDomainNumaGetNodeCount(def->numa);
 const long system_page_size = virGetSystemPageSizeKB();
 bool numa_distances = false;
+bool implicit = false;
+
+if (ncells == 0) {
+if (def->mem.access == VIR_DOMAIN_MEMORY_ACCESS_SHARED) {
+ncells = 1;
+implicit = true;
+} else {
+ret = 0;
+goto cleanup;
+}
+}
 
 if (virDomainNumatuneHasPerNodeBinding(def->numa) &&
 !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
@@ -7645,14 +7672,22 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
 virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) {
 
-if ((rc = qemuBuildMemoryCellBackendStr(def, cfg, i, priv,
-[i])) < 0)
+
+if (implicit)
+rc = qemuBuildMemoryBackendStr(def, cfg, "ram-node", -1,
+   def->mem.total_memory,
+   priv, [i]);
+else
+rc = qemuBuildMemoryCellBackendStr(def, cfg, i,
+   priv, [i]);
+if (rc < 0)
 goto cleanup;
 
 if (rc == 0)
 needBackend = true;
 } else {
-if (virDomainNumaGetNodeMemoryAccessMode(def->numa, i)) {
+if (implicit ||
+virDomainNumaGetNodeMemoryAccessMode(def->numa, i)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Shared memory mapping is not supported "
  "with this QEMU"));
@@ -7667,15 +7702,18 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
 
 for (i = 0; i < ncells; i++) {
 VIR_FREE(cpumask);
-if (!(cpumask = virBitmapFormat(virDomainNumaGetNodeCpumask(def->numa, 
i
-goto cleanup;
 
-if (strchr(cpumask, ',') &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA)) {
-  

[libvirt] [RFC PATCH 04/17] qemu: add vhost-user-gpu capabilities

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_capabilities.c | 6 ++
 src/qemu/qemu_capabilities.h | 4 
 2 files changed, 10 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4ffd2d6257..a6c00308a2 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -504,6 +504,10 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "machine.pseries.cap-htm",
   "usb-storage.werror",
   "memory-backend-memfd",
+
+  /* 315 */
+  "vhost-user-gpu",
+  "vhost-user-vga",
 );
 
 
@@ -1146,6 +1150,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "mch", QEMU_CAPS_DEVICE_MCH },
 { "sev-guest", QEMU_CAPS_SEV_GUEST },
 { "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD },
+{ "vhost-user-gpu", QEMU_CAPS_DEVICE_VHOST_USER_GPU },
+{ "vhost-user-vga", QEMU_CAPS_DEVICE_VHOST_USER_VGA },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index ed612595e0..ee1a19282a 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -489,6 +489,10 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_USB_STORAGE_WERROR, /* -device usb-storage,werror=..,rerror=.. */
 QEMU_CAPS_OBJECT_MEMORY_MEMFD, /* -object memory-backend-memfd */
 
+/* 315 */
+QEMU_CAPS_DEVICE_VHOST_USER_GPU, /* -device vhost-user-gpu */
+QEMU_CAPS_DEVICE_VHOST_USER_VGA, /* -device vhost-user-vga */
+
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 03/17] qemu: use memory-backend-memfd if possible

[marcandre . lureau]

From: Marc-André Lureau 

When the memory is shared and the source isn't specified to be a file,
we can make use of memory-backend-memfd. Sealing is enabled by default
in qemu, hugepage is easier to setup, which makes it a better choice
than memory-backend-file.

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_command.c   | 20 ---
 tests/qemuxml2argvdata/memfd.args | 28 +++
 tests/qemuxml2argvdata/memfd.xml  | 32 +++
 tests/qemuxml2argvtest.c  |  2 ++
 4 files changed, 79 insertions(+), 3 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/memfd.args
 create mode 100644 tests/qemuxml2argvdata/memfd.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f1235099b2..45ab1f85ae 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3137,7 +3137,21 @@ qemuBuildMemoryBackendProps(virJSONValuePtr 
*backendProps,
 if (!(props = virJSONValueNewObject()))
 return -1;
 
-if (useHugepage || mem->nvdimmPath || memAccess ||
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_MEMFD) &&
+memAccess == VIR_DOMAIN_MEMORY_ACCESS_SHARED &&
+!mem->nvdimmPath &&
+def->mem.source != VIR_DOMAIN_MEMORY_SOURCE_FILE) {
+
+backendType = "memory-backend-memfd";
+
+if (useHugepage &&
+virJSONValueObjectAdd(props,
+  "b:hugetlb", true,
+  "U:hugetlbsize", pagesize * 1024,
+  NULL) < 0)
+goto cleanup;
+
+} else if (useHugepage || mem->nvdimmPath || memAccess ||
 def->mem.source == VIR_DOMAIN_MEMORY_SOURCE_FILE) {
 
 if (useHugepage) {
@@ -7670,8 +7684,8 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
  * need to check which approach to use */
 for (i = 0; i < ncells; i++) {
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_RAM) ||
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE)) {
-
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_FILE) ||
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_MEMORY_MEMFD)) {
 
 if (implicit)
 rc = qemuBuildMemoryBackendStr(def, cfg, "ram-node", -1,
diff --git a/tests/qemuxml2argvdata/memfd.args 
b/tests/qemuxml2argvdata/memfd.args
new file mode 100644
index 00..dda8e272ea
--- /dev/null
+++ b/tests/qemuxml2argvdata/memfd.args
@@ -0,0 +1,28 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-object memory-backend-memfd,id=ram-node,size=224395264 \
+-numa node,nodeid=0,memdev=ram-node \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev 
socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait
 \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
+-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/memfd.xml b/tests/qemuxml2argvdata/memfd.xml
new file mode 100644
index 00..c60b169169
--- /dev/null
+++ b/tests/qemuxml2argvdata/memfd.xml
@@ -0,0 +1,32 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  
+
+  
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-i686
+
+  
+  
+  
+
+
+
+
+
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a929e4314e..47bd2ab867 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -945,6 +945,8 @@ mymain(void)
 DO_TEST("pmu-feature", NONE);
 DO_TEST("pmu-feature-off", NONE);
 
+DO_TEST("memfd",
+QEMU_CAPS_OBJECT_MEMORY_MEMFD);
 DO_TEST("hugepages", NONE);
 DO_TEST("hugepages-numa",
 QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4,
-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 00/17] Add vhost-user-gpu support

[marcandre . lureau]

From: Marc-André Lureau 

Hi,

This series of patches add support for running a virtio GPU in a
seperate process, using vhost-user.

The QEMU series "[PATCH v4 00/29] vhost-user for input & GPU" is still
under review, and will hopefully land in 3.1. There are several
benefits of running the GPU process in an external process, since Mesa
is rather heavy on the qemu main loop, and may block for a while or
crash. I observe x5 performance improvements with Unigine Heaven 4
benchmark.

The external GPU process is started with one end of the vhost-user
socket pair, the other end is given to a QEMU chardev. It is also
added to the emulator cgroup to restrict its CPU usage.

vhost-user requires shared VM memory. A few preliminary patches ease
and improve shared memory setup, when no explicit domain NUMA
configuration is given. Also, if there is no need for file-backed
memory, teach libvirt to make use of memfd memory backend, which has
better security guarantees and is easier to setup.

Review welcome!

Marc-André Lureau (17):
  qemu: setup shared memory without explicit numa configuration
  qemu: add memory-backend-memfd capability check
  qemu: use memory-backend-memfd if possible
  qemu: add vhost-user-gpu capabilities
  domain: add "vhost-user" video type
  qemu: fill the vhost-user video type capability
  qemu: check that qemu is vhost-user-vga capable
  qemu: vhost-user is valid as non-primary video device
  qemu: validate vhost-user video model
  qemu: add qemuSecurityStartVhostUserGPU helper
  qemu: add vhost-user-gpu helper unit
  qemu: restrict 'virgl=' option to 'virtio' video type
  qemu: set default address type on vhost-user video model
  qemu: start/stop the vhost-user-gpu external device
  qemu: build vhost-user-backend for vhost-user-gpu
  qemu: build vhost-user-gpu video device arguments
  tests: add vhost-user-gpu xml2argv tests

 docs/formatdomain.html.in |   5 +-
 docs/schemas/domaincommon.rng |   1 +
 src/conf/device_conf.h|   1 +
 src/conf/domain_conf.c|   4 +-
 src/conf/domain_conf.h|   1 +
 src/qemu/Makefile.inc.am  |   2 +
 src/qemu/qemu_capabilities.c  |  10 +
 src/qemu/qemu_capabilities.h  |   5 +
 src/qemu/qemu_command.c   | 191 ---
 src/qemu/qemu_domain.c|   8 +-
 src/qemu/qemu_domain_address.c|   4 +-
 src/qemu/qemu_extdevice.c |  47 ++-
 src/qemu/qemu_process.c   |   6 +-
 src/qemu/qemu_security.c  |  48 +++
 src/qemu/qemu_security.h  |   6 +
 src/qemu/qemu_vhost_user_gpu.c| 318 ++
 src/qemu/qemu_vhost_user_gpu.h|  48 +++
 tests/domaincapsschemadata/full.xml   |   1 +
 .../caps_2.12.0.aarch64.xml   |   1 +
 .../caps_2.12.0.ppc64.xml |   1 +
 .../caps_2.12.0.s390x.xml |   1 +
 .../caps_2.12.0.x86_64.xml|   1 +
 .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml |   1 +
 .../caps_3.0.0.x86_64.xml |   1 +
 .../fd-memory-no-numa-topology.args   |   4 +
 tests/qemuxml2argvdata/memfd.args |  28 ++
 tests/qemuxml2argvdata/memfd.xml  |  32 ++
 .../vhost-user-gpu-secondary.args |  34 ++
 .../vhost-user-gpu-secondary.xml  |  38 +++
 tests/qemuxml2argvdata/vhost-user-vga.args|  31 ++
 tests/qemuxml2argvdata/vhost-user-vga.xml |  35 ++
 tests/qemuxml2argvtest.c  |  14 +
 32 files changed, 877 insertions(+), 51 deletions(-)
 create mode 100644 src/qemu/qemu_vhost_user_gpu.c
 create mode 100644 src/qemu/qemu_vhost_user_gpu.h
 create mode 100644 tests/qemuxml2argvdata/memfd.args
 create mode 100644 tests/qemuxml2argvdata/memfd.xml
 create mode 100644 tests/qemuxml2argvdata/vhost-user-gpu-secondary.args
 create mode 100644 tests/qemuxml2argvdata/vhost-user-gpu-secondary.xml
 create mode 100644 tests/qemuxml2argvdata/vhost-user-vga.args
 create mode 100644 tests/qemuxml2argvdata/vhost-user-vga.xml

-- 
2.18.0.129.ge3331758f1

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

[libvirt] [RFC PATCH 02/17] qemu: add memory-backend-memfd capability check

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
 src/qemu/qemu_capabilities.c   | 2 ++
 src/qemu/qemu_capabilities.h   | 1 +
 tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 +
 tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml   | 1 +
 8 files changed, 9 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 23b483349f..4ffd2d6257 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -503,6 +503,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   "machine.pseries.cap-hpt-max-page-size",
   "machine.pseries.cap-htm",
   "usb-storage.werror",
+  "memory-backend-memfd",
 );
 
 
@@ -1144,6 +1145,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "vhost-vsock-device", QEMU_CAPS_DEVICE_VHOST_VSOCK },
 { "mch", QEMU_CAPS_DEVICE_MCH },
 { "sev-guest", QEMU_CAPS_SEV_GUEST },
+{ "memory-backend-memfd", QEMU_CAPS_OBJECT_MEMORY_MEMFD },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 1fa0ebfea3..ed612595e0 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -487,6 +487,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE, /* -machine 
pseries.cap-hpt-max-page-size */
 QEMU_CAPS_MACHINE_PSERIES_CAP_HTM, /* -machine pseries.cap-htm */
 QEMU_CAPS_USB_STORAGE_WERROR, /* -device usb-storage,werror=..,rerror=.. */
+QEMU_CAPS_OBJECT_MEMORY_MEMFD, /* -object memory-backend-memfd */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
index ecc029f403..e66b8a22ff 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml
@@ -169,6 +169,7 @@
   
   
   
+  
   2011090
   0
   347550
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
index 7139179304..b62cd41bc0 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml
@@ -167,6 +167,7 @@
   
   
   
+  
   2011090
   0
   428334
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
index 87d189e58d..1d2f802040 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml
@@ -133,6 +133,7 @@
   
   
   
+  
   2012000
   0
   375999
diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
index 9c1f6c327c..0fd2d812dd 100644
--- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml
@@ -211,6 +211,7 @@
   
   
   
+  
   2011090
   0
   416196
diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
index 33cd00e613..c6a783353e 100644
--- a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml
@@ -167,6 +167,7 @@
   
   
   
+  
   2012050
   0
   446771
diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
index d7c25c65dd..37e44499ef 100644
--- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml
@@ -210,6 +210,7 @@
   
   
   
+  
   2012050
   0
   437827
-- 
2.18.0.129.ge3331758f1

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

Re: [libvirt] [PATCH v3 7/8] qemu: command: Enable formatting vfio-pci.display option onto cmdline

[Ján Tomko]

On Wed, Jul 11, 2018 at 03:58:27PM +0200, Erik Skultety wrote:

Since QEMU 2.12, QEMU understands a new vfio-pci device option 'display'
which can be used to turn on display capabilities on vgpu-enabled
mediated devices, IOW emulated GPU devices like QXL will no longer be
needed with vgpu-enable mdevs.
QEMU defaults to 'auto' for the 'display' attribute, which is not
foolproof, so we need to play it safe here and explicitly format
display='off' if this attribute wasn't provided in the XML explicitly.

Signed-off-by: Erik Skultety 
---
src/qemu/qemu_command.c| 25 -
.../hostdev-mdev-display-missing-graphics.xml  | 35 ++
.../hostdev-mdev-display-spice-egl-headless.args   | 32 +
.../hostdev-mdev-display-spice-egl-headless.xml| 40 +
.../hostdev-mdev-display-spice-opengl.args | 31 
.../hostdev-mdev-display-spice-opengl.xml  | 41 ++
.../hostdev-mdev-display-vnc-egl-headless.args | 32 +
.../hostdev-mdev-display-vnc-egl-headless.xml  | 40 +
.../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 31 
.../qemuxml2argvdata/hostdev-mdev-display-vnc.xml  | 39 
tests/qemuxml2argvtest.c   | 31 
11 files changed, 376 insertions(+), 1 deletion(-)
create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-missing-graphics.xml
create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.args
create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-spice-egl-headless.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.xml
create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.args
create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-vnc-egl-headless.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 48e224cabc..5f6b340f8f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5207,6 +5207,26 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
virBufferAdd(, dev_str, -1);
virBufferAsprintf(, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);

+/* QEMU 2.12 added support for vfio-pci display type, we need to perform
+ * some additional checks here */
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
+if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("display property of device vfio-pci is "
+ "not supported by this version of QEMU"));
+goto cleanup;
+}


qemuCaps checks belong to *Validate functions.


+} else {
+ /* we default to 'display=off', since QEMU defaults to 'auto' which is
+  * unreliable and we don't want to risk any breakages */
+if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT)
+mdevsrc->display = VIR_TRISTATE_SWITCH_OFF;
+}
+
+if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT)
+virBufferAsprintf(, ",display=%s",
+  virTristateSwitchTypeToString(mdevsrc->display));
+
if (qemuBuildDeviceAddressStr(, def, dev->info, qemuCaps) < 0)
goto cleanup;




@@ -5424,7 +5444,9 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,

/* MDEV */
if (virHostdevIsMdevDevice(hostdev)) {
-switch ((virMediatedDeviceModelType) subsys->u.mdev.model) {
+virDomainHostdevSubsysMediatedDevPtr mdevsrc = >u.mdev;
+
+switch ((virMediatedDeviceModelType) mdevsrc->model) {
case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -5432,6 +5454,7 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
 "supported by this version of QEMU"));
return -1;
}
+
break;
case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {


These two hunks are unrelated. Feel free to push them as trivial in a
separate commit.


diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 6014c81802..a0ab824038 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1590,6 +1590,37 @@ mymain(void)
QEMU_CAPS_DEVICE_VFIO_PCI);
DO_TEST_PARSE_ERROR("hostdev-mdev-invalid-target-address",
QEMU_CAPS_DEVICE_VFIO_PCI);
+DO_TEST("hostdev-mdev-display-spice-opengl",
+

Re: [libvirt] [PATCH v3 6/8] conf: Introduce new attribute 'display'

[Ján Tomko]

On Wed, Jul 11, 2018 at 03:58:26PM +0200, Erik Skultety wrote:

QEMU 2.12 introduced a new type of display for mediated devices using
vfio-pci backend which allows a mediated device to be used as a VGA
compatible device as an alternative to an emulated video device. QEMU
exposes this feature via a vfio device property 'display' with supported
values 'on/off/auto' (default is 'off').

This patch adds the necessary bits to domain config handling in order to
expose this feature. Since there's no convenient way for libvirt to come
up with usable defaults for the display setting, simply because libvirt
is not able to figure out which of the display implementations - dma-buf
which requires OpenGL support vs vfio regions which doesn't need OpenGL
(works with OpenGL enabled too) - the underlying mdev uses.

Signed-off-by: Erik Skultety 
---
docs/formatdomain.html.in  | 20 ++--
docs/schemas/domaincommon.rng  |  5 ++
src/conf/domain_conf.c | 19 +++-
src/conf/domain_conf.h |  1 +
src/qemu/qemu_domain.c | 55 ++
tests/qemuxml2argvdata/hostdev-mdev-display.xml| 39 +++
.../hostdev-mdev-display-active.xml| 47 ++
tests/qemuxml2xmltest.c|  2 +
8 files changed, 184 insertions(+), 4 deletions(-)
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display.xml
create mode 100644 tests/qemuxml2xmloutdata/hostdev-mdev-display-active.xml

@@ -7752,6 +7754,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
   model);
goto cleanup;
}
+
+if (display &&
+(mdevsrc->display = virTristateSwitchTypeFromString(display)) <= 
0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("unknown value '%s' for  attribute "
+ "'display'"),
+   display);
+goto cleanup;
+}
}

switch (def->source.subsys.type) {



diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3deda1d978..8ca9558ceb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -382,6 +382,7 @@ typedef struct _virDomainHostdevSubsysMediatedDev 
virDomainHostdevSubsysMediated
typedef virDomainHostdevSubsysMediatedDev *virDomainHostdevSubsysMediatedDevPtr;
struct _virDomainHostdevSubsysMediatedDev {
int model;  /* enum virMediatedDeviceModelType */
+int display; /* virTristateSwitch */


I was convinced that using the enum type here is the right way, but I've
had my illusions shattered.

If you decide to go that way anyway, don't forget to use a temporary int
variable in the parser, since the compiler can choose to make
virTristateSwitch unsinged and it won't be able to hold the negative
return value from virTristateSwitchTypeFromString.


char uuidstr[VIR_UUID_STRING_BUFLEN];   /* mediated device's uuid string */
};

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5da8c8bfcc..a03b1fb029 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4450,10 +4450,47 @@ qemuDomainDeviceDefValidateNetwork(const 
virDomainNetDef *net)
}


+static int
+qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc,
+  const virDomainDef *def)
+{
+if (mdevsrc->display != VIR_TRISTATE_SWITCH_ABSENT &&
+mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _(" attribute 'display' is only supported"
+ " with model='vfio-pci'"));
+
+return -1;
+}
+
+if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) {
+if (def->ngraphics == 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("graphics device is needed for attribute value "
+ "'display=on' in "));
+return -1;
+}
+
+/* We're not able to tell whether an mdev needs OpenGL or not at the
+ * moment, so print a warning that an extra  element or
+ *  attribute 'display' may need the OpenGL to "
+ "be enabled");


I'd suggest dropping the warning - nobody reads warnings.


+}
+
+return 0;
+}
+
+
static int


Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v3 5/8] conf: Replace 'error' with 'cleanup' in virDomainHostdevDefParseXMLSubsys

[Ján Tomko]

On Wed, Jul 11, 2018 at 03:58:25PM +0200, Erik Skultety wrote:

The exit path is the same for both success and failure, so the label
should be called cleanup.

Signed-off-by: Erik Skultety 
---
src/conf/domain_conf.c | 38 +++---
1 file changed, 19 insertions(+), 19 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v3 4/8] conf: Introduce virDomainGraphicsDefHasOpenGL helper

[Ján Tomko]

On Wed, Jul 11, 2018 at 03:58:24PM +0200, Erik Skultety wrote:

A simple helper which will loop through all the graphics elements and
checks whether at least one of them enables OpenGL support, either by
containing  or being of type 'egl-headless'.

Signed-off-by: Erik Skultety 
Acked-by: Michal Privoznik 
---
src/conf/domain_conf.c   | 43 +++
src/conf/domain_conf.h   |  3 +++
src/libvirt_private.syms |  1 +
3 files changed, 47 insertions(+)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [REPOST PATCHv2 2/2] qemu: Use the correct vm def on cold attach

[John Ferlan]

On 07/09/2018 10:32 AM, Michal Privoznik wrote:
> On 07/06/2018 08:50 PM, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1559867
>>
>> When attaching a device to the domain we need to be sure
>> to use the correct domain definition (vm->def or vm->newDef)
>> when calling virDomainDeviceDefParse because the post parse
>> processing algorithms that may assign an address for the
>> device will use whatever domain definition was passed in.
>>
>> Additionally, some devices (SCSI hostdev and SCSI disk) use
>> algorithms that rely on knowing what already exists of the
>> other type when generating the new device's address. Using
>> the wrong VM definition could result in duplicated addresses.
>>
>> In the case of the bz, two hostdev's with no domain address
>> provided were added to the running domain's config only.
>> However, the parsing algorithm used the live domain in
>> order to figure out the host device address resulting in
>> the same address being used and a subsequent start failing
>> due to duplicate address.
>>
>> Fix this by separating the checks/code into CONFIG and LIVE
>> processing using the correct definition for each block and
>> peforming cleanup for both options as necessary.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_driver.c | 52 
>> ++
>>  1 file changed, 23 insertions(+), 29 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index ef1abe3f68..60085befea 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -8469,7 +8469,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
>>  {
>>  virDomainDefPtr vmdef = NULL;
>>  virQEMUDriverConfigPtr cfg = NULL;
>> -virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
>> +virDomainDeviceDefPtr devConf = NULL;
>> +virDomainDeviceDefPtr devLive = NULL;
>>  int ret = -1;
>>  virCapsPtr caps = NULL;
>>  unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
>> @@ -8483,49 +8484,43 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr 
>> vm,
>>  if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>>  goto cleanup;
>>  
>> -dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
>> - caps, driver->xmlopt,
>> - parse_flags);
>> -if (dev == NULL)
>> -goto cleanup;
>> -
>> -if (virDomainDeviceValidateAliasForHotplug(vm, dev, flags) < 0)
>> -goto cleanup;
>> -
>> -if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
>> -flags & VIR_DOMAIN_AFFECT_LIVE) {
>> -/* If we are affecting both CONFIG and LIVE
>> - * create a deep copy of device as adding
>> - * to CONFIG takes one instance.
>> - */
>> -dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, 
>> driver->xmlopt);
>> -if (!dev_copy)
>> -goto cleanup;
>> -}
>> -
>> +/* The config and live post processing address auto-generation 
>> algorithms
>> + * rely on the correct vm->def or vm->newDef being passed, so call the
>> + * device parse based on which definition is in use */
>>  if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
>> -/* Make a copy for updated domain. */
>>  vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
>>  if (!vmdef)
>>  goto cleanup;
>>  
>> -if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
>> +if (!(devConf = virDomainDeviceDefParse(xml, vmdef, caps,
>> +driver->xmlopt, 
>> parse_flags)))
>> +goto cleanup;
>> +
>> +if (virDomainDefCompatibleDevice(vmdef, devConf, NULL,
>>   VIR_DOMAIN_DEVICE_ACTION_ATTACH,
>>   false) < 0)
>>  goto cleanup;
>> -if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps,
>> +
>> +if ((ret = qemuDomainAttachDeviceConfig(vmdef, devConf, caps,
>>  parse_flags,
>>  driver->xmlopt)) < 0)
>>  goto cleanup;
>>  }
>>  
>>  if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>> -if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
>> +if (!(devLive = virDomainDeviceDefParse(xml, vm->def, caps,
>> +driver->xmlopt, 
>> parse_flags)))
>> +goto cleanup;
>> +
> 
> In the light of 84de7fbfdb2f528e05d98c09e3260fe0fc0739f9: shouldn't we
> parse this as live XML?
> 

Let's see:

qemuDomainAttachDeviceFlags calls virDomainObjUpdateModificationImpact
and then calls qemuDomainAttachDeviceLiveAndConfig, so if I'm reading
your question properly, then I think we're good here.

> Also don't other drivers suffer the same problem (even though I vaguely
> recall you posting a patch 

Re: [libvirt] [REPOST PATCHv2 1/2] qemu: Check for existing hostdev address for cold attach device

[John Ferlan]

On 07/09/2018 10:32 AM, Michal Privoznik wrote:
> On 07/06/2018 08:50 PM, John Ferlan wrote:
>> Prior to the hostdev being inserted in the hostdevs list,
>> add a check during qemuDomainAttachDeviceConfig to determine
>> whether the new/incoming  device is providing
>> the same  as some existing hostdev on the list
>> and if so fail the cold attach.
>>
>> This cannot be done during virDomainHostdevDefPostParse
>> because that is called after virDomainDefParseXML would
>> have inserted a hostdev onto the hostdevs list and thus
>> would have a "conflict" with itself. Therefore, the post
>> parse processing can only compare if the current hostdev
>> address conflicts with a SCSI  address.
>>
>> By comparison this is similar to the validation phase
>> checks in virDomainDefCheckDuplicateDriveAddresses that
>> occur during define/startup processing but are not run
>> during config attach of a live guest.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_driver.c | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 9a35e04a85..ef1abe3f68 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -8007,6 +8007,12 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
>> _("device is already in the domain 
>> configuration"));
>>  return -1;
>>  }
>> +if (dev->data.hostdev->info->type != 
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>> +virDomainDefHasDeviceAddress(vmdef, dev->data.hostdev->info)) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +   _("a device with the same address already exists 
>> "));
>> +return -1;
>> +}
>>  if (virDomainHostdevInsert(vmdef, hostdev))
>>  return -1;
>>  dev->data.hostdev = NULL;
>>
> 
> I think hostdevs are not the only type of device suffering from this. In
> fact, I've just tested disks and libvirt accepts attaching another disk
> onto same  happily.
> 
> I wonder if this should go into virDomainDefCompatibleDevice() (now that
> we have @action there ;-) ).
> 

It's a case of myopia for the bug I'm working on as listed in patch2.

What I did is no different than the VIR_DOMAIN_DEVICE_RNG check in the
same function.

Still, if I change this patch to add:

 if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH && !live &&
data.newInfo &&
data.newInfo->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
virDomainDefHasDeviceAddress(def, data.newInfo)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
   _("a device with the same address already exists "));
return -1;
}

to virDomainDefCompatibleDevice and remove the (new)hostdev and
(existing)rng checks, then I believe it covers the existing cases.

Tks -

John

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

Re: [libvirt] [PATCH v3 3/8] qemu: caps: Add vfio-pci.display capability

[Ján Tomko]

On Wed, Jul 11, 2018 at 03:58:23PM +0200, Erik Skultety wrote:

QEMU 2.12 introduced a new vfio-pci device option 'display=on/off/auto'.
This patch introduces the necessary capability.

Signed-off-by: Erik Skultety 
Reviewed-by: John Ferlan 
---
src/qemu/qemu_capabilities.c   | 4 
src/qemu/qemu_capabilities.h   | 3 +++
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  | 1 +
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml   | 1 +
8 files changed, 13 insertions(+)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v3 2/8] conf: Introduce a new graphics display type 'headless'

[Ján Tomko]

s/conf/qemu/ in the commit summary, since you chose to group the XML
parser and the QEMU driver changes in one commit.

On Wed, Jul 11, 2018 at 03:58:22PM +0200, Erik Skultety wrote:

Since 2.10 QEMU supports a new display type egl-headless which uses the
drm nodes for OpenGL rendering copying back the rendered bits back to
QEMU into a dma-buf which can be accessed by standard "display" apps
like VNC or SPICE. Although this display type can be used on its own,
for any practical use case it makes sense to pair it with either VNC or
SPICE display. The clear benefit of this display is that VNC gains
OpenGL support, which it natively doesn't have, and SPICE gains remote
OpenGL support (native OpenGL support only works locally through a UNIX
socket, i.e. listen type=socket/none)

Signed-off-by: Erik Skultety 
---
docs/formatdomain.html.in  | 33 +-
docs/schemas/domaincommon.rng  |  3 ++
src/conf/domain_conf.c |  6 ++-
src/conf/domain_conf.h |  1 +
src/libxl/libxl_conf.c |  1 +
src/qemu/qemu_command.c| 35 ++-
src/qemu/qemu_domain.c | 52 +-
src/qemu/qemu_driver.c |  2 +
src/qemu/qemu_hotplug.c|  1 +
src/qemu/qemu_process.c|  4 ++
src/vmx/vmx.c  |  1 +
tests/domaincapsschemadata/full.xml|  1 +
tests/qemuxml2argvdata/graphics-egl-headless.args  | 26 +++
tests/qemuxml2argvdata/graphics-egl-headless.xml   | 31 +
.../qemuxml2argvdata/graphics-sdl-egl-headless.xml | 35 +++
.../graphics-spice-egl-headless.args   | 31 +
.../graphics-spice-egl-headless.xml| 36 +++
.../graphics-spice-invalid-egl-headless.xml| 37 +++
.../graphics-vnc-egl-headless.args | 28 
.../qemuxml2argvdata/graphics-vnc-egl-headless.xml | 37 +++
tests/qemuxml2argvtest.c   | 17 +++
.../graphics-spice-egl-headless.xml| 44 ++
.../graphics-vnc-egl-headless.xml  | 42 +
tests/qemuxml2xmltest.c|  2 +
24 files changed, 501 insertions(+), 5 deletions(-)
create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless.args
create mode 100644 tests/qemuxml2argvdata/graphics-egl-headless.xml
create mode 100644 tests/qemuxml2argvdata/graphics-sdl-egl-headless.xml
create mode 100644 tests/qemuxml2argvdata/graphics-spice-egl-headless.args
create mode 100644 tests/qemuxml2argvdata/graphics-spice-egl-headless.xml
create mode 100644 
tests/qemuxml2argvdata/graphics-spice-invalid-egl-headless.xml
create mode 100644 tests/qemuxml2argvdata/graphics-vnc-egl-headless.args
create mode 100644 tests/qemuxml2argvdata/graphics-vnc-egl-headless.xml
create mode 100644 tests/qemuxml2xmloutdata/graphics-spice-egl-headless.xml
create mode 100644 tests/qemuxml2xmloutdata/graphics-vnc-egl-headless.xml




diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 44ae8dcef7..48e224cabc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8213,6 +8213,27 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr 
cfg,
return -1;
}

+
+static int
+qemuBuildGraphicsHeadlessCommandLine(virQEMUDriverConfigPtr cfg 
ATTRIBUTE_UNUSED,
+ virCommandPtr cmd,
+ virQEMUCapsPtr qemuCaps,
+ virDomainGraphicsDefPtr def 
ATTRIBUTE_UNUSED)


These arguments are unused now as well as at the end of the series and
can be dropped.


+{
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_EGL_HEADLESS)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("egl-headless display is not supported with this "
+ "QEMU binary"));
+return -1;
+}
+


This belongs in qemu.*Validate.


+virCommandAddArg(cmd, "-display");
+virCommandAddArg(cmd, "egl-headless");
+
+return 0;
+}
+
+
static int
qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
 virCommandPtr cmd,
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ed76495309..5da8c8bfcc 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5528,6 +5528,53 @@ qemuDomainDeviceDefValidateTPM(virDomainTPMDef *tpm,
}


+static int
+qemuDomainDeviceDefValidateGraphics(const virDomainGraphicsDef *graphics,
+const virDomainDef *def)
+{
+bool have_egl_headless = false;
+size_t i;
+
+/* are we running with egl-headless? */


The variable name is self-explanatory, no need to explain the for cycle
here.


+for (i = 0; i < def->ngraphics; i++) {

Re: [libvirt] [PATCH v3 1/8] qemu: caps: Introduce a capability for egl-headless

[Ján Tomko]

On Wed, Jul 11, 2018 at 03:58:21PM +0200, Erik Skultety wrote:

Since QEMU 2.10, it's possible to use a new type of display -
egl-headless which uses drm nodes to provide OpenGL support. This patch
adds a capability for that. However, since QEMU doesn't provide a QMP
command to probe it, we have to base the capability on specific QEMU
version.

Signed-off-by: Erik Skultety 
Acked-by: Michal Privoznik 
---
src/qemu/qemu_capabilities.c   | 6 ++
src/qemu/qemu_capabilities.h   | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.ppc64.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  | 1 +
tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  | 1 +
tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml| 1 +
tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml   | 1 +
13 files changed, 18 insertions(+)



Sigh.

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 0/6] tests: qemu: Fix testing QMP commands against the schema

[Ján Tomko]

On Thu, Jul 12, 2018 at 01:58:38PM +0200, Peter Krempa wrote:

I was trying to add some new commands and made a typo. The tests did not
catch it. I must have messed up something when sending the original
schema validation impl.

Peter Krempa (6):
 tests: qemuschema: Fix copy-paste error in function name
 tests: qemuschema: Add line break to debug message
 tests: qemumonitorutils: Don't crash on wrong monitor command
 tests: qemumonitorjson: Raise the necessary debug level for QAPI
   schema checks
 tests: qemumonitorjson: Fix schema testing of monitor commands
 tests: qemumonitorjson: Do QMP schema validation for DO_TEST_GEN

tests/qemumonitorjsontest.c  | 10 ++
tests/qemumonitortestutils.c |  3 ++-
tests/testutilsqemuschema.c  | 10 +-
3 files changed, 13 insertions(+), 10 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH 4/6] tests: qemumonitorjson: Raise the necessary debug level for QAPI schema checks

[Ján Tomko]

On Thu, Jul 12, 2018 at 01:58:42PM +0200, Peter Krempa wrote:

The debug output of the schema validator on success is not so
interresting that it should be printed when basic debugging is enabled.



*interesting


Print it only when test debugging is set to 3 and more.

Signed-off-by: Peter Krempa 
---
tests/qemumonitorjsontest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)


Jano


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

Re: [libvirt] [jenkins-ci PATCH v2 02/12] lcitool: Stub out Python implementation

[Ján Tomko]

On Thu, Jul 12, 2018 at 05:19:19PM +0200, Andrea Bolognani wrote:

Doesn't do much right now, but it's a start :)

Signed-off-by: Andrea Bolognani 
---
guests/lcitool | 69 ++
1 file changed, 69 insertions(+)
create mode 100755 guests/lcitool

diff --git a/guests/lcitool b/guests/lcitool
new file mode 100755
index 000..1cba8ad
--- /dev/null
+++ b/guests/lcitool
@@ -0,0 +1,69 @@
+#!/usr/bin/env python
+
+# lcitool - libvirt CI guest management tool
+# Copyright (C) 2017-2018  Andrea Bolognani 
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by the
+# Free Software Foundation; either version 2 of the License, or (at your
+# option) any later version.
+#
+# This program 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 General
+# Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+


Please use:
   You should have received a copy of the GNU General Public License
   along with this program.  If not, see .

Per the recommendation on https://www.gnu.org/licenses/gpl-howto.html
instead of listing the physical address.

Jano


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

Re: [libvirt] [PATCH 2/4] conf: Move hugepage XML validation check out of qemu_command

[Pavel Hrdina]

On Wed, Jul 11, 2018 at 06:03:08PM +0200, Pavel Hrdina wrote:
> To make it clear I'll summarize all the possible combinations and how it
> should work so we are on the same page.

originally: before commit [1]
now: after commit [1] (current master)
expect: what this patch series should fix


=== Non-NUMA guests ===


* Only one hugepage specified without any nodeset


  

  


This is correct, was always working and we should not change it.

originally: works
now: works
expected: works


* Only one hugapage specified with nodeset


  

  


This is questionable since there is no guest NUMA node specified,
but on the other hand we can consider non-NUMA guest to have exactly
one NUMA node.

This was working but has been broken by commit [1]  which tried to
fix a case where you are trying to specify non-existing NUMA node.

Because of that assumption we can consider this as valid XML even
though there is no NUMA node specified [2].  There are two possible
solutions:

- we can leave the XML intact

- we can silently remove the 'nodeset' attribute to 'fix' the
  XML definition

originally: works
now: fails
expect: works



  

  


If the nodeset is != 0 it should newer work becuase there is no
guest NUMA topology and even if we take into account the assumption
that there is always one NUMA node it is still invalid.

originally: works
now: fails
expect: fails


* One hugepage with specific nodeset and second default hugepage


  


  


This was working but was 'fixed' by commit [1] because the code
checks only the first hugepage and uses only the first hugepage.

It should never worked because it doesn't make any sense, there
is no guest NUMA node configured and even if we take into account
the assumption that non-NUMA guest has one NUMA node there is need
for the default page size.

originally: works
now: fails
expect: fails


There is yet another issue with the current state in libvirt, if
you swap the order of pages:


  


  


it will work even with current libvirt with the fix [1].  The reason
is that code in qemuBuildMemPathStr() function takes into account
only the first page size so it depends on the order of elements
which is wrong.

We should not allow any of these two configurations.  Setting
nodeset to != 0 will should not make any difference.

originally: works
now: works
expect: fails


== NUMA guest ==


* Only one hugepage specified without any nodeset


  

  

...

  
  


  


originally: works
now: works
expect: works


* Only one hugapage specified with nodeset


  

  

...

  
  


  


All possible combinations for the nodeset attribute are allowed
if it always corresponds to existing guest NUMA node:



or



originally: works
now: works
expect: works



  

  

...

  
  


  


There is invalid guest NUMA node specified for the hugepage.

originally: fails
now: fails
expect: fails

* One hugepage with specific nodeset and second default hugepage


  


  

...

  
  


  


There are two guest NUMA nodes where we have default hugepage size
configured and for NUMA node '0' we have a different size.

originally: works
now: works
expect: works



  


  

...

  
  


  


originally: works
now: works
expect: fails ???

In this situation all the guest NUMA nodes are covered by the
hugepage size with specified nodeset attribute.  The default one
is not used at all so is pointless here.

The difference between this and non-NUMA guest is that if we change
the order it will still work as expected, it doesn't depend on the
order of elements.  However, we might consider is as invalid
configuration because there is no need to have the default page size
configured at all.


* Multiple combination of default and specific hugepage sizes

There are some restriction if we use multiple page sizes:

- There cannot be two different default hugepage sizes

- Two different page elements cannot have the same guest NUMA
  node specified in nodeset attribute

- hugepages are not allowed if memory backing allocation is set
  to 'ondemand' (not documented)

- hugepages are not allowed if memory backing source is set to
  'anonymous' (not 

Re: [libvirt] [PATCH v2 3/3] docs: news: Provide an update about the video type 'none'

[Ján Tomko]

On Thu, Jul 12, 2018 at 05:08:48PM +0200, Erik Skultety wrote:

Signed-off-by: Erik Skultety 
---
docs/news.xml | 14 ++
1 file changed, 14 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 773c95b0da..93832acc4c 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -46,6 +46,20 @@
  


+  
+
+  qemu: Introduce a new video model of type 'none'
+
+
+  Historically, libvirt would add a default 'cirrus' video device to
+  a guest if the XML specified 'graphics' but lacked 'video'.


Historical behavior is hardly news :P


This can
+  be incovenient with GPU mediated devices which can serve as the only
+  rendering devices within the guest, rather than still relying on an
+  emulated GPU which would also be the primary device. Having a 'none'
+  model is our only backwards compatible option how to turn this legacy
+  behaviour off when needed.


How about:

Introduce a new video model type that disables the automatic addition of
a video device to domains with 'graphics' specified in their XML.

This can be useful with GPU mediated devices which can serve as the only
rendering devices within the guest.

Jano


+
+  



--
2.14.4

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


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

[libvirt] [PATCH v2] virnetdevtap: Don't crash on !ifname in virNetDevTapInterfaceStats

[Michal Privoznik]

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

Some domain  do not have a name (because they are
not TAP devices). Therefore, if
virNetDevTapInterfaceStats(net->ifname, ...) is called an instant
crash occurs. In Linux version of the function strlen() is called
over the name and in BSD version STREQ() is called.

Signed-off-by: Michal Privoznik 
---

diff to v1:
- changed the error message
- fixed check in BSD version

 src/util/virnetdevtap.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index bd0710ad2e..3118ca18e8 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -691,6 +691,12 @@ virNetDevTapInterfaceStats(const char *ifname,
 FILE *fp;
 char line[256], *colon;
 
+if (!ifname) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Interface name not provided"));
+return -1;
+}
+
 fp = fopen("/proc/net/dev", "r");
 if (!fp) {
 virReportSystemError(errno, "%s",
@@ -768,6 +774,12 @@ virNetDevTapInterfaceStats(const char *ifname,
 struct if_data *ifd;
 int ret = -1;
 
+if (!ifname) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Interface name not provided"));
+return -1;
+}
+
 if (getifaddrs() < 0) {
 virReportSystemError(errno, "%s",
  _("Could not get interface list"));
-- 
2.16.4

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

Re: [libvirt] [PATCH v2 2/3] conf: Introduce new video type 'none'

[Ján Tomko]

On Thu, Jul 12, 2018 at 05:08:47PM +0200, Erik Skultety wrote:

Historically, we've always enabled an emulated video device every time we
see that graphics should be supported with a guest. With the appearance
of mediated devices which can support QEMU's vfio-display capability,
users might want to use such a device as the only video device.
Therefore introduce a new, effectively a 'disable', type for video
device.

Signed-off-by: Erik Skultety 
---
docs/formatdomain.html.in  | 13 -
docs/schemas/domaincommon.rng  |  1 +
src/conf/domain_conf.c | 55 --
src/conf/domain_conf.h |  1 +
src/qemu/qemu_command.c| 14 --
src/qemu/qemu_domain.c |  3 ++
src/qemu/qemu_domain_address.c | 10 
tests/domaincapsschemadata/full.xml|  1 +
.../video-invalid-multiple-devices.xml | 33 +
tests/qemuxml2argvdata/video-none-device.args  | 27 +++
tests/qemuxml2argvdata/video-none-device.xml   | 39 +++
tests/qemuxml2argvtest.c   |  4 +-
tests/qemuxml2xmloutdata/video-none-device.xml | 42 +
tests/qemuxml2xmltest.c|  1 +
14 files changed, 223 insertions(+), 21 deletions(-)
create mode 100644 tests/qemuxml2argvdata/video-invalid-multiple-devices.xml
create mode 100644 tests/qemuxml2argvdata/video-none-device.args
create mode 100644 tests/qemuxml2argvdata/video-none-device.xml
create mode 100644 tests/qemuxml2xmloutdata/video-none-device.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 84ab5a9d12..03d94f0533 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6622,9 +6622,18 @@ qemu-kvm -net nic,model=? /dev/null
  The model element has a mandatory type
  attribute which takes the value "vga", "cirrus", "vmvga", "xen",
  "vbox", "qxl" (since 0.8.6),
-  "virtio" (since 1.3.0)
-  or "gop" (since 3.2.0)
+  "virtio" (since 1.3.0),
+  "gop" (since 3.2.0), or
+  "none" (since 4.6.0)
  depending on the hypervisor features available.
+  The purpose of the type none is to instruct libvirt not
+  to add a default video device in the guest (see the paragraph above).
+  This legacy behaviour can be inconvenient in cases where GPU mediated
+  devices are meant to be the only rendering device within a guest and
+  so specifying another video device along with type
+  none.
+  Refer to Host device assignment to see
+  how to add a mediated device into a guest.


  You can provide the amount of video memory in kibibytes (blocks of
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index bd687ce9d3..ed989c000f 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3451,6 +3451,7 @@
vbox
virtio
gop
+none
  


diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7396616eda..d4927f0226 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -590,7 +590,8 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
  "qxl",
  "parallels",
  "virtio",
-  "gop")
+  "gop",
+  "none")

VIR_ENUM_IMPL(virDomainVideoVGAConf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
  "io",
@@ -5091,25 +5092,48 @@ static int
virDomainDefPostParseVideo(virDomainDefPtr def,
   void *opaque)
{
+size_t i;
+
if (def->nvideos == 0)
return 0;

-virDomainDeviceDef device = {
-.type = VIR_DOMAIN_DEVICE_VIDEO,
-.data.video = def->videos[0],
-};
-
-/* Mark the first video as primary. If the user specified
- * primary="yes", the parser already inserted the device at
- * def->videos[0]
+/* it doesn't make sense to pair video device type 'none' with any other
+ * types, there can be only a single video device in such case
 */
-def->videos[0]->primary = true;
+for (i = 0; i < def->nvideos; i++) {
+if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE &&
+def->nvideos > 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("a '%s' video type must be the only video device "
+ "defined for the domain"));
+return -1;
+}
+}


This looks like something virDomainDefValidate should do.



-/* videos[0] might have been added in AddImplicitDevices, after we've
- * done the per-device post-parse */
-if (virDomainDefPostParseDeviceIterator(def, ,
-

Re: [libvirt] [PATCH v2 1/3] docs: Rephrase the mediated devices hostdev section a bit

[Ján Tomko]

On Thu, Jul 12, 2018 at 05:08:46PM +0200, Erik Skultety wrote:

Currently it reads:
Refer MDEV to create a mediated device on the host

...even though it resembles English, it's not a proper English.

Signed-off-by: Erik Skultety 
---
docs/formatdomain.html.in | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)



Looks like a more properer English now.

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial

[Cornelia Huck]

On Thu, 12 Jul 2018 17:47:00 +0200
Thomas Huth  wrote:

> On 12.07.2018 08:32, Markus Armbruster wrote:
> > Daniel P. Berrangé  writes:  
> [...]
> >> For libvirt, I think whenever something is proposed for deprecation
> >> we could just CC libvir-list, or ask one of the libvirt people to
> >> confirm its not being used. If it is, then we should file BZ against
> >> libvirt.  
> > 
> > Makes sense, but relying on developers getting their cc: right every
> > time is a setting us up for failure.
> > 
> > Our tool to help with getting cc: wrong less often is the MAINTAINERS
> > file.  Could one of the libvirt developers watch changes to qemu-doc
> > appendix "Deprecated features"?  Would putting the appendix in its own
> > .texi help with that?  
> 
> Sound like a good idea to put the appendix in its own texi file. Then
> add an "R: libvir-list" entry for that file to MAINTAINERS and we should
> be fine (at least for the people who use the get_maintainers.pl script).

+1, like that idea

Are there other consumers of QEMU's interfaces which should be cc:ed in
a similar way?

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

Re: [libvirt] [PATCH] virnetdevtap: Don't crash on !ifname in virNetDevTapInterfaceStats

[Andrea Bolognani]

On Fri, 2018-07-13 at 11:24 +0200, Michal Privoznik wrote:
[...]
> +if (!ifname) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("Interface not found"));
> +return -1;
> +}

I would use something like "Interface name not provided" here,
as it would better describe the cause of the error.

[...]
>  if (ifa->ifa_addr->sa_family != AF_LINK)
>  continue;
>  
> -if (STREQ(ifa->ifa_name, ifname)) {
> +if (STREQ_NULLABLE(ifa->ifa_name, ifname)) {

Why is the fix different for the BSD implementation? I would
expect you to just add the same snippet as above to guard the
rest of the function from ifname being NULL...

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [jenkins-ci PATCH] guests: Stop using search() as a filter

[Erik Skultety]

On Fri, Jul 13, 2018 at 10:56:56AM +0200, Andrea Bolognani wrote:
> Recent versions of Ansible complain about this, and suggest
> to replace 'result|search' with 'result is search'.
>
> Since f17097c7af59, however, we've been including operating
> system information in the inventory: this allows us to drop
> our use of search() entirely.
>
> Signed-off-by: Andrea Bolognani 

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCHv2 06/11] qemu_monitor: Introduce qemuMonitorCPUModelInfoRemovePropByBoolValue

[Jiri Denemark]

On Mon, Jul 09, 2018 at 22:56:50 -0500, Chris Venteicher wrote:
> Filter out cpu properties in qemuMonitorCPUModelInfo structure based on
> boolean value of true or false.
> 
> Goal is to form a list of "enabled" or "disabled" properties.
> 
> Required to convert between cpu model feature / property lists that
> indicate if property is or isn't include in model and the form of cpu
> model feature / property lists that only enumerate properties that are
> actually included in the model.
> ---
>  src/qemu/qemu_monitor.c | 29 +
>  src/qemu/qemu_monitor.h |  4 
>  2 files changed, 33 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 91b946c8b4..dd8510fbab 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3766,6 +3766,35 @@ qemuMonitorCPUModelInfoCopy(const 
> qemuMonitorCPUModelInfo *orig)
>  return NULL;
>  }
>  
> +
> +/* Squash CPU Model Info property list
> + * removing props of type boolean matching value */

The usefulness of this new function depends a question I'll ask in later
review of patch 11/11.

> +void
> +qemuMonitorCPUModelInfoRemovePropByBoolValue(qemuMonitorCPUModelInfoPtr 
> model,
> + bool value)
> +{
> +qemuMonitorCPUPropertyPtr src, dst;
> +size_t i, dst_size = 0;

Single variable per line, please.

> +
> +for (i = 0; i < model->nprops; i++) {
> +src = &(model->props[i]);
> +dst = &(model->props[dst_size]);
> +
> +if ((src->type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN) &&
> +(src->value.boolean == value))

Redundant parentheses.

> +continue;
> +
> +*dst = *src;
> +
> +dst_size++;
> +}
> +
> +model->nprops = dst_size;
> +
> +ignore_value(VIR_REALLOC_N(model->props, dst_size)); /* not fatal */

This should call VIR_REALLOC_N_QUIET and I think the comment is not very
useful.

> +}
> +
> +
>  int
>  qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon,
> qemuMonitorCPUModelInfoPtr model_a,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 6b4b527512..9841ab230c 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1028,6 +1028,10 @@ int qemuMonitorCPUModelInfoInit(const char *name, 
> qemuMonitorCPUModelInfoPtr mod
>  qemuMonitorCPUModelInfoPtr
>  qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig);
>  
> +void qemuMonitorCPUModelInfoRemovePropByBoolValue( 
> qemuMonitorCPUModelInfoPtr model,

s/( /(/

> +   bool value)

And align this accordingly.

> +ATTRIBUTE_NONNULL(1);
> +
>  int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon,
> qemuMonitorCPUModelInfoPtr model_a,
> qemuMonitorCPUModelInfoPtr model_b,

Jirka

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

Re: [libvirt] [PATCH v2 0/2] test: Implement virConnectListAllNodeDevices

[Erik Skultety]

On Tue, Jul 10, 2018 at 05:46:00PM -0400, Cole Robinson wrote:
> v1: https://www.redhat.com/archives/libvir-list/2018-February/msg01135.html
>
> virnodedeviceobj.c generic ListAll infrastructure is not stateless
> and will try to refresh nodedev scsi/pci/etc. config. Understandable
> this doesn't play well with the test driver. Trying to untangle it
> is a bit tough though.
>
> This series adds a way to skip touching the host so we can implement
> ListAllDevices in the test driver, which we want for virt-manager and
> libvirt-dbus test suites
>
> Cole Robinson (2):
>   conf: nodedev: Don't refresh host caps in testdriver
>   test: Implement virConnectListAllNodeDevices
>
>  src/conf/virnodedeviceobj.c | 13 -
>  src/conf/virnodedeviceobj.h |  4 
>  src/test/test_driver.c  | 15 +++
>  3 files changed, 31 insertions(+), 1 deletion(-)

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v2 1/2] conf: nodedev: Don't refresh host caps in testdriver

[Erik Skultety]

On Tue, Jul 10, 2018 at 05:46:01PM -0400, Cole Robinson wrote:
> Add a 'skipUpdateCaps' bool that we set for test_driver.c nodedevs
> which will skip accessing host resources via virNodeDeviceUpdateCaps
>
> Signed-off-by: Cole Robinson 
>

...

> @@ -5565,6 +5566,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver,
>  goto cleanup;
>  def = NULL;
>  objdef = virNodeDeviceObjGetDef(obj);
> +virNodeDeviceObjSetSkipUpdateCaps(obj, true);

I'd probably move this 2 lines up, but I don't really care.

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

[libvirt] [PATCH] virnetdevtap: Don't crash on !ifname in virNetDevTapInterfaceStats

[Michal Privoznik]

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

Some domain  do not have a name (because they are
not TAP devices). Therefore, if
virNetDevTapInterfaceStats(net->ifname, ...) is called an instant
crash occurs. In Linux version of the function strlen() is called
over the name and in BSD version STREQ() is called.

Signed-off-by: Michal Privoznik 
---
 src/util/virnetdevtap.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index bd0710ad2e..2123ffe718 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -691,6 +691,12 @@ virNetDevTapInterfaceStats(const char *ifname,
 FILE *fp;
 char line[256], *colon;
 
+if (!ifname) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Interface not found"));
+return -1;
+}
+
 fp = fopen("/proc/net/dev", "r");
 if (!fp) {
 virReportSystemError(errno, "%s",
@@ -781,7 +787,7 @@ virNetDevTapInterfaceStats(const char *ifname,
 if (ifa->ifa_addr->sa_family != AF_LINK)
 continue;
 
-if (STREQ(ifa->ifa_name, ifname)) {
+if (STREQ_NULLABLE(ifa->ifa_name, ifname)) {
 ifd = (struct if_data *)ifa->ifa_data;
 if (swapped) {
 stats->tx_bytes = ifd->ifi_ibytes;
-- 
2.16.4

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

Re: [libvirt] [jenkins-ci PATCH] guests: List all known guests in inventory

[Andrea Bolognani]

On Fri, 2018-07-13 at 11:14 +0200, Erik Skultety wrote:
> > > > +libvirt-ubuntu-16
> > > > +libvirt-ubuntu-18
> > > 
> > > shouldn't ubuntu 14 be mentioned here too?
> > 
> > We were able to drop Ubuntu 14.04 support, at long last, about a
> > month ago with commit 19a3626d1bab :)
> 
> If only I pulled the master before I start digging /o\. Anyway, thanks for 
> your
> input.

Eheh, don't worry about it. Thank *you* for the review! :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [jenkins-ci PATCH] guests: List all known guests in inventory

[Erik Skultety]

On Fri, Jul 13, 2018 at 11:07:23AM +0200, Andrea Bolognani wrote:
> On Fri, 2018-07-13 at 10:55 +0200, Erik Skultety wrote:
> > On Wed, Jul 11, 2018 at 12:59:13PM +0200, Andrea Bolognani wrote:
> [...]
> > >  libvirt-freebsd-10
> > >  libvirt-freebsd-11
> > > +libvirt-freebsd-current
> >
> > what does ^this map to?
>
> It doesn't map to any other FreeBSD version: -CURRENT it's just
> the name for the in-development version, just like sid for Debian
> and Rawhide for Fedora.
>
> Snapshots are published periodically at
>
>   http://ftp.freebsd.org/pub/FreeBSD/snapshots/VM-IMAGES/12.0-CURRENT/
>
> and can be used just like any other FreeBSD disk image.
>
> > > +libvirt-ubuntu-16
> > > +libvirt-ubuntu-18
> >
> > shouldn't ubuntu 14 be mentioned here too?
>
> We were able to drop Ubuntu 14.04 support, at long last, about a
> month ago with commit 19a3626d1bab :)

If only I pulled the master before I start digging /o\. Anyway, thanks for your
input.

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [jenkins-ci PATCH] guests: List all known guests in inventory

[Andrea Bolognani]

On Fri, 2018-07-13 at 10:55 +0200, Erik Skultety wrote:
> On Wed, Jul 11, 2018 at 12:59:13PM +0200, Andrea Bolognani wrote:
[...]
> >  libvirt-freebsd-10
> >  libvirt-freebsd-11
> > +libvirt-freebsd-current
> 
> what does ^this map to?

It doesn't map to any other FreeBSD version: -CURRENT it's just
the name for the in-development version, just like sid for Debian
and Rawhide for Fedora.

Snapshots are published periodically at

  http://ftp.freebsd.org/pub/FreeBSD/snapshots/VM-IMAGES/12.0-CURRENT/

and can be used just like any other FreeBSD disk image.

> > +libvirt-ubuntu-16
> > +libvirt-ubuntu-18
> 
> shouldn't ubuntu 14 be mentioned here too?

We were able to drop Ubuntu 14.04 support, at long last, about a
month ago with commit 19a3626d1bab :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [jenkins-ci PATCH] guests: Stop using search() as a filter

[Andrea Bolognani]

Recent versions of Ansible complain about this, and suggest
to replace 'result|search' with 'result is search'.

Since f17097c7af59, however, we've been including operating
system information in the inventory: this allows us to drop
our use of search() entirely.

Signed-off-by: Andrea Bolognani 
---
 guests/tasks/bootstrap.yml | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/guests/tasks/bootstrap.yml b/guests/tasks/bootstrap.yml
index 24848c8..d7abb86 100644
--- a/guests/tasks/bootstrap.yml
+++ b/guests/tasks/bootstrap.yml
@@ -2,21 +2,19 @@
 - name: Bootstrap the pkgng package manager
   raw: env ASSUME_ALWAYS_YES=YES pkg bootstrap
   when:
-- inventory_hostname|search('freebsd')
+- package_format == 'pkg'
 
 - name: Bootstrap Ansible
   raw: yum install -y python2
   when:
-- ( inventory_hostname|search('centos') or
-inventory_hostname|search('fedora') )
+- package_format == 'rpm'
 
 - name: Bootstrap Ansible
   raw: apt-get install -y python
   when:
-- ( inventory_hostname|search('debian') or
-inventory_hostname|search('ubuntu') )
+- package_format == 'deb'
 
 - name: Bootstrap Ansible
   raw: pkg install -y python2
   when:
-- inventory_hostname|search('freebsd')
+- package_format == 'pkg'
-- 
2.17.1

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

Re: [libvirt] [jenkins-ci PATCH] guests: List all known guests in inventory

[Erik Skultety]

On Wed, Jul 11, 2018 at 12:59:13PM +0200, Andrea Bolognani wrote:
> Users will probably want to only work with a subset of
> guests but, like any other customization, that should be
> handled with local tweaks.
>
> Signed-off-by: Andrea Bolognani 
> ---
>  guests/inventory | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/guests/inventory b/guests/inventory
> index 6ea43d9..9838d7c 100644
> --- a/guests/inventory
> +++ b/guests/inventory
> @@ -1,8 +1,12 @@
>  libvirt-centos-7
>  libvirt-debian-8
>  libvirt-debian-9
> +libvirt-debian-sid
>  libvirt-fedora-27
>  libvirt-fedora-28
>  libvirt-fedora-rawhide
>  libvirt-freebsd-10
>  libvirt-freebsd-11
> +libvirt-freebsd-current

what does ^this map to?

> +libvirt-ubuntu-16
> +libvirt-ubuntu-18

shouldn't ubuntu 14 be mentioned here too?

Erik

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