[libvirt] [RFC v3 4/8] Resctrl: Add private interface to set cachebanks

[Eli Qiao]

virResCtrlSetCacheBanks: Set cache banks of a libvirt domain. It will
create new resource domain under `/sys/fs/resctrl` and fill the
schemata according the cache banks configration.

virResCtrlUpdate: Update the schemata after libvirt domain destroy.

Signed-off-by: Eli Qiao 
---
 src/libvirt_private.syms |   6 +-
 src/util/virresctrl.c| 699 ++-
 src/util/virresctrl.h|   6 +-
 3 files changed, 705 insertions(+), 6 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cc6c433..85b9e94 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2318,7 +2318,11 @@ virRandomInt;
 virResCtrlAvailable;
 virResCtrlGet;
 virResCtrlInit;
-
+virResCtrlSetCacheBanks;
+virResCtrlTypeFromString;
+virResCtrlTypeToString;
+virResCtrlUpdate;
+#
 
 # util/virrotatingfile.h
 virRotatingFileReaderConsume;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index b49e965..0d400a9 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -72,9 +72,59 @@ do { \
 #define VIR_RESCTRL_SET_SCHEMATA(p, type, pos, val) \
 p->schematas[type]->schemata_items[pos] = val
 
+/**
+ * a virResSchemata represents a schemata object under a resource control
+ * domain.
+ */
+typedef struct _virResSchemataItem virResSchemataItem;
+typedef virResSchemataItem *virResSchemataItemPtr;
+struct _virResSchemataItem {
+unsigned int socket_no;
+unsigned schemata;
+};
+
+typedef struct _virResSchemata virResSchemata;
+typedef virResSchemata *virResSchemataPtr;
+struct _virResSchemata {
+unsigned int n_schemata_items;
+virResSchemataItemPtr schemata_items;
+};
+
+/**
+ * a virResDomain represents a resource control domain. It's a double linked
+ * list.
+ */
+
+typedef struct _virResDomain virResDomain;
+typedef virResDomain *virResDomainPtr;
+
+struct _virResDomain {
+char *name;
+virResSchemataPtr schematas[VIR_RDT_RESOURCE_LAST];
+char **tasks;
+size_t n_tasks;
+size_t n_sockets;
+virResDomainPtr pre;
+virResDomainPtr next;
+};
+
+/* All resource control domains on this host*/
+
+typedef struct _virResCtrlDomain virResCtrlDomain;
+typedef virResCtrlDomain *virResCtrlDomainPtr;
+struct _virResCtrlDomain {
+unsigned int num_domains;
+virResDomainPtr domains;
+};
+
 
 static unsigned int host_id;
 
+/* Global static struct to be maintained which is a interface */
+static virResCtrlDomain domainall;
+
+/* Global static struct array to be maintained which indicate
+ * resource status on a host */
 static virResCtrl resctrlall[] = {
 {
 .name = "L3",
@@ -94,6 +144,66 @@ static virResCtrl resctrlall[] = {
 },
 };
 
+/*
+ * How many bits is set in schemata
+ * eg:
+ * virResCtrlBitsNum(1011) = 2 */
+static int virResCtrlBitsContinuesNum(unsigned schemata)
+{
+size_t i;
+int ret = 0;
+for (i = 0; i < MAX_CBM_BIT_LEN; i ++) {
+if ((schemata & 0x1) == 0x1)
+ret++;
+else
+break;
+schemata = schemata >> 1;
+if (schemata == 0) break;
+}
+return ret;
+}
+
+static int virResCtrlGetStr(const char *domain_name, const char *item_name, 
char **ret)
+{
+char *path;
+int rc = 0;
+
+CONSTRUCT_RESCTRL_PATH(domain_name, item_name);
+
+if (virFileReadAll(path, MAX_FILE_LEN, ret) < 0) {
+rc = -1;
+goto cleanup;
+}
+
+ cleanup:
+VIR_FREE(path);
+return rc;
+}
+
+static int virResCtrlGetTasks(const char *domain_name, char **pids)
+{
+return virResCtrlGetStr(domain_name, "tasks", pids);
+}
+
+static int virResCtrlGetSchemata(const int type, const char *name, char 
**schemata)
+{
+int rc;
+char *tmp, *end;
+char *buf;
+
+if ((rc = virResCtrlGetStr(name, "schemata", )) < 0)
+return rc;
+
+tmp = strstr(buf, resctrlall[type].name);
+end = strchr(tmp, '\n');
+*end = '\0';
+if (VIR_STRDUP(*schemata, tmp) < 0)
+rc = -1;
+
+VIR_FREE(buf);
+return rc;
+}
+
 static int virResCtrlGetInfoStr(const int type, const char *item, char **str)
 {
 int ret = 0;
@@ -114,6 +224,70 @@ static int virResCtrlGetInfoStr(const int type, const char 
*item, char **str)
 return ret;
 }
 
+/* Return pointer of and ncount of schemata*/
+static virResSchemataPtr virParseSchemata(const char *schemata_str, size_t 
*ncount)
+{
+const char *p, *q;
+int pos;
+int ischemata;
+virResSchemataPtr schemata;
+virResSchemataItemPtr schemataitems, tmpitem;
+unsigned int socket_no = 0;
+char *tmp;
+
+if (VIR_ALLOC(schemata) < 0)
+goto cleanup;
+
+p = q = schemata_str;
+pos = strchr(schemata_str, ':') - p;
+
+/* calculate cpu socket count */
+*ncount = 1;
+while ((q = strchr(p, ';')) != 0) {
+p = q + 1;
+(*ncount)++;
+}
+
+/* allocat an arrry to store schemata for each socket*/
+if (VIR_ALLOC_N_QUIET(tmpitem, *ncount) < 0)
+goto cleanup;
+
+

[libvirt] [RFC v3 0/8] Support cache tune in libvirt

[Eli Qiao]

Addressed comment from v2 -> v3:

Daniel:
  * Fixed coding style, passed `make check` and `make syntax-check`

  * Variables renaming and move from header file to c file.

  * For locking/mutex support, no progress.

  There are some discussion from mailing list, but I can not find a better
  way to add locking support without performance impact.

  I'll explain the process and please help to advice what shoud we do.

  VM create:
  1) Get the cache left value on each bank of the host. This should be
 shared amount all VMs.
  2) Calculate the schemata on the bank based on all created resctrl
 domain's schemata
  3) Calculate the default schemata by scaning all domain's schemata.
  4) Flush default schemata to /sys/fs/resctrl/schemata

  VM destroy:
  1) Remove the resctrl domain of that VM
  2) Recalculate default schemata
  3) Flush default schemata to /sys/fs/resctrl/schemata

  The key point is that all VMs shares /sys/fs/resctrl/schemata, and
  when a VM create a resctrl domain, the schemata of that VM depends on
  the default schemata and all other exsited schematas. So a global
  mutex is reqired.

  Before calculate a schemata or update default schemata, libvirt
  should gain this global mutex.

  I will try to think more about how to support this gracefully in next
  patch set.

Marcelo:
  * Added vcpu support for cachetune, this will allow user to define which
vcpu using which cache allocation bank.



vcpus is a cpumap, the vcpu pids will be added to tasks file

  * Added cdp compatible, user can specify l3 cache even host enable cdp.
See patch 8.
On a cdp enabled host, specify l3code/l3data by



This will create a schemata like:
L3data:0=0xff00;...
L3code:0=0xff00;...

  * Would you please help to test if the functions work.

Martin:
  * Xml test case, I have no time to work on this yet, would you please
show me an example, would like to amend it later.

This series patches are for supportting CAT featues, which also
called cache tune in libvirt.

First to expose cache information which could be tuned in capabilites XML.
Then add new domain xml element support to add cacahe bank which will apply
on this libvirt domain.

This series patches add a util file `resctrl.c/h`, an interface to talk with
linux kernel's sys fs.

There are still some TODOs such as expose new public interface to get free
cache information.

Some discussion about this feature support can be found from:
https://www.redhat.com/archives/libvir-list/2017-January/msg00644.html

Eli Qiao (8):
  Resctrl: Add some utils functions
  Resctrl: expose cache information to capabilities
  Resctrl: Add new xml element to support cache tune
  Resctrl: Add private interface to set cachebanks
  Qemu: Set cache banks
  Resctrl: enable l3code/l3data
  Resctrl: Make sure l3data/l3code are pairs
  Resctrl: Compatible mode for cdp enabled

 docs/schemas/domaincommon.rng |   46 ++
 include/libvirt/virterror.h   |1 +
 po/POTFILES.in|1 +
 src/Makefile.am   |1 +
 src/conf/capabilities.c   |   56 +++
 src/conf/capabilities.h   |   23 +
 src/conf/domain_conf.c|  182 +++
 src/conf/domain_conf.h|   19 +
 src/libvirt_private.syms  |   11 +
 src/nodeinfo.c|   64 +++
 src/nodeinfo.h|1 +
 src/qemu/qemu_capabilities.c  |8 +
 src/qemu/qemu_driver.c|6 +
 src/qemu/qemu_process.c   |   53 ++
 src/util/virerror.c   |1 +
 src/util/virresctrl.c | 1091 +
 src/util/virresctrl.h |   85 
 17 files changed, 1649 insertions(+)
 create mode 100644 src/util/virresctrl.c
 create mode 100644 src/util/virresctrl.h

--
1.9.1

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

[libvirt] [RFC v3 1/8] Resctrl: Add some utils functions

[Eli Qiao]

This patch adds some utils struct and functions to expose resctrl
information.

virResCtrlAvailable: If resctrl interface exist on host
virResCtrlGet: get specify type resource contral information
virResCtrlInit: initialize resctrl struct from the host's sys fs.
resctrlall[]: an array to maintain resource control information.

Signed-off-by: Eli Qiao 
---
 include/libvirt/virterror.h |   1 +
 po/POTFILES.in  |   1 +
 src/Makefile.am |   1 +
 src/libvirt_private.syms|   4 +
 src/util/virerror.c |   1 +
 src/util/virresctrl.c   | 343 
 src/util/virresctrl.h   |  80 +++
 7 files changed, 431 insertions(+)
 create mode 100644 src/util/virresctrl.c
 create mode 100644 src/util/virresctrl.h

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 2efee8f..3dd2d08 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -132,6 +132,7 @@ typedef enum {
 
 VIR_FROM_PERF = 65, /* Error from perf */
 VIR_FROM_LIBSSH = 66,   /* Error from libssh connection transport */
+VIR_FROM_RESCTRL = 67,  /* Error from resource control */
 
 # ifdef VIR_ENUM_SENTINELS
 VIR_ERR_DOMAIN_LAST
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 365ea66..f7fda98 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -240,6 +240,7 @@ src/util/virportallocator.c
 src/util/virprocess.c
 src/util/virqemu.c
 src/util/virrandom.c
+src/util/virresctrl.c
 src/util/virrotatingfile.c
 src/util/virscsi.c
 src/util/virscsivhost.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 2f32d41..b626f29 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -162,6 +162,7 @@ UTIL_SOURCES =  
\
util/virprocess.c util/virprocess.h \
util/virqemu.c util/virqemu.h   \
util/virrandom.h util/virrandom.c   \
+   util/virresctrl.h util/virresctrl.c \
util/virrotatingfile.h util/virrotatingfile.c   \
util/virscsi.c util/virscsi.h   \
util/virscsivhost.c util/virscsivhost.h \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8e994c7..743e5ac 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2313,6 +2313,10 @@ virRandomGenerateWWN;
 virRandomInt;
 
 
+# util/virresctrl.h
+virResCtrlAvailable;
+virResCtrlInit;
+
 # util/virrotatingfile.h
 virRotatingFileReaderConsume;
 virRotatingFileReaderFree;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index ef17fb5..93dfd4f 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -139,6 +139,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,
 
   "Perf", /* 65 */
   "Libssh transport layer",
+  "Rescouce Control",
 )
 
 
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
new file mode 100644
index 000..80481bc
--- /dev/null
+++ b/src/util/virresctrl.c
@@ -0,0 +1,343 @@
+/*
+ * virresctrl.c: methods for managing resource contral
+ *
+ * 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
+ * .
+ *
+ * Authors:
+ *  Eli Qiao 
+ */
+#include 
+
+#include 
+#if defined HAVE_SYS_SYSCALL_H
+# include 
+#endif
+#include 
+#include 
+#include 
+
+#include "virresctrl.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virfile.h"
+#include "virhostcpu.h"
+#include "virlog.h"
+#include "virstring.h"
+#include "nodeinfo.h"
+
+VIR_LOG_INIT("util.resctrl");
+
+#define VIR_FROM_THIS VIR_FROM_RESCTRL
+
+#define RESCTRL_DIR "/sys/fs/resctrl"
+#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
+#define SYSFS_SYSTEM_PATH "/sys/devices/system"
+
+#define MAX_CPU_SOCKET_NUM 8
+#define MAX_CBM_BIT_LEN 32
+#define MAX_SCHEMATA_LEN 1024
+#define MAX_FILE_LEN ( 10 * 1024 * 1024)
+
+
+static unsigned int host_id;
+
+static virResCtrl resctrlall[] = {
+{
+.name = "L3",
+.cache_level = "l3",
+},
+{
+.name = "L3DATA",
+.cache_level = "l3",
+},
+{
+.name = "L3CODE",
+.cache_level = "l3",
+},
+{
+.name = "L2",
+.cache_level = "l2",
+},
+};
+
+static int virResCtrlGetInfoStr(const int type, 

[libvirt] [RFC v3 7/8] Resctrl: Make sure l3data/l3code are pairs

[Eli Qiao]

l3data and l3code type of cache banks should be configured pairs.

Signed-off-by: Eli Qiao 
---
 src/conf/domain_conf.c | 19 +++
 src/util/virresctrl.c  |  1 -
 src/util/virresctrl.h  |  2 ++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6ce819d..d790eb8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15619,9 +15619,13 @@ virDomainCacheTuneDefParseXML(virDomainDefPtr def,
 int type = -1;
 virDomainCacheBankPtr bank = NULL;
 virResCtrlPtr resctrl;
+/* A Array to make sure l3code and l3data are pairs*/
+int* sem = NULL;
 
 if (VIR_ALLOC_N(bank, n) < 0)
 goto cleanup;
+if (VIR_ALLOC_N(sem, MAX_CPU_SOCKET_NUM) < 0)
+goto cleanup;
 
 for (i = 0; i < n; i++) {
 if (!(tmp = virXMLPropString(nodes[i], "id"))) {
@@ -15669,6 +15673,12 @@ virDomainCacheTuneDefParseXML(virDomainDefPtr def,
 goto cleanup;
 }
 
+/* VIR_RDT_RESOURCE_L3DATA and VIR_RDT_RESOURCE_L3CODE should be pair 
*/
+if (type == VIR_RDT_RESOURCE_L3DATA)
+sem[bank[i].host_id] ++;
+else if (type == VIR_RDT_RESOURCE_L3CODE)
+sem[bank[i].host_id] --;
+
 resctrl = virResCtrlGet(type);
 
 if (resctrl == NULL || !resctrl->enabled) {
@@ -15717,6 +15727,14 @@ virDomainCacheTuneDefParseXML(virDomainDefPtr def,
 }
 }
 
+for (i = 0; i < MAX_CPU_SOCKET_NUM; i ++) {
+if (sem[i] != 0) {
+virReportError(VIR_ERR_XML_ERROR,
+_("'l3code and l3data shoud be show up pairs on bank 
%zu'"),
+i);
+goto cleanup;
+}
+}
 def->cachetune.cache_banks = bank;
 def->cachetune.n_banks = n;
 return 0;
@@ -15724,6 +15742,7 @@ virDomainCacheTuneDefParseXML(virDomainDefPtr def,
  cleanup:
 VIR_FREE(bank);
 VIR_FREE(tmp);
+VIR_FREE(sem);
 return -1;
 }
 
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index bdf922d..e6f9f30 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -40,7 +40,6 @@
 VIR_LOG_INIT("util.resctrl");
 
 #define VIR_FROM_THIS VIR_FROM_RESCTRL
-#define MAX_CPU_SOCKET_NUM 8
 #define MAX_CBM_BIT_LEN 32
 #define MAX_SCHEMATA_LEN 1024
 #define MAX_FILE_LEN ( 10 * 1024 * 1024)
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index ba5697e..ee7e115 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -28,6 +28,8 @@
 # include "virbitmap.h"
 # include "domain_conf.h"
 
+#define MAX_CPU_SOCKET_NUM 8
+
 enum {
 VIR_RDT_RESOURCE_L3,
 VIR_RDT_RESOURCE_L3DATA,
-- 
1.9.1

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

[libvirt] [RFC v3 3/8] Resctrl: Add new xml element to support cache tune

[Eli Qiao]

This patch adds new xml element to support cache tune as:


  ...
  
  ...


id: any non-minus number
host_id: reference of the host's cache banks id, it's from capabilities
type: cache bank type
size: should be multiples of the min_size of the bank on host.
vcpus: cache allocation on vcpu set, if empty, will apply the allocation
on all vcpus
---
 docs/schemas/domaincommon.rng |  46 +
 src/conf/domain_conf.c| 152 ++
 src/conf/domain_conf.h|  19 ++
 src/util/virresctrl.c |  32 +++--
 src/util/virresctrl.h |   1 +
 5 files changed, 245 insertions(+), 5 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index cc6e0d0..edb2888 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -795,6 +795,32 @@
 
   
 
+
+  
+
+  
+
+
+  
+
+
+  
+
+
+  
+
+
+  
+
+  
+
+
+  
+
+  
+
+  
+
 
   
 
@@ -5451,6 +5477,26 @@
   -1
 
   
+  
+
+  [0-9]+
+
+  
+  
+
+  [0-9]+
+
+  
+  
+
+  (l3)
+
+  
+  
+
+  KiB
+
+  
   
   
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c06b128..6ce819d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -56,6 +56,7 @@
 #include "virstring.h"
 #include "virnetdev.h"
 #include "virhostdev.h"
+#include "virresctrl.h"
 
 #define VIR_FROM_THIS VIR_FROM_DOMAIN
 
@@ -15604,6 +15605,127 @@ virDomainVcpuPinDefParseXML(virDomainDefPtr def,
 return ret;
 }
 
+/* Parse the XML definition for cachetune
+ * and a cachetune has the form
+ * 
+ */
+static int
+virDomainCacheTuneDefParseXML(virDomainDefPtr def,
+  int n,
+  xmlNodePtr* nodes)
+{
+char* tmp = NULL;
+size_t i, j;
+int type = -1;
+virDomainCacheBankPtr bank = NULL;
+virResCtrlPtr resctrl;
+
+if (VIR_ALLOC_N(bank, n) < 0)
+goto cleanup;
+
+for (i = 0; i < n; i++) {
+if (!(tmp = virXMLPropString(nodes[i], "id"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing id in cache 
tune"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].id)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("invalid setting for cache id '%s'"), tmp);
+goto cleanup;
+}
+
+VIR_FREE(tmp);
+if (!(tmp = virXMLPropString(nodes[i], "host_id"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing host id in 
cache tune"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &(bank[i].host_id)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("invalid setting for cache host id '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+if (!(tmp = virXMLPropString(nodes[i], "size"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing size in cache 
tune"));
+goto cleanup;
+}
+if (virStrToLong_ull(tmp, NULL, 10, &(bank[i].size)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("invalid setting for cache size '%s'"), tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+if (!(tmp = virXMLPropString(nodes[i], "type"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("missing cache type"));
+goto cleanup;
+}
+
+if ((type = virResCtrlTypeFromString(tmp)) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+_("'unsupported cache type '%s'"), tmp);
+goto cleanup;
+}
+
+resctrl = virResCtrlGet(type);
+
+if (resctrl == NULL || !resctrl->enabled) {
+virReportError(VIR_ERR_XML_ERROR,
+_("'host doesn't enabled cache type '%s'"), tmp);
+goto cleanup;
+}
+
+bool found_host_id = false;
+/* Loop for banks to search host_id */
+for (j = 0; j < resctrl->num_banks; j++) {
+if (resctrl->cache_banks[j].host_id == bank[i].host_id) {
+found_host_id = true;
+break;
+}
+}
+
+if (! found_host_id) {
+virReportError(VIR_ERR_XML_ERROR,
+_("'cache bank's host id %u not found on the host"),
+bank[i].host_id);
+goto cleanup;
+}
+
+if (bank[i].size == 0 ||
+bank[i].size % resctrl->cache_banks[j].cache_min 

[libvirt] [RFC v3 8/8] Resctrl: Compatible mode for cdp enabled

[Eli Qiao]

This patch support l3 cache allocation compatible mode if cdp enabled
on host.

In this case l3code/l3data has same schemata.

Signed-off-by: Eli Qiao 
---
 src/conf/domain_conf.c | 15 +--
 src/util/virresctrl.c  | 11 +++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d790eb8..107eb87 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15682,9 +15682,20 @@ virDomainCacheTuneDefParseXML(virDomainDefPtr def,
 resctrl = virResCtrlGet(type);
 
 if (resctrl == NULL || !resctrl->enabled) {
-virReportError(VIR_ERR_XML_ERROR,
+/* support cdp compatible */
+if (type == VIR_RDT_RESOURCE_L3) {
+resctrl = virResCtrlGet(type + 1);
+if (resctrl == NULL || !resctrl->enabled) {
+virReportError(VIR_ERR_XML_ERROR,
 _("'host doesn't enabled cache type '%s'"), tmp);
-goto cleanup;
+goto cleanup;
+}
+VIR_WARN("Use cdp compatible mode.");
+} else {
+virReportError(VIR_ERR_XML_ERROR,
+_("'host doesn't enabled cache type '%s'"), tmp);
+goto cleanup;
+}
 }
 
 bool found_host_id = false;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index e6f9f30..1ff249c 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -941,6 +941,7 @@ int virResCtrlSetCacheBanks(virDomainCachetunePtr cachetune,
 char name[VIR_UUID_STRING_BUFLEN];
 virResDomainPtr p;
 int type;
+int pair_type = -1;
 int sid;
 int schemata;
 
@@ -974,6 +975,13 @@ int virResCtrlSetCacheBanks(virDomainCachetunePtr 
cachetune,
  cachetune->cache_banks[i].type);
 continue;
 }
+/* use cdp compatible mode */
+if (!VIR_RESCTRL_ENABLED(type) &&
+(type == VIR_RDT_RESOURCE_L3) &&
+VIR_RESCTRL_ENABLED(VIR_RDT_RESOURCE_L3DATA)) {
+type = VIR_RDT_RESOURCE_L3DATA;
+pair_type = VIR_RDT_RESOURCE_L3CODE;
+}
 
 if ((sid = virResCtrlGetSocketIdByHostID(
 type, cachetune->cache_banks[i].host_id)) < 0) {
@@ -991,6 +999,9 @@ int virResCtrlSetCacheBanks(virDomainCachetunePtr cachetune,
 }
 
 p->schematas[type]->schemata_items[sid].schemata = schemata;
+if(pair_type > 0) {
+p->schematas[pair_type]->schemata_items[sid].schemata = 
schemata;
+}
 }
 
 for (i = 0; i < npid; i++)
-- 
1.9.1

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

[libvirt] [RFC v3 2/8] Resctrl: expose cache information to capabilities

[Eli Qiao]

This patch expose cache information to host's capabilites xml.

For l3 cache allocation

  

  
  

  


For l3 cache allocation supported cdp(seperate data/code):

  


  
  


  


RFC on mailing list.
https://www.redhat.com/archives/libvir-list/2017-January/msg00644.html

Signed-off-by: Eli Qiao 
---
 src/conf/capabilities.c  | 56 ++
 src/conf/capabilities.h  | 23 
 src/libvirt_private.syms |  3 +++
 src/nodeinfo.c   | 64 
 src/nodeinfo.h   |  1 +
 src/qemu/qemu_capabilities.c |  8 ++
 src/qemu/qemu_driver.c   |  4 +++
 7 files changed, 159 insertions(+)

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 9ab343b..23e21e4 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -198,6 +198,18 @@ virCapabilitiesClearSecModel(virCapsHostSecModelPtr 
secmodel)
 }
 
 static void
+virCapabilitiesClearCacheBank(virCapsHostCacheBankPtr cachebank)
+{
+size_t i;
+for (i = 0; i < cachebank->ncontrol; i++)
+VIR_FREE(cachebank->control[i].scope);
+
+VIR_FREE(cachebank->type);
+VIR_FREE(cachebank->cpus);
+}
+
+
+static void
 virCapabilitiesDispose(void *object)
 {
 virCapsPtr caps = object;
@@ -221,6 +233,10 @@ virCapabilitiesDispose(void *object)
 virCapabilitiesClearSecModel(>host.secModels[i]);
 VIR_FREE(caps->host.secModels);
 
+for (i = 0; i < caps->host.ncachebank; i++)
+virCapabilitiesClearCacheBank(caps->host.cachebank[i]);
+VIR_FREE(caps->host.cachebank);
+
 VIR_FREE(caps->host.netprefix);
 VIR_FREE(caps->host.pagesSize);
 virCPUDefFree(caps->host.cpu);
@@ -844,6 +860,41 @@ virCapabilitiesFormatNUMATopology(virBufferPtr buf,
 return 0;
 }
 
+static int
+virCapabilitiesFormatCache(virBufferPtr buf,
+   size_t ncachebank,
+   virCapsHostCacheBankPtr *cachebank)
+{
+size_t i;
+size_t j;
+
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+
+for (i = 0; i < ncachebank; i++) {
+virBufferAsprintf(buf,
+  "\n",
+  cachebank[i]->id,
+  cachebank[i]->type,
+  cachebank[i]->size,
+  cachebank[i]->cpus);
+
+virBufferAdjustIndent(buf, 2);
+for (j = 0; j < cachebank[i]->ncontrol; j++) {
+virBufferAsprintf(buf,
+  "\n",
+  cachebank[i]->control[j].min,
+  cachebank[i]->control[j].reserved,
+  cachebank[i]->control[j].scope);
+}
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+}
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+return 0;
+}
+
 /**
  * virCapabilitiesFormatXML:
  * @caps: capabilities to format
@@ -931,6 +982,11 @@ virCapabilitiesFormatXML(virCapsPtr caps)
 virBufferAddLit(, "\n");
 }
 
+if (caps->host.ncachebank &&
+virCapabilitiesFormatCache(, caps->host.ncachebank,
+   caps->host.cachebank) < 0)
+return NULL;
+
 if (caps->host.netprefix)
 virBufferAsprintf(, "%s\n",
   caps->host.netprefix);
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
index cfdc34a..b446de5 100644
--- a/src/conf/capabilities.h
+++ b/src/conf/capabilities.h
@@ -138,6 +138,25 @@ struct _virCapsHostSecModel {
 virCapsHostSecModelLabelPtr labels;
 };
 
+typedef struct _virCapsHostCacheControl virCapsHostCacheControl;
+typedef virCapsHostCacheControl *virCapsHostCacheControlPtr;
+struct _virCapsHostCacheControl {
+unsigned long long min;
+unsigned long long reserved;
+char* scope;
+};
+
+typedef struct _virCapsHostCacheBank virCapsHostCacheBank;
+typedef virCapsHostCacheBank *virCapsHostCacheBankPtr;
+struct _virCapsHostCacheBank {
+unsigned int id;
+char* type;
+char* cpus;
+unsigned long long size;
+size_t ncontrol;
+virCapsHostCacheControlPtr control;
+};
+
 typedef struct _virCapsHost virCapsHost;
 typedef virCapsHost *virCapsHostPtr;
 struct _virCapsHost {
@@ -160,6 +179,10 @@ struct _virCapsHost {
 size_t nsecModels;
 virCapsHostSecModelPtr secModels;
 
+size_t ncachebank;
+size_t ncachebank_max;
+virCapsHostCacheBankPtr *cachebank;
+
 char *netprefix;
 virCPUDefPtr cpu;
 int nPagesSize; /* size of pagesSize array */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 743e5ac..cc6c433 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1123,6 +1123,7 @@ virLogManagerNew;
 # nodeinfo.h
 nodeCapsInitNUMA;
 nodeGetInfo;
+virCapsInitCache;
 virHostCPUGetCount;
 virHostCPUGetKVMMaxVCPUs;
 

[libvirt] [RFC v3 5/8] Qemu: Set cache banks

[Eli Qiao]

Set cache banks while booting a new domain.

Signed-off-by: Eli Qiao 
---
 src/qemu/qemu_driver.c  |  6 --
 src/qemu/qemu_process.c | 53 +
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7995511..1e3ed1a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -850,8 +850,10 @@ qemuStateInitialize(bool privileged,
 run_gid = cfg->group;
 }
 
-if (virResCtrlAvailable() && virResCtrlInit() < 0)
-VIR_WARN("Faild to initialize resource control.");
+if (virResCtrlAvailable() && virResCtrlInit() < 0) {
+VIR_ERROR(_("Faild to initialize resource control"));
+goto error;
+}
 
 qemu_driver->qemuCapsCache = virQEMUCapsCacheNew(cfg->libDir,
  cfg->cacheDir,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 184440d..baf3ff1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -76,6 +76,7 @@
 #include "configmake.h"
 #include "nwfilter_conf.h"
 #include "netdev_bandwidth_conf.h"
+#include "virresctrl.h"
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
@@ -5021,6 +5022,49 @@ qemuProcessVcpusSortOrder(const void *a,
 return vcpua->order - vcpub->order;
 }
 
+static int
+qemuProcessSetCacheBanks(virDomainObjPtr vm)
+{
+size_t i, j;
+virDomainCachetunePtr cachetune;
+unsigned int max_vcpus = virDomainDefGetVcpusMax(vm->def);
+pid_t *pids = NULL;
+virDomainVcpuDefPtr vcpu;
+size_t npid = 0;
+size_t count = 0;
+int ret = -1;
+
+cachetune = &(vm->def->cachetune);
+
+for (i = 0; i < cachetune->n_banks; i++) {
+if (cachetune->cache_banks[i].vcpus) {
+for (j = 0; j < max_vcpus; j++) {
+if (virBitmapIsBitSet(cachetune->cache_banks[i].vcpus, j)) {
+
+vcpu = virDomainDefGetVcpu(vm->def, j);
+if (!vcpu->online)
+continue;
+
+if (VIR_RESIZE_N(pids, npid, count, 1) < 0)
+goto cleanup;
+pids[count ++] = qemuDomainGetVcpuPid(vm, j);
+}
+}
+}
+}
+
+if (pids == NULL) {
+if (VIR_ALLOC_N(pids, 1) < 0)
+goto cleanup;
+pids[0] = vm->pid;
+count = 1;
+}
+ret = virResCtrlSetCacheBanks(cachetune, vm->def->uuid, pids, count);
+
+ cleanup:
+VIR_FREE(pids);
+return ret;
+}
 
 static int
 qemuProcessSetupHotpluggableVcpus(virQEMUDriverPtr driver,
@@ -5714,6 +5758,11 @@ qemuProcessLaunch(virConnectPtr conn,
 qemuProcessAutoDestroyAdd(driver, vm, conn) < 0)
 goto cleanup;
 
+VIR_DEBUG("Cache allocation");
+
+if (virResCtrlAvailable() && qemuProcessSetCacheBanks(vm) < 0)
+goto cleanup;
+
 ret = 0;
 
  cleanup:
@@ -6216,6 +6265,10 @@ void qemuProcessStop(virQEMUDriverPtr driver,
 virPerfFree(priv->perf);
 priv->perf = NULL;
 
+if (virResCtrlAvailable() && virResCtrlUpdate() < 0)
+VIR_WARN("Failed to update resource control for %s",
+ vm->def->name);
+
 qemuProcessRemoveDomainStatus(driver, vm);
 
 /* Remove VNC and Spice ports from port reservation bitmap, but only if
-- 
1.9.1

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

[libvirt] [RFC v3 6/8] Resctrl: enable l3code/l3data

[Eli Qiao]

Enable l3code/l3data while doing cache tune.

l3code/l3data should use a continus cbm in their seperated schemata and
the cache size are shared between them, so we need to deal them
differently with l3 cache.

This should enable cdp feature while mounting /sys/fs/resctrl, eg:
mount -t resctrl resctrl -o cdp  /sys/fs/resctrl

Signed-off-by: Eli Qiao 
---
 docs/schemas/domaincommon.rng |  2 +-
 src/util/virresctrl.c | 25 -
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index edb2888..f61c57a 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5489,7 +5489,7 @@
   
   
 
-  (l3)
+  (l3|l3code|l3data)
 
   
   
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 0d400a9..bdf922d 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -557,6 +557,7 @@ int virResCtrlRefreshSchemata(void)
 size_t i, j, k;
 unsigned int tmp_schemata;
 unsigned int default_schemata;
+int pair_type = 0;
 
 virResDomainPtr header, p;
 
@@ -567,6 +568,12 @@ int virResCtrlRefreshSchemata(void)
 
 for (i = 0; i < VIR_RDT_RESOURCE_LAST; i++) {
 if (VIR_RESCTRL_ENABLED(i)) {
+
+if (i == VIR_RDT_RESOURCE_L3DATA)
+pair_type = VIR_RDT_RESOURCE_L3CODE;
+if (i == VIR_RDT_RESOURCE_L3CODE)
+pair_type = VIR_RDT_RESOURCE_L3DATA;
+
 for (j = 0; j < header->schematas[i]->n_schemata_items; j ++) {
 p = header->next;
 default_schemata = 
VIR_RESCTRL_GET_SCHEMATA(resctrlall[i].cbm_len);
@@ -574,6 +581,8 @@ int virResCtrlRefreshSchemata(void)
 /* NOTEs: if only header domain, the schemata will be set to 
default one*/
 for (k = 1; k < domainall.num_domains; k++) {
 tmp_schemata |= 
p->schematas[i]->schemata_items[j].schemata;
+if (pair_type > 0)
+tmp_schemata |= 
p->schematas[pair_type]->schemata_items[j].schemata;
 p = p->next;
 }
 /* sys fs doens't let us use 0 */
@@ -685,6 +694,7 @@ virResCtrlWrite(const char *name, const char *item, const 
char *content)
 goto cleanup;
 
 rc = 0;
+
  cleanup:
 VIR_FREE(path);
 VIR_FORCE_CLOSE(writefd);
@@ -888,6 +898,7 @@ virResCtrlCalculateSchemata(int type,
 virResDomainPtr p;
 unsigned int tmp_schemata;
 unsigned int schemata_sum = 0;
+int pair_type = 0;
 
 if (resctrlall[type].cache_banks[sid].cache_left < size) {
 VIR_ERROR(_("Not enough cache left on bank %u"), hostid);
@@ -902,8 +913,18 @@ virResCtrlCalculateSchemata(int type,
 
 p = domainall.domains;
 p = p->next;
+
+/* for type is l3code and l3data, we need to deal them specially*/
+if (type == VIR_RDT_RESOURCE_L3DATA)
+pair_type = VIR_RDT_RESOURCE_L3CODE;
+
+if (type == VIR_RDT_RESOURCE_L3CODE)
+pair_type = VIR_RDT_RESOURCE_L3DATA;
+
 for (i = 1; i < domainall.num_domains; i ++) {
 schemata_sum |= p->schematas[type]->schemata_items[sid].schemata;
+if (pair_type > 0)
+schemata_sum |= 
p->schematas[pair_type]->schemata_items[sid].schemata;
 p = p->next;
 }
 
@@ -944,6 +965,9 @@ int virResCtrlSetCacheBanks(virDomainCachetunePtr cachetune,
 }
 
 if (p != NULL) {
+
+virResCtrlAppendDomain(p);
+
 for (i = 0; i < cachetune->n_banks; i++) {
 if ((type = virResCtrlTypeFromString(
 cachetune->cache_banks[i].type)) < 0) {
@@ -978,7 +1002,6 @@ int virResCtrlSetCacheBanks(virDomainCachetunePtr 
cachetune,
 virResCtrlDestroyDomain(p);
 return -1;
 }
-virResCtrlAppendDomain(p);
 } else {
 VIR_ERROR(_("Failed to create a domain in sysfs"));
 return -1;
-- 
1.9.1

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

Re: [libvirt] [PATCH V2 1/2] libxl: set default disk format in device post-parse

[Michal Privoznik]

On 02/07/2017 08:46 PM, Jim Fehlig wrote:
> When starting a domian, a libxl_domain_config object is created from
> virDomainDef. Any virDomainDiskDef devices with a format of
> VIR_STORAGE_FILE_NONE are mapped to LIBXL_DISK_FORMAT_RAW in the
> corresponding libxl_disk_device, but the virDomainDiskDef format is
> never updated to reflect the change.
> 
> A better place to set a default format for disk devices is the
> device post-parse callback, ensuring the virDomainDiskDef object
> reflects the default format.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_conf.c   | 10 ++
>  src/libxl/libxl_domain.c |  7 ++-
>  2 files changed, 8 insertions(+), 9 deletions(-)

ACK.

BTW looks like you forgot about one other location:

src/libxl/libxl_driver.c=5452=libxlDomainBlockStatsGatherSingle(virDomainObjPtr 
vm,
--
src/libxl/libxl_driver.c-5469-
src/libxl/libxl_driver.c-5470-if (STREQ(disk_drv, "phy")) {
src/libxl/libxl_driver.c-5471-if (disk_fmt != VIR_STORAGE_FILE_RAW &&
src/libxl/libxl_driver.c:5472:disk_fmt != VIR_STORAGE_FILE_NONE) {
src/libxl/libxl_driver.c-5473-
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
src/libxl/libxl_driver.c-5474-   _("unsupported format 
%s"),
src/libxl/libxl_driver.c-5475-   
virStorageFileFormatTypeToString(disk_fmt));


Michal

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

Re: [libvirt] [PATCH V2 2/2] libxl: fix disk detach when not specified

[Michal Privoznik]

On 02/07/2017 08:46 PM, Jim Fehlig wrote:
> When a user does not explicitly set a  in the disk config,
> libvirt defers selection of a default to libxl. This approach works
> fine when starting a domain with such configuration or attaching a
> disk to a running domain. But when detaching such a disk, libxl
> will fail with "unrecognized disk backend type: 0". libxl makes no
> attempt to recalculate a default backend (driver) on detach and
> simply fails when uninitialized.
> 
> This patch updates the libvirt disk config with the backend selected
> by libxl when starting a domain or attaching a disk to a running
> domain. Another benefit of this approach is that the live XML is
> also updated with the backend driver selected by libxl.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_conf.c   | 32 
>  src/libxl/libxl_conf.h   |  4 
>  src/libxl/libxl_domain.c | 25 +
>  src/libxl/libxl_driver.c |  1 +
>  4 files changed, 62 insertions(+)

ACK

Michal

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

[libvirt] [PATCH 3/3] qemu: Add configuration variable to control nodedev enumeration

[John Ferlan]

Add an 'enumerate_nodedev' qemu configuration variable to control
whether the node device enumeration and add/remove event notification
mechanism is enabled. This ensures only those environments that desire
to utilize a domain vHBA need to have the (slight) overhead of adding
the device hash table to qemu and managing add/remove events for
specific scsi_host and scsi_target events.

Signed-off-by: John Ferlan 
---
 src/qemu/libvirtd_qemu.aug |  1 +
 src/qemu/qemu.conf | 19 +++
 src/qemu/qemu_conf.c   |  5 +
 src/qemu/qemu_conf.h   |  3 +++
 src/qemu/qemu_driver.c | 17 ++---
 src/qemu/test_libvirtd_qemu.aug.in |  1 +
 6 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index de723b2..d011f2c 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -92,6 +92,7 @@ module Libvirtd_qemu =
let device_entry = bool_entry "mac_filter"
  | bool_entry "relaxed_acs_check"
  | bool_entry "allow_disk_format_probing"
+ | bool_entry "nodedev_enumeration"
  | str_entry "lock_manager"
 
let rpc_entry = int_entry "max_queued"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index a8cd369..c726bc9 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -531,6 +531,25 @@
 #allow_disk_format_probing = 1
 
 
+# In order for a properly configured qemu domain to be able to passthrough
+# NPIV vHBA LUN's to the guest qemu will need to enumerate all the node
+# device's and then handle the add and remove events. In order to enable
+# qemu to handle this, set this parameter to 1 and restart libvirtd. This
+# enables communication between qemu and udev node device event management
+# to provide the functionality. When a domain is started or reconnected
+# with a vHBA controller configured in the domain XML or when a vHBA
+# controller is hot-plugged, the qemu driver will handle the "scsi_hostN"
+# device creation and removal events by hot-(un)plugging "scsi_targetB_T_L"
+# (Bus, Target, Lun) Direct-Access LUNs to/from the guest.
+#
+# This is not required for qemu domain environments that utilize the Storage
+# Pool NPIV vHBA's to define LUNs in the domain XML.
+#
+# Defaults to 0.
+#
+#nodedev_enumeration = 1
+
+
 # In order to prevent accidentally starting two domains that
 # share one writable disk, libvirt offers two approaches for
 # locking files. The first one is sanlock, the other one,
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index f88c7f3..cfe0f84 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -347,6 +347,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
privileged)
 goto error;
 #endif
 
+cfg->nodeDeviceEnumeration = false;
+
 return cfg;
 
  error:
@@ -707,6 +709,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 goto cleanup;
 if (virConfGetValueBool(conf, "allow_disk_format_probing", 
>allowDiskFormatProbing) < 0)
 goto cleanup;
+if (virConfGetValueBool(conf, "nodedev_enumeration",
+>nodeDeviceEnumeration) < 0)
+goto cleanup;
 if (virConfGetValueBool(conf, "set_process_name", >setProcessName) < 
0)
 goto cleanup;
 if (virConfGetValueUInt(conf, "max_processes", >maxProcesses) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 7d514a9..9e5d9ad 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -196,6 +196,9 @@ struct _virQEMUDriverConfig {
 virFirmwarePtr *firmwares;
 size_t nfirmwares;
 unsigned int glusterDebugLevel;
+
+/* Node device enumeration enabled */
+bool nodeDeviceEnumeration;
 };
 
 /* Main driver state */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 758a7f5..023bb30 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -798,14 +798,17 @@ qemuStateInitialize(bool privileged,
 if (!(qemu_driver->sharedDevices = virHashCreate(30, 
qemuSharedDeviceEntryFree)))
 goto error;
 
-/* Create a hash table to keep track of node device's by name */
-if (!(qemu_driver->nodeDevices = virHashCreate(100, NULL)))
-goto error;
+/* If node device enumeration is enabled, create a hash table to
+ * keep track of node device's by name and set up a callback mechanism
+ * with the node device conf code to get called whenever a node device
+ * is added or removed. */
+if (cfg->nodeDeviceEnumeration) {
+if (!(qemu_driver->nodeDevices = virHashCreate(100, NULL)))
+goto error;
 
-/* Set up a callback mechanism with the node device conf code to get
- * called whenever a node device is added or removed. */
-if (virNodeDeviceRegisterCallbackDriver() < 0)
-goto error;
+if (virNodeDeviceRegisterCallbackDriver() < 
0)
+goto error;
+

[libvirt] [PATCH 2/3] qemu: Use nodedev callback mechanism to to get a nodedev data

[John Ferlan]

Using the nodedev driver callback mechanism - allow qemu to be
notified whenever a nodedev is added/removed.

Keep track of the node devices in a hash table rather than requiring
a connection in order to get specific information about a node device
that qemu would eventually like to keep track of.

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_conf.c   | 48 
 src/qemu/qemu_conf.h   | 14 ++
 src/qemu/qemu_driver.c | 31 +++
 3 files changed, 93 insertions(+)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 6613d59..f88c7f3 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1598,3 +1598,51 @@ qemuGetDomainHupageMemPath(const virDomainDef *def,
 
 return 0;
 }
+
+
+/**
+ * @driver: qemu driver pointer
+ * @def: Node device definition
+ * @enumeration: boolean for emumeration indication
+ *
+ * Taking the @def from the node device driver, add the device to
+ * the qemu node device hash table. The @enumeration is true when
+ * this callback is called from the node device enumeration and false
+ * when called when the @def was added to the node device.
+ *
+ * Returns -1 on failure, 0 on adding device name, 1 on already added name
+ */
+int
+qemuNodeDeviceEntryAdd(virQEMUDriverPtr driver,
+   virNodeDeviceDefPtr def,
+   bool enumerate)
+{
+VIR_DEBUG("Attempt to add name='%s', parent='%s' enumerate=%d",
+  def->name, def->parent, enumerate);
+
+if (!virHashLookup(driver->nodeDevices, def->name)) {
+if (virHashAddEntry(driver->nodeDevices, def->name, NULL) < 0)
+return -1;
+return 0;
+}
+return 1;
+}
+
+
+/**
+ * @driver: qemu driver pointer
+ * @def: node device definition
+ *
+ * Remove the definition from qemu's node device hash table.
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+qemuNodeDeviceEntryRemove(virQEMUDriverPtr driver,
+  virNodeDeviceDefPtr def)
+{
+VIR_DEBUG("Attempt to remove name='%s' parent='%s'",
+  def->name, def->parent);
+
+return virHashRemoveEntry(driver->nodeDevices, def->name);
+}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 92a7a50..7d514a9 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -32,6 +32,7 @@
 # include "network_conf.h"
 # include "domain_conf.h"
 # include "snapshot_conf.h"
+# include "node_device_conf.h"
 # include "domain_event.h"
 # include "virthread.h"
 # include "security/security_manager.h"
@@ -253,6 +254,9 @@ struct _virQEMUDriver {
 virHashTablePtr sharedDevices;
 
 /* Immutable pointer, self-locking APIs */
+virHashTablePtr nodeDevices;
+
+/* Immutable pointer, self-locking APIs */
 virPortAllocatorPtr remotePorts;
 
 /* Immutable pointer, self-locking APIs */
@@ -348,4 +352,14 @@ int qemuGetDomainHupageMemPath(const virDomainDef *def,
virQEMUDriverConfigPtr cfg,
unsigned long long pagesize,
char **memPath);
+
+void qemuNodeDeviceEntryFree(void *payload, const void *name);
+
+int qemuNodeDeviceEntryAdd(virQEMUDriverPtr driver,
+   virNodeDeviceDefPtr def,
+   bool enumerate);
+
+int qemuNodeDeviceEntryRemove(virQEMUDriverPtr driver,
+  virNodeDeviceDefPtr def);
+
 #endif /* __QEMUD_CONF_H */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 89bc833..758a7f5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -197,6 +197,28 @@ static virNWFilterCallbackDriver qemuCallbackDriver = {
 };
 
 
+static int
+qemuNodeDeviceAdd(virNodeDeviceDefPtr def,
+  bool enumerate)
+{
+return qemuNodeDeviceEntryAdd(qemu_driver, def, enumerate);
+}
+
+
+static int
+qemuNodeDeviceRemove(virNodeDeviceDefPtr def)
+{
+return qemuNodeDeviceEntryRemove(qemu_driver, def);
+}
+
+
+static virNodeDeviceCallbackDriver qemuNodedevCallbackDriver = {
+.name = QEMU_DRIVER_NAME,
+.nodeDeviceAdd = qemuNodeDeviceAdd,
+.nodeDeviceRemove = qemuNodeDeviceRemove,
+};
+
+
 struct qemuAutostartData {
 virQEMUDriverPtr driver;
 virConnectPtr conn;
@@ -776,6 +798,15 @@ qemuStateInitialize(bool privileged,
 if (!(qemu_driver->sharedDevices = virHashCreate(30, 
qemuSharedDeviceEntryFree)))
 goto error;
 
+/* Create a hash table to keep track of node device's by name */
+if (!(qemu_driver->nodeDevices = virHashCreate(100, NULL)))
+goto error;
+
+/* Set up a callback mechanism with the node device conf code to get
+ * called whenever a node device is added or removed. */
+if (virNodeDeviceRegisterCallbackDriver() < 0)
+goto error;
+
 if (qemuMigrationErrorInit(qemu_driver) < 0)
 goto error;
 
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com

[libvirt] [PATCH 1/3] nodedev: Add driver callback mechanism to add/remove devices

[John Ferlan]

Add a callback mechanism as a side door of sorts to the event
mgmt functions that are triggered when a node_device object is
added or removed. This includes a mechanism to enumerate already
added devices for those stateInitialize functions called after
the initial nodedevRegister is called.

Signed-off-by: John Ferlan 
---
 src/check-aclrules.pl  |   3 +-
 src/check-driverimpls.pl   |   1 +
 src/conf/node_device_conf.c| 125 -
 src/conf/node_device_conf.h|  18 ++
 src/libvirt_private.syms   |   3 +
 src/node_device/node_device_udev.c |  29 +
 6 files changed, 177 insertions(+), 2 deletions(-)

diff --git a/src/check-aclrules.pl b/src/check-aclrules.pl
index 8739cda..161d954 100755
--- a/src/check-aclrules.pl
+++ b/src/check-aclrules.pl
@@ -243,7 +243,8 @@ while (<>) {
 } elsif (/^(?:static\s+)?(vir(?:\w+)?Driver)\s+/) {
 if ($1 ne "virNWFilterCallbackDriver" &&
 $1 ne "virNWFilterTechDriver" &&
-$1 ne "virDomainConfNWFilterDriver") {
+$1 ne "virDomainConfNWFilterDriver" &&
+$1 ne "virNodeDeviceCallbackDriver") {
 $intable = 1;
 $table = $1;
 }
diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl
index e320558..b707fc8 100755
--- a/src/check-driverimpls.pl
+++ b/src/check-driverimpls.pl
@@ -70,6 +70,7 @@ while (<>) {
 } elsif (/^(?:static\s+)?(vir(?:\w+)?Driver)\s+/) {
 next if $1 eq "virNWFilterCallbackDriver" ||
 $1 eq "virNWFilterTechDriver" ||
+$1 eq "virNodeDeviceCallbackDriver" ||
 $1 eq "virConnectDriver";
 $intable = 1;
 $table = $1;
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 4d3268f..7a4df44 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -33,6 +33,7 @@
 #include "virstring.h"
 #include "node_device_conf.h"
 #include "device_conf.h"
+#include "virlog.h"
 #include "virxml.h"
 #include "virbuffer.h"
 #include "viruuid.h"
@@ -40,6 +41,8 @@
 
 #define VIR_FROM_THIS VIR_FROM_NODEDEV
 
+VIR_LOG_INIT("conf.node_device_conf");
+
 VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST,
   "system",
   "pci",
@@ -263,6 +266,104 @@ void virNodeDeviceDefFree(virNodeDeviceDefPtr def)
 VIR_FREE(def);
 }
 
+
+/* Provide callback infrastructure that is expected to be called when
+ * devices are added/removed from a node_device subsystem that supports
+ * the callback mechanism. This provides a way for drivers to register
+ * to be notified when a node_device is added/removed. */
+virNodedevEnumerateAddDevices virNodedevEnumerateAddDevicesCb = NULL;
+int nodeDeviceCallbackDriver;
+#define MAX_CALLBACK_DRIVER 10
+static virNodeDeviceCallbackDriverPtr nodeDeviceDrvArray[MAX_CALLBACK_DRIVER];
+
+
+/**
+ * @cb: Driver function to be called.
+ *
+ * Set a callback at driver initialization to provide a callback to a
+ * driver specific function that can handle the enumeration of the existing
+ * devices and the addition of those devices for the registering driver.
+ *
+ * The initial node_device enumeration is done prior to other drivers, thus
+ * this provides a mechanism to load the existing data since the functions
+ * virNodeDeviceAssignDef and virNodeDeviceObjRemove would typically
+ * only be called when a new device is added/removed after the initial
+ * enumeration. The registering driver will need to handle duplication
+ * of data.
+ */
+void
+virNodeDeviceConfEnumerateInit(virNodedevEnumerateAddDevices cb)
+{
+virNodedevEnumerateAddDevicesCb = cb;
+}
+
+
+/**
+ * @cbd: Driver callback pointers to add/remove devices
+ *
+ * Register a callback function in the registering driver to be called
+ * when devices are added or removed. Additionally, ensure the initial
+ * enumeration of the devices is completed taking care to do it after
+ * setting the callbacks
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+virNodeDeviceRegisterCallbackDriver(virNodeDeviceCallbackDriverPtr cbd)
+{
+if (nodeDeviceCallbackDriver >= MAX_CALLBACK_DRIVER) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("no space available for another callback driver"));
+return -1;
+}
+
+if (!cbd->nodeDeviceAdd || !cbd->nodeDeviceRemove) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("callback requires both add and remove functions"));
+return -1;
+}
+
+nodeDeviceDrvArray[nodeDeviceCallbackDriver++] = cbd;
+
+/* Setting the add/remove callback first ensures that there is no
+ * window of opportunity for a device to be added after enumeration
+ * is complete, but before the callback is in place. So, set the
+ * callback first, then do the enumeration. */
+if (virNodedevEnumerateAddDevicesCb) {
+if 

[libvirt] [PATCH 0/3] Add callback mech for qemu and nodedev

[John Ferlan]

Add a callback mechanism for the nodedev driver (mimics the nwfilter)
to allow the (c)ability to directly enumerate node devices and add/remove
devices based on udev events.

This is a set up of sorts for the ability to add a vHBA definition
to a domain, create the vHBA, and allow the LUN's associated with the
vHBA to be automagially added/removed to/from the domain. The vHBA
code exists partially in some local branches I have, but I figure I
need to get this part of the logic accepted. See bz:

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

for some "thoughts" on where this is headed.

An alternative solution would require the qemu driver to handle or
recognize nodedev connection events, but would require a few more
calls in order to get the data needed for vHBA handling.

The logic ends up being "hidden" behind a new qemu configuration
variable "enumerate_nodedev" (I'm fine using a different name). The
default is for the code to be disabled.

The end result may be usable by the (on list) vGPU mdev code as well
as a way to find/recogize an mdev create and the various "children"
of the mdev that would be created (and of course deletion). But that's
just me considering other uses.

John Ferlan (3):
  nodedev: Add driver callback mechanism to add/remove devices
  qemu: Use nodedev callback mechanism to to get a nodedev data
  qemu: Add configuration variable to control nodedev enumeration

 src/check-aclrules.pl  |   3 +-
 src/check-driverimpls.pl   |   1 +
 src/conf/node_device_conf.c| 125 -
 src/conf/node_device_conf.h|  18 ++
 src/libvirt_private.syms   |   3 +
 src/node_device/node_device_udev.c |  29 +
 src/qemu/libvirtd_qemu.aug |   1 +
 src/qemu/qemu.conf |  19 ++
 src/qemu/qemu_conf.c   |  53 
 src/qemu/qemu_conf.h   |  17 +
 src/qemu/qemu_driver.c |  34 ++
 src/qemu/test_libvirtd_qemu.aug.in |   1 +
 12 files changed, 302 insertions(+), 2 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH] conf: fix use-after-free when sending event message

[Wang King]

If there is a process with a client which registers event callbacks,
and it calls libvirt's API which uses the same virConnectPtr in that
callback function. When this process exit abnormally lead to client
disconnect, there is a possibility that the main thread is refer to
virServerClient just after the virServerClient been freed by job
thread of libvirtd.

Following is the backtrace:
 #0  0x7fda223d66d8 in virClassIsDerivedFrom 
(klass=0xdeadbeef,parent=0x7fda24c81b40)
 #1  0x7fda223d6a1e in virObjectIsClass 
(anyobj=anyobj@entry=0x7fd9e575b400,klass=)
 #2  0x7fda223d6a44 in virObjectLock (anyobj=anyobj@entry=0x7fd9e575b400)
 #3  0x7fda22507f71 in virNetServerClientSendMessage 
(client=client@entry=0x7fd9e575b400, msg=msg@entry=0x7fd9ec30de90)
 #4  0x7fda230d714d in remoteDispatchObjectEventSend 
(client=0x7fd9e575b400, program=0x7fda24c844e0, procnr=procnr@entry=348, 
proc=0x7fda2310e5e0 , 
data=data@entry=0x7ffc3857fdb0)
 #5  0x7fda230dd71b in remoteRelayDomainEventTunable (conn=, 
dom=0x7fda27cd7660, params=0x7fda27f3aae0, nparams=1, opaque=0x7fd9e6c99e00)
 #6  0x7fda224484cb in virDomainEventDispatchDefaultFunc 
(conn=0x7fda27cd0120, event=0x7fda2736ea00, cb=0x7fda230dd610 
, cbopaque=0x7fd9e6c99e00)
 #7  0x7fda22446871 in virObjectEventStateDispatchCallbacks 
(callbacks=, callbacks=, event=0x7fda2736ea00, 
state=0x7fda24ca3960)
 #8  virObjectEventStateQueueDispatch (callbacks=0x7fda24c65800, 
queue=0x7ffc3857fe90, state=0x7fda24ca3960)
 #9  virObjectEventStateFlush (state=0x7fda24ca3960)
 #10 virObjectEventTimer (timer=, opaque=0x7fda24ca3960)
 #11 0x7fda223ae8b9 in virEventPollDispatchTimeouts ()
 #12 virEventPollRunOnce ()
 #13 0x7fda223ad1d2 in virEventRunDefaultImpl ()
 #14 0x7fda225046cd in virNetDaemonRun (dmn=dmn@entry=0x7fda24c775c0)
 #15 0x7fda230d6351 in main (argc=, argv=)

(gdb) p *(virNetServerClientPtr)0x7fd9e575b400
$2 = {parent = {parent = {u = {dummy_align1 = 140573849338048, dummy_align2 = 
0x7fd9e65ac0c0, s = {magic = 3864707264, refs = 32729}}, klass = 
0x7fda0078}, lock = {lock = {__data = {__lock = 0,
  __count = 0, __owner = 0, __nusers = 0, __kind = 0, __spins = 0, 
__list = {__prev = 0x0, __next = 0x0}}, __size = '\000' , 
__align = 0}}}, wantClose = false,
  delayedClose = false, sock = 0x0, auth = 0, readonly = false, tlsCtxt = 0x0, 
tls = 0x0, sasl = 0x0, sockTimer = 0, identity = 0x0, nrequests = 0, 
nrequests_max = 0, rx = 0x0, tx = 0x0, filters = 0x0,
  nextFilterID = 0, dispatchFunc = 0x0, dispatchOpaque = 0x0, privateData = 
0x0, privateDataFreeFunc = 0x0, privateDataPreExecRestart = 0x0, 
privateDataCloseFunc = 0x0, keepalive = 0x0}
---
 src/rpc/virnetserverclient.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 81da82c..562516f 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1021,6 +1021,12 @@ void virNetServerClientClose(virNetServerClientPtr 
client)
 client->sock = NULL;
 }
 
+if (client->privateData &&
+client->privateDataFreeFunc) {
+client->privateDataFreeFunc(client->privateData);
+client->privateData = NULL;
+}
+
 virObjectUnlock(client);
 }
 
-- 
2.8.3


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

Re: [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting

[Jim Fehlig]

Joao Martins wrote:
> On 02/08/2017 04:06 PM, Jim Fehlig wrote:
>> Joao Martins wrote:
>>> On 02/02/2017 10:31 PM, Jim Fehlig wrote:
 When the libxl driver is initialized, it creates a virDomainDef
 object for dom0 and adds it to the list of domains. Total memory
 for dom0 was being set from the max_memkb field of libxl_dominfo
 struct retrieved from libxl, but this field can be set to
 LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
 explicitly set by the user.

 This patch adds some simple parsing of the Xen commandline,
 looking for a dom0_mem parameter that also specifies a 'max' value.
 If not specified, dom0 maximum memory is effectively all physical
 host memory.

 Signed-off-by: Jim Fehlig 
 ---
  src/libxl/libxl_conf.c   | 75 
 
  src/libxl/libxl_conf.h   |  3 ++
  src/libxl/libxl_driver.c |  2 +-
  3 files changed, 79 insertions(+), 1 deletion(-)

 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
 index b5186f2..bfe0e92 100644
 --- a/src/libxl/libxl_conf.c
 +++ b/src/libxl/libxl_conf.c
 @@ -34,6 +34,7 @@
  #include "internal.h"
  #include "virlog.h"
  #include "virerror.h"
 +#include "c-ctype.h"
  #include "datatypes.h"
  #include "virconf.h"
  #include "virfile.h"
 @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr 
 cfg,
  
  }
  
 +/*
 + * dom0's maximum memory can be controled by the user with the 'dom0_mem' 
 Xen
 + * command line parameter. E.g. to set dom0's initial memory to 4G and max
 + * memory to 8G: dom0_mem=4G,max:8G
 + *
 + * If not constrained by the user, dom0 can effectively use all host 
 memory.
 + * This function returns the configured maximum memory for dom0 in 
 kilobytes,
 + * either the user-specified value or total physical memory as a default.
 + */
 +unsigned long long
 +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
 +{
 +char **cmd_tokens = NULL;
 +char **mem_tokens = NULL;
 +size_t i;
 +size_t j;
 +unsigned long long ret;
 +libxl_physinfo physinfo;
 +
 +if (cfg->verInfo->commandline == NULL ||
 +!(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
 +goto physmem;
 +
 +for (i = 0; cmd_tokens[i] != NULL; i++) {
 +if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
 +continue;
 +
 +if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
 +break;
 +for (j = 0; mem_tokens[j] != NULL; j++) {
 +if (STRPREFIX(mem_tokens[j], "max:")) {
 +char *p = mem_tokens[j] + 4;
 +unsigned long long multiplier = 1;
 +
 +while (c_isdigit(*p))
 +p++;
 +if (virStrToLong_ull(mem_tokens[j] + 4, , 10, ) < 0)
 +break;
 +if (*p) {
 +switch (*p) {
 +case 'k':
 +case 'K':
 +multiplier = 1024;
 +break;
 +case 'm':
 +case 'M':
 +multiplier = 1024 * 1024;
 +break;
 +case 'g':
 +case 'G':
 +multiplier = 1024 * 1024 * 1024;
 +break;
 +}
 +}
 +ret = (ret * multiplier) / 1024;
 +goto cleanup;
 +}
 +}
 +}
 +
 + physmem:
 +/* No 'max' specified in dom0_mem, so dom0 can use all physical 
 memory */
 +libxl_physinfo_init();
 +libxl_get_physinfo(cfg->ctx, );
>>> Despite being an unlikely event, libxl_get_physinfo can fail here - I think 
>>> you
>>> need to check the return value here.
>> Bummer. I was hoping this function could always return a sane value for dom0 
>> max
>> memory. But I'm not really sure what that would be if libxl_get_physinfo 
>> failed.
>>
> Yeah, We probably have some serious concerns if this fails. And given that
> libxlAddDom0 happens early while bootstrapping the libxl module, it might be
> good to warn the user (following similar pattern throughout the rest of the 
> code):
> 
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>_("libxl_get_physinfo_info failed"));
> 
> or perhaps simply VIR_WARN?
> 
 +ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
 +libxl_physinfo_dispose();
 +
 + cleanup:
 +virStringListFree(cmd_tokens);
 +virStringListFree(mem_tokens);
 +

[libvirt] [PATCH V2 2/2] libxl: fix dom0 maximum memory setting

[Jim Fehlig]

When the libxl driver is initialized, it creates a virDomainDef
object for dom0 and adds it to the list of domains. Total memory
for dom0 was being set from the max_memkb field of libxl_dominfo
struct retrieved from libxl, but this field can be set to
LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
explicitly set by the user.

This patch adds some simple parsing of the Xen commandline,
looking for a dom0_mem parameter that also specifies a 'max' value.
If not specified, dom0 maximum memory is effectively all physical
host memory.

Signed-off-by: Jim Fehlig 
---

V2: Check return value of libxl_get_physinfo and
libxlDriverGetDom0MaxmemConf.

 src/libxl/libxl_conf.c   | 77 
 src/libxl/libxl_conf.h   |  4 +++
 src/libxl/libxl_driver.c |  5 +++-
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 28b05e1..7e30c7d 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -34,6 +34,7 @@
 #include "internal.h"
 #include "virlog.h"
 #include "virerror.h"
+#include "c-ctype.h"
 #include "datatypes.h"
 #include "virconf.h"
 #include "virfile.h"
@@ -1533,6 +1534,82 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
 
 }
 
+/*
+ * dom0's maximum memory can be controled by the user with the 'dom0_mem' Xen
+ * command line parameter. E.g. to set dom0's initial memory to 4G and max
+ * memory to 8G: dom0_mem=4G,max:8G
+ *
+ * If not constrained by the user, dom0 can effectively use all host memory.
+ * This function returns the configured maximum memory for dom0 in kilobytes,
+ * either the user-specified value or total physical memory as a default.
+ */
+int
+libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg,
+ unsigned long long *maxmem)
+{
+char **cmd_tokens = NULL;
+char **mem_tokens = NULL;
+size_t i;
+size_t j;
+libxl_physinfo physinfo;
+int ret = -1;
+
+if (cfg->verInfo->commandline == NULL ||
+!(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
+goto physmem;
+
+for (i = 0; cmd_tokens[i] != NULL; i++) {
+if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
+continue;
+
+if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
+break;
+for (j = 0; mem_tokens[j] != NULL; j++) {
+if (STRPREFIX(mem_tokens[j], "max:")) {
+char *p = mem_tokens[j] + 4;
+unsigned long long multiplier = 1;
+
+while (c_isdigit(*p))
+p++;
+if (virStrToLong_ull(mem_tokens[j] + 4, , 10, maxmem) < 0)
+break;
+if (*p) {
+switch (*p) {
+case 'm':
+case 'M':
+multiplier = 1024;
+break;
+case 'g':
+case 'G':
+multiplier = 1024 * 1024;
+break;
+}
+}
+*maxmem = *maxmem * multiplier;
+ret = 0;
+goto cleanup;
+}
+}
+}
+
+ physmem:
+/* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
+libxl_physinfo_init();
+if (libxl_get_physinfo(cfg->ctx, )) {
+VIR_WARN("libxl_get_physinfo failed");
+goto cleanup;
+}
+*maxmem = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
+libxl_physinfo_dispose();
+ret = 0;
+
+ cleanup:
+virStringListFree(cmd_tokens);
+virStringListFree(mem_tokens);
+return ret;
+}
+
+
 #ifdef LIBXL_HAVE_DEVICE_CHANNEL
 static int
 libxlPrepareChannel(virDomainChrDefPtr channel,
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 69d7885..4942171 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -174,6 +174,10 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
   const char *filename);
 
 int
+libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg,
+ unsigned long long *maxmem);
+
+int
 libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
 int
 libxlMakeNic(virDomainDefPtr def,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 040b986..1558d5b 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -576,6 +576,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
 virDomainObjPtr vm = NULL;
 virDomainDefPtr oldDef = NULL;
 libxl_dominfo d_info;
+unsigned long long maxmem;
 int ret = -1;
 
 libxl_dominfo_init(_info);
@@ -615,7 +616,9 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
 if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
 goto cleanup;
 vm->def->mem.cur_balloon = d_info.current_memkb;
-virDomainDefSetMemoryTotal(vm->def, d_info.max_memkb);

[libvirt] [PATCH V2 1/2] libxl: fix reporting of maximum memory

[Jim Fehlig]

The libxl driver reports different values of maximum memory depending
on state of a domain. If inactive, maximum memory value is reported
correctly. When active, maximum memory is derived from max_pages value
returned by the XEN_SYSCTL_getdomaininfolist sysctl operation. But
max_pages can be changed by toolstacks and does not necessarily
represent the maximum memory a domain can use during its active
lifetime.

A better location for determining a domain's maximum memory is the
/local/domain//memory/static-max node in xenstore. This value
is set from the libxl_domain_build_info.max_memkb field when creating
the domain. Currently it cannot be changed nor can its value be
exceeded by a balloon operation. From libvirt's perspective, always
reporting maximum memory with virDomainDefGetMemoryTotal() will produce
the same results as reading the static-max node in xenstore.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_driver.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 38ad91e..040b986 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1640,10 +1640,10 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr 
info)
 if (virDomainGetInfoEnsureACL(dom->conn, vm->def) < 0)
 goto cleanup;
 
+info->maxMem = virDomainDefGetMemoryTotal(vm->def);
 if (!virDomainObjIsActive(vm)) {
 info->cpuTime = 0;
 info->memory = vm->def->mem.cur_balloon;
-info->maxMem = virDomainDefGetMemoryTotal(vm->def);
 } else {
 libxl_dominfo_init(_info);
 
@@ -1655,7 +1655,6 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr 
info)
 }
 info->cpuTime = d_info.cpu_time;
 info->memory = d_info.current_memkb;
-info->maxMem = d_info.max_memkb;
 
 libxl_dominfo_dispose(_info);
 }
@@ -5174,7 +5173,7 @@ libxlDomainMemoryStats(virDomainPtr dom,
 goto endjob;
 }
 mem = d_info.current_memkb;
-maxmem = d_info.max_memkb;
+maxmem = virDomainDefGetMemoryTotal(vm->def);
 
 LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, mem);
 LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
-- 
2.9.2

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

[libvirt] [PATCH V2 0/2] libxl: a few domain maximum memory fixes

[Jim Fehlig]

Patch 1 fixes reporting of domain maximum memory for running domains.
When creating a virDomainDef object to represent dom0, max memory was
not set correctly, which is fixed by patch2.

V2:
Check return value of libxl_get_physinfo and libxlDriverGetDom0MaxmemConf
in patch2.

Jim Fehlig (2):
  libxl: fix reporting of maximum memory
  libxl: fix dom0 maximum memory setting

 src/libxl/libxl_conf.c   | 77 
 src/libxl/libxl_conf.h   |  4 +++
 src/libxl/libxl_driver.c | 10 ---
 3 files changed, 87 insertions(+), 4 deletions(-)

-- 
2.9.2

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

[libvirt] [PATCH] nodedev: Return the parent for a virNodeDevicePtr struct

[John Ferlan]

When the 'parent' was added to the virNodeDevicePtr structure
by commit id 'e8a4ea75a' the 'parent' field was not properly filled
in when a virGetNodeDevice call was made within driver/config code.
Only the device name was ever filled in. Fetching the parent required
a second trip via virNodeDeviceGetParent into the node device lookup
code was required in order to retrieve the specific parent field (and
still the parent field was never filled in although it was free'd).

Since we have the data when we initially call virGetNodeDevice from
within driver/node_config code - let's just fill in the parent field
as well for anyone that wants it without requiring another trip into
the node_device lookup just to get the parent.

This will allow API's such as virConnectListAllNodeDevices,
virNodeDeviceLookupByName, and virNodeDeviceLookupSCSIHostByWWN
to retrieve both name and parent in the returned virNodeDevicePtr.

Signed-off-by: John Ferlan 
---

 Found this while working on some vHBA in the domain code when I was
 thinking about using a virConnectListAllNodeDevices... The returned
 structures didn't have the ->parent filled in... 

 src/conf/node_device_conf.c  |  4 ++--
 src/node_device/node_device_driver.c | 10 --
 src/test/test_driver.c   |  6 +-
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 4d3268f..f6d7692 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2158,8 +2158,8 @@ virNodeDeviceObjListExport(virConnectPtr conn,
 if ((!filter || filter(conn, devobj->def)) &&
 virNodeDeviceMatch(devobj, flags)) {
 if (devices) {
-if (!(device = virGetNodeDevice(conn,
-devobj->def->name))) {
+if (!(device = virGetNodeDevice(conn, devobj->def->name)) ||
+VIR_STRDUP(device->parent, devobj->def->parent) < 0) {
 virNodeDeviceObjUnlock(devobj);
 goto cleanup;
 }
diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 5238e23..4900e32 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -261,7 +261,10 @@ nodeDeviceLookupByName(virConnectPtr conn, const char 
*name)
 if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0)
 goto cleanup;
 
-ret = virGetNodeDevice(conn, name);
+if ((ret = virGetNodeDevice(conn, name))) {
+if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
+virObjectUnref(ret);
+}
 
  cleanup:
 if (obj)
@@ -302,7 +305,10 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, 
obj->def) < 0)
 goto out;
 
-dev = virGetNodeDevice(conn, obj->def->name);
+if ((dev = virGetNodeDevice(conn, obj->def->name))) {
+if (VIR_STRDUP(dev->parent, obj->def->parent) < 0)
+virObjectUnref(dev);
+}
 virNodeDeviceObjUnlock(obj);
 goto out;
 }
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 6820298..8dd738b 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5450,7 +5450,10 @@ testNodeDeviceLookupByName(virConnectPtr conn, const 
char *name)
 goto cleanup;
 }
 
-ret = virGetNodeDevice(conn, name);
+if ((ret = virGetNodeDevice(conn, name))) {
+if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
+virObjectUnref(ret);
+}
 
  cleanup:
 if (obj)
@@ -5648,6 +5651,7 @@ testNodeDeviceCreateXML(virConnectPtr conn,
0);
 
 dev = virGetNodeDevice(conn, def->name);
+ignore_value(VIR_STRDUP(def->parent, def->parent));
 def = NULL;
  cleanup:
 testDriverUnlock(driver);
-- 
2.7.4

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

Re: [libvirt] [PATCH 2/2] libxl: use init and dispose functions with libxl_physinfo

[Jim Fehlig]

Joao Martins wrote:
> On 02/08/2017 04:17 PM, Jim Fehlig wrote:
>> Joao Martins wrote:
>>> On 02/02/2017 10:39 PM, Jim Fehlig wrote:
 The typical pattern when calling libxl functions that populate a
 structure is

   libxl_foo foo;
   libxl_foo_init();
   libxl_get_foo(ctx, );
   ...
   libxl_foo_dispose();

 Fix several instances of libxl_physinfo missing the init and
 dispose calls.
>>> Indeed,
>>>
 Signed-off-by: Jim Fehlig 
>>> Reviewed-by: Joao Martins 
>>>
>>> See also one comment/nit below, perhaps one libxl_physinfo_init could be 
>>> moved
>>> slightly up..
>>>
 [...]
 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index 3a69720..8951bef 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -4286,6 +4286,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn)
  if (virNodeGetFreeMemoryEnsureACL(conn) < 0)
  goto cleanup;
  
 +libxl_physinfo_init(_info);
>>> .. namely here? That is before virNodeGetFreeMemoryEnsureACL.
>> Nice catch. Moved as suggested in my local branch.
>>
>> Any other comments on this small series? Would be nice to get these bug fixes
>> committed :-).
>>
> Nope, looks all good to me:
> 
> Acked-by: Joao Martins 

Thanks, I've pushed these fixes now.

Regards,
Jim

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

Re: [libvirt] [PATCH] vz: cleanup: remove unused constant

[Andrea Bolognani]

On Wed, 2017-02-08 at 18:26 +0300, Maxim Nestratov wrote:
> PARALLELS_STATISTICS_DROP_COUNT isn't used anymore
> 
> Signed-off-by: Maxim Nestratov 

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting

[Joao Martins]

On 02/08/2017 04:06 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>> On 02/02/2017 10:31 PM, Jim Fehlig wrote:
>>> When the libxl driver is initialized, it creates a virDomainDef
>>> object for dom0 and adds it to the list of domains. Total memory
>>> for dom0 was being set from the max_memkb field of libxl_dominfo
>>> struct retrieved from libxl, but this field can be set to
>>> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
>>> explicitly set by the user.
>>>
>>> This patch adds some simple parsing of the Xen commandline,
>>> looking for a dom0_mem parameter that also specifies a 'max' value.
>>> If not specified, dom0 maximum memory is effectively all physical
>>> host memory.
>>>
>>> Signed-off-by: Jim Fehlig 
>>> ---
>>>  src/libxl/libxl_conf.c   | 75 
>>> 
>>>  src/libxl/libxl_conf.h   |  3 ++
>>>  src/libxl/libxl_driver.c |  2 +-
>>>  3 files changed, 79 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index b5186f2..bfe0e92 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -34,6 +34,7 @@
>>>  #include "internal.h"
>>>  #include "virlog.h"
>>>  #include "virerror.h"
>>> +#include "c-ctype.h"
>>>  #include "datatypes.h"
>>>  #include "virconf.h"
>>>  #include "virfile.h"
>>> @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr 
>>> cfg,
>>>  
>>>  }
>>>  
>>> +/*
>>> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' 
>>> Xen
>>> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
>>> + * memory to 8G: dom0_mem=4G,max:8G
>>> + *
>>> + * If not constrained by the user, dom0 can effectively use all host 
>>> memory.
>>> + * This function returns the configured maximum memory for dom0 in 
>>> kilobytes,
>>> + * either the user-specified value or total physical memory as a default.
>>> + */
>>> +unsigned long long
>>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
>>> +{
>>> +char **cmd_tokens = NULL;
>>> +char **mem_tokens = NULL;
>>> +size_t i;
>>> +size_t j;
>>> +unsigned long long ret;
>>> +libxl_physinfo physinfo;
>>> +
>>> +if (cfg->verInfo->commandline == NULL ||
>>> +!(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
>>> +goto physmem;
>>> +
>>> +for (i = 0; cmd_tokens[i] != NULL; i++) {
>>> +if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
>>> +continue;
>>> +
>>> +if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
>>> +break;
>>> +for (j = 0; mem_tokens[j] != NULL; j++) {
>>> +if (STRPREFIX(mem_tokens[j], "max:")) {
>>> +char *p = mem_tokens[j] + 4;
>>> +unsigned long long multiplier = 1;
>>> +
>>> +while (c_isdigit(*p))
>>> +p++;
>>> +if (virStrToLong_ull(mem_tokens[j] + 4, , 10, ) < 0)
>>> +break;
>>> +if (*p) {
>>> +switch (*p) {
>>> +case 'k':
>>> +case 'K':
>>> +multiplier = 1024;
>>> +break;
>>> +case 'm':
>>> +case 'M':
>>> +multiplier = 1024 * 1024;
>>> +break;
>>> +case 'g':
>>> +case 'G':
>>> +multiplier = 1024 * 1024 * 1024;
>>> +break;
>>> +}
>>> +}
>>> +ret = (ret * multiplier) / 1024;
>>> +goto cleanup;
>>> +}
>>> +}
>>> +}
>>> +
>>> + physmem:
>>> +/* No 'max' specified in dom0_mem, so dom0 can use all physical memory 
>>> */
>>> +libxl_physinfo_init();
>>> +libxl_get_physinfo(cfg->ctx, );
>> Despite being an unlikely event, libxl_get_physinfo can fail here - I think 
>> you
>> need to check the return value here.
> 
> Bummer. I was hoping this function could always return a sane value for dom0 
> max
> memory. But I'm not really sure what that would be if libxl_get_physinfo 
> failed.
> 
Yeah, We probably have some serious concerns if this fails. And given that
libxlAddDom0 happens early while bootstrapping the libxl module, it might be
good to warn the user (following similar pattern throughout the rest of the 
code):

virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
   _("libxl_get_physinfo_info failed"));

or perhaps simply VIR_WARN?

>>> +ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
>>> +libxl_physinfo_dispose();
>>> +
>>> + cleanup:
>>> +virStringListFree(cmd_tokens);
>>> +virStringListFree(mem_tokens);
>>> +return ret;
>>> +}
>>> +
>>> +
>>>  #ifdef LIBXL_HAVE_DEVICE_CHANNEL
>>>  static int
>>>  libxlPrepareChannel(virDomainChrDefPtr channel,
>>> diff --git 

Re: [libvirt] [PATCH 2/2] libxl: use init and dispose functions with libxl_physinfo

[Joao Martins]

On 02/08/2017 04:17 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>> On 02/02/2017 10:39 PM, Jim Fehlig wrote:
>>> The typical pattern when calling libxl functions that populate a
>>> structure is
>>>
>>>   libxl_foo foo;
>>>   libxl_foo_init();
>>>   libxl_get_foo(ctx, );
>>>   ...
>>>   libxl_foo_dispose();
>>>
>>> Fix several instances of libxl_physinfo missing the init and
>>> dispose calls.
>> Indeed,
>>
>>> Signed-off-by: Jim Fehlig 
>>
>> Reviewed-by: Joao Martins 
>>
>> See also one comment/nit below, perhaps one libxl_physinfo_init could be 
>> moved
>> slightly up..
>>
>>> [...]
>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index 3a69720..8951bef 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -4286,6 +4286,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn)
>>>  if (virNodeGetFreeMemoryEnsureACL(conn) < 0)
>>>  goto cleanup;
>>>  
>>> +libxl_physinfo_init(_info);
>>
>> .. namely here? That is before virNodeGetFreeMemoryEnsureACL.
> 
> Nice catch. Moved as suggested in my local branch.
> 
> Any other comments on this small series? Would be nice to get these bug fixes
> committed :-).
> 
Nope, looks all good to me:

Acked-by: Joao Martins 

> Regards,
> Jim
> 
>>
>> Not that it matters much, as init/dispose in this case just zeroes out 
>> phy_info
>> region. But just perhaps consistency as you would end disposing an non
>> initialized object.
>>
>>>  if (libxl_get_physinfo(cfg->ctx, _info)) {
>>>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> _("libxl_get_physinfo_info failed"));
>>> @@ -4295,6 +4296,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn)
>>>  ret = phy_info.free_pages * cfg->verInfo->pagesize;
>>>  
>>>   cleanup:
>>> +libxl_physinfo_dispose(_info);
>>>  virObjectUnref(cfg);
>>>  return ret;
>>>  }
>>>
> 

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

[libvirt] [PATCH RFC 1/4] qemu_agent: move agent into util

[Joao Martins]

As it could be shared with libxl which now allows channels to
be created. Also changed filename to match others in the same
directory namely to virqemuagent.{h,c}

Signed-off-by: Joao Martins 
---
 po/POTFILES.in   |2 +-
 src/Makefile.am  |2 +-
 src/libvirt_private.syms |   21 +
 src/qemu/qemu_agent.c| 2248 --
 src/qemu/qemu_agent.h|  123 ---
 src/qemu/qemu_domain.h   |2 +-
 src/qemu/qemu_driver.c   |2 +-
 src/util/virqemuagent.c  | 2248 ++
 src/util/virqemuagent.h  |  123 +++
 tests/qemuagenttest.c|2 +-
 tests/qemumonitortestutils.c |2 +-
 tests/qemumonitortestutils.h |2 +-
 12 files changed, 2399 insertions(+), 2378 deletions(-)
 delete mode 100644 src/qemu/qemu_agent.c
 delete mode 100644 src/qemu/qemu_agent.h
 create mode 100644 src/util/virqemuagent.c
 create mode 100644 src/util/virqemuagent.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 365ea66..ebb247b 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -122,7 +122,6 @@ src/openvz/openvz_conf.c
 src/openvz/openvz_driver.c
 src/openvz/openvz_util.c
 src/phyp/phyp_driver.c
-src/qemu/qemu_agent.c
 src/qemu/qemu_alias.c
 src/qemu/qemu_capabilities.c
 src/qemu/qemu_cgroup.c
@@ -239,6 +238,7 @@ src/util/virpolkit.c
 src/util/virportallocator.c
 src/util/virprocess.c
 src/util/virqemu.c
+src/util/virqemuagent.c
 src/util/virrandom.c
 src/util/virrotatingfile.c
 src/util/virscsi.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 2f32d41..62c8733 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -161,6 +161,7 @@ UTIL_SOURCES =  
\
util/virprobe.h \
util/virprocess.c util/virprocess.h \
util/virqemu.c util/virqemu.h   \
+   util/virqemuagent.c util/virqemuagent.h \
util/virrandom.h util/virrandom.c   \
util/virrotatingfile.h util/virrotatingfile.c   \
util/virscsi.c util/virscsi.h   \
@@ -815,7 +816,6 @@ VBOX_DRIVER_EXTRA_DIST =
\
vbox/vbox_XPCOMCGlue.c vbox/vbox_XPCOMCGlue.h
 
 QEMU_DRIVER_SOURCES =  \
-   qemu/qemu_agent.c qemu/qemu_agent.h \
qemu/qemu_alias.c qemu/qemu_alias.h \
qemu/qemu_blockjob.c qemu/qemu_blockjob.h   \
qemu/qemu_capabilities.c qemu/qemu_capabilities.h   \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d556c7d..a5a1313 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2306,6 +2306,27 @@ virQEMUBuildLuksOpts;
 virQEMUBuildObjectCommandlineFromJSON;
 
 
+# util/virqemuagent.h
+qemuAgentArbitraryCommand;
+qemuAgentClose;
+qemuAgentFSFreeze;
+qemuAgentFSThaw;
+qemuAgentFSTrim;
+qemuAgentGetFSInfo;
+qemuAgentGetInterfaces;
+qemuAgentGetTime;
+qemuAgentGetVCPUs;
+qemuAgentNotifyClose;
+qemuAgentNotifyEvent;
+qemuAgentOpen;
+qemuAgentSetVCPUs;
+qemuAgentSetUserPassword;
+qemuAgentSetTime;
+qemuAgentShutdown;
+qemuAgentSuspend;
+qemuAgentUpdateCPUInfo;
+
+
 # util/virrandom.h
 virRandom;
 virRandomBits;
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
deleted file mode 100644
index 46cad53..000
--- a/src/qemu/qemu_agent.c
+++ /dev/null
@@ -1,2248 +0,0 @@
-/*
- * qemu_agent.c: interaction with QEMU guest agent
- *
- * Copyright (C) 2006-2014 Red Hat, Inc.
- * Copyright (C) 2006 Daniel P. Berrange
- *
- * 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: Daniel P. Berrange 
- */
-
-#include 
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#include "qemu_agent.h"
-#include "viralloc.h"
-#include "virlog.h"
-#include "virerror.h"
-#include "virjson.h"
-#include "virfile.h"
-#include "virprocess.h"
-#include "virtime.h"
-#include "virobject.h"
-#include "virstring.h"
-#include "base64.h"
-
-#define VIR_FROM_THIS VIR_FROM_QEMU
-
-VIR_LOG_INIT("qemu.qemu_agent");
-
-#define LINE_ENDING "\n"
-
-#define DEBUG_IO 0

[libvirt] [PATCH RFC 2/4] qemu_agent: ignore requests echoed back by guest

[Joao Martins]

Signed-off-by: Joao Martins 
---
 src/util/virqemuagent.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/util/virqemuagent.c b/src/util/virqemuagent.c
index caabae0..ffb3489 100644
--- a/src/util/virqemuagent.c
+++ b/src/util/virqemuagent.c
@@ -333,7 +333,8 @@ qemuAgentIOProcessLine(qemuAgentPtr mon,
 goto cleanup;
 }
 
-if (virJSONValueObjectHasKey(obj, "QMP") == 1) {
+if (virJSONValueObjectHasKey(obj, "QMP") == 1 ||
+virJSONValueObjectHasKey(obj, "execute") == 1) {
 ret = 0;
 } else if (virJSONValueObjectHasKey(obj, "event") == 1) {
 ret = qemuAgentIOProcessEvent(mon, obj);
-- 
2.1.4

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

[libvirt] [PATCH RFC 0/4] libxl: qemu agent support

[Joao Martins]

Hey!

Back when channels were introduced in libxl (in answer to Michal[0]) I
suggested the idea of integrating qemu guest agent (which currently lives
qemu driver).

This series is an attempt at pulling qemu agent from qemu driver into util in
using it in libxl in subsequent patches. What do folks think of the idea? Note
that this is still all very RFC because 1) there's a lot of code we could
potentially share between qemu and libxl with respect to finding guest agent
config and keeping some of its state (see patch 3); 2) also we need to ignore
"execute" messages to be able to query the agent and see it's returned data
(patch 2). Which despite the commit not being incorrect I am not sure yet why
we need it yet.

As PoC I only implemented domainQemuAgentCommand/domainInterfaceAddresses, but
there's a lot more driver APIs we can potentially introduce after this. Tracing
all driver APIs that might require a guest agent:

* domainFSThaw, domainFSFreeze, domainFSTrim, domainGetFSInfo
* domainSetUserPassword
* domainGetTime, domainSetTime
* domainShutdown (with VIR_DOMAIN_SHUTDOWN_GUEST_AGENT)
* domainReboot (with VIR_DOMAIN_REBOOT_GUEST_AGENT)
* domainGetGuestVcpus
* domainSetGuestVcpus
* domainSetVcpusFlags (with VIR_DOMAIN_VCPU_GUEST)
* domainGetVcpusFlags (with VIR_DOMAIN_VCPU_GUEST)

Comments/Feedback is appreciated :)

Cheers,
Joao

[0] https://www.spinics.net/linux/fedora/libvir/msg136685.html

Joao Martins (4):
  qemu_agent: move agent into util
  qemu_agent: ignore requests echoed back by guest
  libxl: implement qemu-agent-command
  libxl: domainInterfaceAddresses agent support

 po/POTFILES.in   |2 +-
 src/Makefile.am  |2 +-
 src/libvirt_private.syms |   21 +
 src/libxl/libxl_domain.c |  239 -
 src/libxl/libxl_domain.h |   16 +
 src/libxl/libxl_driver.c |   69 ++
 src/qemu/qemu_agent.c| 2248 -
 src/qemu/qemu_agent.h|  123 ---
 src/qemu/qemu_domain.h   |2 +-
 src/qemu/qemu_driver.c   |2 +-
 src/util/virqemuagent.c  | 2249 ++
 src/util/virqemuagent.h  |  123 +++
 tests/qemuagenttest.c|2 +-
 tests/qemumonitortestutils.c |2 +-
 tests/qemumonitortestutils.h |2 +-
 15 files changed, 2723 insertions(+), 2379 deletions(-)
 delete mode 100644 src/qemu/qemu_agent.c
 delete mode 100644 src/qemu/qemu_agent.h
 create mode 100644 src/util/virqemuagent.c
 create mode 100644 src/util/virqemuagent.h

-- 
2.1.4

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

[libvirt] [PATCH RFC 4/4] libxl: domainInterfaceAddresses agent support

[Joao Martins]

Allow querying of guest interface address through agent
through command `virsh domifaddr test --source agent`

Signed-off-by: Joao Martins 
---
 src/libxl/libxl_driver.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index cf5e702..8f8fbec 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -6261,6 +6261,8 @@ libxlDomainInterfaceAddresses(virDomainPtr dom,
   unsigned int source,
   unsigned int flags)
 {
+libxlDriverPrivatePtr driver = dom->conn->privateData;
+libxlDomainObjPrivatePtr priv;
 virDomainObjPtr vm = NULL;
 int ret = -1;
 
@@ -6282,6 +6284,22 @@ libxlDomainInterfaceAddresses(virDomainPtr dom,
 case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE:
 ret = libxlGetDHCPInterfaces(dom, vm, ifaces);
 break;
+case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT:
+priv = vm->privateData;
+if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
+goto cleanup;
+
+if (!libxlDomainAgentAvailable(vm, true))
+goto endjob;
+
+libxlDomainObjEnterAgent(vm);
+ret = qemuAgentGetInterfaces(priv->agent, ifaces);
+libxlDomainObjExitAgent(vm);
+
+endjob:
+libxlDomainObjEndJob(driver, vm);
+
+break;
 
 default:
 virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
-- 
2.1.4

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

[libvirt] [PATCH RFC 3/4] libxl: implement qemu-agent-command

[Joao Martins]

Signed-off-by: Joao Martins 
---
 src/libxl/libxl_domain.c | 239 ++-
 src/libxl/libxl_domain.h |  16 
 src/libxl/libxl_driver.c |  51 ++
 3 files changed, 305 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index ed73cd2..6bdd0ec 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -782,6 +782,12 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
 }
 }
 
+if (priv->agent) {
+qemuAgentClose(priv->agent);
+priv->agent = NULL;
+priv->agentError = false;
+}
+
 if ((vm->def->nnets)) {
 size_t i;
 
@@ -940,6 +946,228 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config 
*d_config)
 return -1;
 }
 
+/*
+ * This is a callback registered with a qemuAgentPtr instance,
+ * and to be invoked when the agent console hits an end of file
+ * condition, or error, thus indicating VM shutdown should be
+ * performed
+ */
+static void
+libxlHandleAgentEOF(qemuAgentPtr agent,
+  virDomainObjPtr vm)
+{
+libxlDomainObjPrivatePtr priv;
+
+VIR_DEBUG("Received EOF from agent on %p '%s'", vm, vm->def->name);
+
+virObjectLock(vm);
+
+priv = vm->privateData;
+
+if (!priv->agent) {
+VIR_DEBUG("Agent freed already");
+goto unlock;
+}
+
+qemuAgentClose(agent);
+priv->agent = NULL;
+
+ unlock:
+virObjectUnlock(vm);
+return;
+}
+
+
+/*
+ * This is invoked when there is some kind of error
+ * parsing data to/from the agent. The VM can continue
+ * to run, but no further agent commands will be
+ * allowed
+ */
+static void
+libxlHandleAgentError(qemuAgentPtr agent ATTRIBUTE_UNUSED,
+virDomainObjPtr vm)
+{
+libxlDomainObjPrivatePtr priv;
+
+VIR_DEBUG("Received error from agent on %p '%s'", vm, vm->def->name);
+
+virObjectLock(vm);
+
+priv = vm->privateData;
+
+priv->agentError = true;
+
+virObjectUnlock(vm);
+}
+
+static void libxlHandleAgentDestroy(qemuAgentPtr agent,
+  virDomainObjPtr vm)
+{
+VIR_DEBUG("Received destroy agent=%p vm=%p", agent, vm);
+
+virObjectUnref(vm);
+}
+
+static qemuAgentCallbacks agentCallbacks = {
+.destroy = libxlHandleAgentDestroy,
+.eofNotify = libxlHandleAgentEOF,
+.errorNotify = libxlHandleAgentError,
+};
+
+static virDomainChrDefPtr
+libxlFindAgentConfig(virDomainDefPtr def)
+{
+size_t i;
+
+for (i = 0; i < def->nchannels; i++) {
+virDomainChrDefPtr channel = def->channels[i];
+
+if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN)
+continue;
+
+if (STREQ_NULLABLE(channel->target.name, "org.qemu.guest_agent.0"))
+return channel;
+}
+
+return NULL;
+}
+
+bool
+libxlDomainAgentAvailable(virDomainObjPtr vm, bool reportError)
+{
+libxlDomainObjPrivatePtr priv = vm->privateData;
+
+if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
+if (reportError) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("domain is not running"));
+}
+return false;
+}
+if (priv->agentError) {
+if (reportError) {
+virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
+   _("QEMU guest agent is not "
+ "available due to an error"));
+}
+return false;
+}
+if (!priv->agent) {
+if (libxlFindAgentConfig(vm->def)) {
+if (reportError) {
+virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
+   _("QEMU guest agent is not connected"));
+}
+return false;
+} else {
+if (reportError) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+   _("QEMU guest agent is not configured"));
+}
+return false;
+}
+}
+return true;
+}
+
+static int
+libxlConnectAgent(virDomainObjPtr vm)
+{
+virDomainChrDefPtr config = libxlFindAgentConfig(vm->def);
+libxlDomainObjPrivatePtr priv = vm->privateData;
+qemuAgentPtr agent = NULL;
+int ret = -1;
+
+if (!config)
+return 0;
+
+if (priv->agent)
+return 0;
+
+/* Hold an extra reference because we can't allow 'vm' to be
+ * deleted while the agent is active */
+virObjectRef(vm);
+
+ignore_value(virTimeMillisNow(>agentStart));
+virObjectUnlock(vm);
+
+agent = qemuAgentOpen(vm, config->source, );
+
+virObjectLock(vm);
+priv->agentStart = 0;
+
+if (agent == NULL)
+virObjectUnref(vm);
+
+if (!virDomainObjIsActive(vm)) {
+qemuAgentClose(agent);
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("guest crashed while connecting to the guest agent"));
+ret = 

[libvirt] [PATCH 05/11] daemon: Refactor connection driver module loading

[Peter Krempa]

Pass the registration function name to virDriverLoadModule so that we
can later call specific functions if necessary (e.g. for testing
purposes). This gets rid of the rather ugly automatic name generator and
unifies the code to load/initialize the modules.

It's also clear which registration function gets called.
---
 daemon/libvirtd.c   | 136 
 src/driver.c|  37 +++-
 src/driver.h|   3 +-
 tests/virdrivermoduletest.c |  23 ++--
 4 files changed, 77 insertions(+), 122 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index b6d76ed84..2f9f5c77d 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -341,6 +341,14 @@ static int daemonErrorLogFilter(virErrorPtr err, int 
priority)
 return priority;
 }

+
+#ifdef WITH_DRIVER_MODULES
+# define VIR_DAEMON_LOAD_MODULE(func, module) \
+virDriverLoadModule(module, #func)
+#else
+# define VIR_DAEMON_LOAD_MODULE(func, module) \
+func()
+#endif
 static void daemonInitialize(void)
 {
 /*
@@ -350,99 +358,55 @@ static void daemonInitialize(void)
  * driver, since their resources must be auto-started before any
  * domains can be auto-started.
  */
-#ifdef WITH_DRIVER_MODULES
 /* We don't care if any of these fail, because the whole point
  * is to allow users to only install modules they want to use.
  * If they try to open a connection for a module that
  * is not loaded they'll get a suitable error at that point
  */
-# ifdef WITH_NETWORK
-virDriverLoadModule("network");
-# endif
-# ifdef WITH_INTERFACE
-virDriverLoadModule("interface");
-# endif
-# ifdef WITH_STORAGE
-virDriverLoadModule("storage");
-# endif
-# ifdef WITH_NODE_DEVICES
-virDriverLoadModule("nodedev");
-# endif
-# ifdef WITH_SECRETS
-virDriverLoadModule("secret");
-# endif
-# ifdef WITH_NWFILTER
-virDriverLoadModule("nwfilter");
-# endif
-# ifdef WITH_XEN
-virDriverLoadModule("xen");
-# endif
-# ifdef WITH_LIBXL
-virDriverLoadModule("libxl");
-# endif
-# ifdef WITH_QEMU
-virDriverLoadModule("qemu");
-# endif
-# ifdef WITH_LXC
-virDriverLoadModule("lxc");
-# endif
-# ifdef WITH_UML
-virDriverLoadModule("uml");
-# endif
-# ifdef WITH_VBOX
-virDriverLoadModule("vbox");
-# endif
-# ifdef WITH_BHYVE
-virDriverLoadModule("bhyve");
-# endif
-# ifdef WITH_VZ
-virDriverLoadModule("vz");
-# endif
-#else
-# ifdef WITH_NETWORK
-networkRegister();
-# endif
-# ifdef WITH_INTERFACE
-interfaceRegister();
-# endif
-# ifdef WITH_STORAGE
-storageRegister();
-# endif
-# ifdef WITH_NODE_DEVICES
-nodedevRegister();
-# endif
-# ifdef WITH_SECRETS
-secretRegister();
-# endif
-# ifdef WITH_NWFILTER
-nwfilterRegister();
-# endif
-# ifdef WITH_XEN
-xenRegister();
-# endif
-# ifdef WITH_LIBXL
-libxlRegister();
-# endif
-# ifdef WITH_QEMU
-qemuRegister();
-# endif
-# ifdef WITH_LXC
-lxcRegister();
-# endif
-# ifdef WITH_UML
-umlRegister();
-# endif
-# ifdef WITH_VBOX
-vboxRegister();
-# endif
-# ifdef WITH_BHYVE
-bhyveRegister();
-# endif
-# ifdef WITH_VZ
-vzRegister();
-# endif
+#ifdef WITH_NETWORK
+VIR_DAEMON_LOAD_MODULE(networkRegister, "network");
+#endif
+#ifdef WITH_INTERFACE
+VIR_DAEMON_LOAD_MODULE(interfaceRegister, "interface");
+#endif
+#ifdef WITH_STORAGE
+VIR_DAEMON_LOAD_MODULE(storageRegister, "storage");
+#endif
+#ifdef WITH_NODE_DEVICES
+VIR_DAEMON_LOAD_MODULE(nodedevRegister, "nodedev");
+#endif
+#ifdef WITH_SECRETS
+VIR_DAEMON_LOAD_MODULE(secretRegister, "secret");
+#endif
+#ifdef WITH_NWFILTER
+VIR_DAEMON_LOAD_MODULE(nwfilterRegister, "nwfilter");
+#endif
+#ifdef WITH_XEN
+VIR_DAEMON_LOAD_MODULE(xenRegister, "xen");
+#endif
+#ifdef WITH_LIBXL
+VIR_DAEMON_LOAD_MODULE(libxlRegister, "libxl");
+#endif
+#ifdef WITH_QEMU
+VIR_DAEMON_LOAD_MODULE(qemuRegister, "qemu");
+#endif
+#ifdef WITH_LXC
+VIR_DAEMON_LOAD_MODULE(lxcRegister, "lxc");
+#endif
+#ifdef WITH_UML
+VIR_DAEMON_LOAD_MODULE(umlRegister, "uml");
+#endif
+#ifdef WITH_VBOX
+VIR_DAEMON_LOAD_MODULE(vboxRegister, "vbox");
+#endif
+#ifdef WITH_BHYVE
+VIR_DAEMON_LOAD_MODULE(bhyveRegister, "bhyve");
+#endif
+#ifdef WITH_VZ
+VIR_DAEMON_LOAD_MODULE(vzRegister, "vz");
 #endif
 }
+#undef VIR_DAEMON_LOAD_MODULE


 static int ATTRIBUTE_NONNULL(3)
diff --git a/src/driver.c b/src/driver.c
index 783e08a28..f47bea0fb 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -23,15 +23,12 @@
 #include 

 #include 
-#include 

 #include "driver.h"
 #include "viralloc.h"
 #include "virfile.h"
 #include "virlog.h"
-#include "virutil.h"
 #include "configmake.h"
-#include "virstring.h"

 VIR_LOG_INIT("driver");

@@ -134,14 +131,12 @@ virDriverLoadModuleFull(const char *path,
 }


-void *
-virDriverLoadModule(const char *name)
+int
+virDriverLoadModule(const char *name,
+const char *regfunc)
 {
 char *modfile = NULL;
-char *fixedname = NULL;

[libvirt] [PATCH 10/11] spec: Modularize the storage driver

[Peter Krempa]

Create a new set of sub-packages containing the new storage driver
modules so that certain heavy-weight backends (gluster, rbd) can be
installed separately only if required.

To keep backward compatibility the 'libvirt-driver-storage' package
will be turned into a virtual package pulling in all the new storage
backend sub-packages. The storage driver module will be moved into
libvirt-driver-storage-core including the filesystem backend which is
mandatory.

This then allows to make libvirt-daemon-driver-qemu depend only on the
core of the storage driver.

All other meta-packages still depend on the full storage driver and thus
pull in all the backends.
---
 libvirt.spec.in | 168 +++-
 1 file changed, 143 insertions(+), 25 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 3098ed2dd..9b2ca1c42 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -583,35 +583,13 @@ Requires: libvirt-daemon = %{version}-%{release}
 The secret driver plugin for the libvirtd daemon, providing
 an implementation of the secret key APIs.

-
-%package daemon-driver-storage
-Summary: Storage driver plugin for the libvirtd daemon
+%package daemon-driver-storage-core
+Summary: Storage driver plugin including base backends for the libvirtd daemon
 Group: Development/Libraries
 Requires: libvirt-daemon = %{version}-%{release}
 Requires: nfs-utils
 # For mkfs
 Requires: util-linux
-# For glusterfs
-%if 0%{?fedora}
-Requires: glusterfs-client >= 2.0.1
-%endif
-# gluster cli tool for pool discovery
-%if (0%{?fedora} || 0%{?with_storage_gluster})
-Requires: /usr/sbin/gluster
-%endif
-# For LVM drivers
-Requires: lvm2
-# For ISCSI driver
-Requires: iscsi-initiator-utils
-# For disk driver
-Requires: parted
-Requires: device-mapper
-# For multipath support
-Requires: device-mapper
-%if %{with_storage_sheepdog}
-# For Sheepdog support
-Requires: sheepdog
-%endif
 %if %{with_qemu}
 # From QEMU RPMs
 Requires: /usr/bin/qemu-img
@@ -622,6 +600,128 @@ Requires: /usr/sbin/qcow-create
 %endif
 %endif

+%description daemon-driver-storage-core
+The storage driver plugin for the libvirtd daemon, providing
+an implementation of the storage APIs using files, local disks, LVM, SCSI,
+iSCSI, and multipath storage.
+
+%package daemon-driver-storage-logical
+Summary: Storage driver plugin for lvm volumes
+Group: Development/Libraries
+Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
+Requires: lvm2
+
+%description daemon-driver-storage-logical
+The storage driver backend adding implementation of the storage APIs for block
+volumes using lvm.
+
+
+%package daemon-driver-storage-disk
+Summary: Storage driver plugin for disk
+Group: Development/Libraries
+Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
+Requires: parted
+Requires: device-mapper
+
+%description daemon-driver-storage-disk
+The storage driver backend adding implementation of the storage APIs for block
+volumes using the host disks.
+
+
+%package daemon-driver-storage-scsi
+Summary: Storage driver plugin for local scsi devices
+Group: Development/Libraries
+Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
+
+%description daemon-driver-storage-scsi
+The storage driver backend adding implementation of the storage APIs for scsi
+host devices.
+
+
+%package daemon-driver-storage-iscsi
+Summary: Storage driver plugin for iscsi
+Group: Development/Libraries
+Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
+Requires: iscsi-initiator-utils
+
+%description daemon-driver-storage-iscsi
+The storage driver backend adding implementation of the storage APIs for iscsi
+volumes using the host iscsi stack.
+
+
+%package daemon-driver-storage-mpath
+Summary: Storage driver plugin for multipath volumes
+Group: Development/Libraries
+Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
+Requires: device-mapper
+
+%description daemon-driver-storage-mpath
+The storage driver backend adding implementation of the storage APIs for
+multipath storage using device mapper.
+
+
+%if %{with_storage_gluster}
+%package daemon-driver-storage-gluster
+Summary: Storage driver plugin for gluster
+Group: Development/Libraries
+Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
+%if 0%{?fedora}
+Requires: glusterfs-client >= 2.0.1
+%endif
+%if (0%{?fedora} || 0%{?with_storage_gluster})
+Requires: /usr/sbin/gluster
+%endif
+
+%description daemon-driver-storage-gluster
+The storage driver backend adding implementation of the storage APIs for 
gluster
+volumes using libgfapi.
+%endif
+
+
+%if %{with_storage_rbd}
+%package daemon-driver-storage-rbd
+Summary: Storage driver plugin for rbd
+Group: Development/Libraries
+Requires: libvirt-daemon-driver-storage-core = %{version}-%{release}
+
+%description daemon-driver-storage-rbd
+The storage driver backend adding implementation of the storage APIs for rbd
+volumes using the ceph protocol.

[libvirt] [PATCH 01/11] configure: Fix configure output for RBD storage backend

[Peter Krempa]

We'd print status for the 'dir' backend instead of the correct one.
---
 m4/virt-storage-rbd.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/m4/virt-storage-rbd.m4 b/m4/virt-storage-rbd.m4
index 0104b7cfc..48522a6a7 100644
--- a/m4/virt-storage-rbd.m4
+++ b/m4/virt-storage-rbd.m4
@@ -41,7 +41,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_RBD], [
 ])

 AC_DEFUN([LIBVIRT_STORAGE_RESULT_RBD], [
-  LIBVIRT_RESULT([RBD], [$with_storage_dir])
+  LIBVIRT_RESULT([RBD], [$with_storage_rbd])
 ])

 AC_DEFUN([LIBVIRT_RESULT_RBD], [
-- 
2.11.0

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

[libvirt] [PATCH 08/11] storage: Turn storage backends into dynamic modules

[Peter Krempa]

If driver modules are enabled turn storage driver backends into
dynamically loadable objects. This will allow greater modularity for
binary distributions, where heavyweight dependencies as rbd and gluster
can be avoided by selecting only a subset of drivers if the rest is not
necessary.

The storage modules are installed into 'LIBDIR/libvirt/storage-backend/'
and users can't override the location by using
'LIBVIRT_STORAGE_BACKEND_DIR' environment variable.

rpm based distros will at this point install all the backends when
libvirt-daemon-driver-storage package is installed.
---
 libvirt.spec.in   | 17 +
 src/Makefile.am   | 85 ++-
 src/storage/storage_backend.c | 60 +++---
 tests/Makefile.am |  4 +-
 4 files changed, 151 insertions(+), 15 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index e8c272bd7..3098ed2dd 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1234,6 +1234,8 @@ rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/lock-driver/*.la
 rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/lock-driver/*.a
 rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/connection-driver/*.la
 rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/connection-driver/*.a
+rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/storage-backend/*.la
+rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/storage-backend/*.a
 %if %{with_wireshark}
 %if 0%{fedora} >= 24
 rm -f $RPM_BUILD_ROOT%{_libdir}/wireshark/plugins/libvirt.la
@@ -1689,6 +1691,21 @@ exit 0
 %files daemon-driver-storage
 %attr(0755, root, root) %{_libexecdir}/libvirt_parthelper
 %{_libdir}/%{name}/connection-driver/libvirt_driver_storage.so
+%{_libdir}/%{name}/storage-backend/libvirt_storage_backend_fs.so
+%{_libdir}/%{name}/storage-backend/libvirt_storage_backend_disk.so
+%{_libdir}/%{name}/storage-backend/libvirt_storage_backend_logical.so
+%{_libdir}/%{name}/storage-backend/libvirt_storage_backend_scsi.so
+%{_libdir}/%{name}/storage-backend/libvirt_storage_backend_iscsi.so
+%{_libdir}/%{name}/storage-backend/libvirt_storage_backend_mpath.so
+%if %{with_storage_gluster}
+%{_libdir}/%{name}/storage-backend/libvirt_storage_backend_gluster.so
+%endif
+%if %{with_storage_rbd}
+%{_libdir}/%{name}/storage-backend/libvirt_storage_backend_rbd.so
+%endif
+%if %{with_storage_sheepdog}
+%{_libdir}/%{name}/storage-backend/libvirt_storage_backend_sheepdog.so
+%endif

 %if %{with_qemu}
 %files daemon-driver-qemu
diff --git a/src/Makefile.am b/src/Makefile.am
index b71209a9d..cdac7a1b5 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -970,9 +970,12 @@ SECRET_DRIVER_SOURCES =
\
secret/secret_driver.h secret/secret_driver.c

 # Storage backend specific impls
+STORAGE_DRIVER_BACKEND_SOURCES = \
+   storage/storage_backend.h storage/storage_backend.c
+
 STORAGE_DRIVER_SOURCES =   \
storage/storage_driver.h storage/storage_driver.c   \
-   storage/storage_backend.h storage/storage_backend.c \
+   $(STORAGE_DRIVER_BACKEND_SOURCES) \
storage/storage_util.h storage/storage_util.c

 STORAGE_DRIVER_FS_SOURCES =\
@@ -1660,6 +1663,12 @@ if WITH_BLKID
 libvirt_driver_storage_impl_la_CFLAGS += $(BLKID_CFLAGS)
 libvirt_driver_storage_impl_la_LIBADD += $(BLKID_LIBS)
 endif WITH_BLKID
+
+if WITH_DRIVER_MODULES
+storagebackenddir = $(libdir)/libvirt/storage-backend
+storagebackend_LTLIBRARIES =
+endif WITH_DRIVER_MODULES
+
 if WITH_STORAGE
 noinst_LTLIBRARIES += libvirt_driver_storage_impl.la
 libvirt_driver_storage_la_SOURCES =
@@ -1681,8 +1690,14 @@ libvirt_storage_backend_fs_la_CFLAGS =   \
-I$(srcdir)/conf \
$(AM_CFLAGS)

+if WITH_DRIVER_MODULES
+storagebackend_LTLIBRARIES += libvirt_storage_backend_fs.la
+libvirt_storage_backend_fs_la_LDFLAGS = \
+   -module -avoid-version $(AM_LDFLAGS)
+else ! WITH_DRIVER_MODULES
 noinst_LTLIBRARIES += libvirt_storage_backend_fs.la
 libvirt_driver_storage_impl_la_LIBADD += libvirt_storage_backend_fs.la
+endif ! WITH_DRIVER_MODULES
 endif WITH_STORAGE

 if WITH_STORAGE_LVM
@@ -1692,9 +1707,15 @@ libvirt_storage_backend_logical_la_CFLAGS = \
-I$(srcdir)/conf \
$(AM_CFLAGS)

+if WITH_DRIVER_MODULES
+storagebackend_LTLIBRARIES += libvirt_storage_backend_logical.la
+libvirt_storage_backend_logical_la_LDFLAGS = \
+   -module -avoid-version $(AM_LDFLAGS)
+else ! WITH_DRIVER_MODULES
 noinst_LTLIBRARIES += libvirt_storage_backend_logical.la
 libvirt_driver_storage_impl_la_LIBADD += \
libvirt_storage_backend_logical.la
+endif ! WITH_DRIVER_MODULES
 endif WITH_STORAGE_LVM

 if WITH_STORAGE_ISCSI
@@ -1705,9 +1726,15 @@ libvirt_storage_backend_iscsi_la_CFLAGS = \
-I$(srcdir)/secret \
$(AM_CFLAGS)

+if WITH_DRIVER_MODULES
+storagebackend_LTLIBRARIES += libvirt_storage_backend_iscsi.la
+libvirt_storage_backend_iscsi_la_LDFLAGS = 

[libvirt] [PATCH 04/11] driver: Split/refactor driver module loading

[Peter Krempa]

Split the convoluted driver loader function into simpler parts which
will potentially allow reuse.
---
 src/driver.c| 133 +---
 src/driver.h|   3 +
 src/libvirt_driver_modules.syms |   1 +
 3 files changed, 101 insertions(+), 36 deletions(-)

diff --git a/src/driver.c b/src/driver.c
index 67ac02006..783e08a28 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -43,15 +43,105 @@ VIR_LOG_INIT("driver");
 # include 
 # define DEFAULT_DRIVER_DIR LIBDIR "/libvirt/connection-driver"

+
+static void *
+virDriverLoadModuleFile(const char *file)
+{
+void *handle = NULL;
+
+VIR_DEBUG("Load module file '%s'", file);
+
+if (access(file, R_OK) < 0) {
+VIR_INFO("Module %s not accessible", file);
+return NULL;
+}
+
+virUpdateSelfLastChanged(file);
+
+if (!(handle = dlopen(file, RTLD_NOW | RTLD_GLOBAL)))
+VIR_ERROR(_("failed to load module %s %s"), file, dlerror());
+
+return handle;
+}
+
+
+static void *
+virDriverLoadModuleFunc(void *handle,
+const char *funcname)
+{
+void *regsym;
+
+VIR_DEBUG("Lookup function '%s'", funcname);
+
+if (!(regsym = dlsym(handle, funcname)))
+VIR_ERROR(_("Missing module registration symbol %s"), funcname);
+
+return regsym;
+}
+
+
+/**
+ * virDriverLoadModuleFull:
+ * @path: filename of module to load
+ * @regfunc: name of the function that registers the module
+ * @handle: Returns handle of the loaded library if not NULL
+ *
+ * Loads a loadable module named @path and calls the
+ * registration function @regfunc. If @handle is not NULL the handle is 
returned
+ * in the variable. Otherwise the handle is leaked so that the module stays
+ * loaded forever.
+ *
+ * The module is automatically looked up in the appropriate place (git or
+ * installed directory).
+ *
+ * Returns 0 on success, 1 if the module was not found and -1 on any error.
+ */
+int
+virDriverLoadModuleFull(const char *path,
+const char *regfunc,
+void **handle)
+{
+void *rethandle = NULL;
+int (*regsym)(void);
+int ret = -1;
+
+VIR_DEBUG("Module load %s", path);
+
+if (!(rethandle = virDriverLoadModuleFile(path))) {
+ret = 1;
+goto cleanup;
+}
+
+if (!(regsym = virDriverLoadModuleFunc(rethandle, regfunc)))
+goto cleanup;
+
+if ((*regsym)() < 0) {
+VIR_ERROR(_("Failed module registration %s"), regfunc);
+goto cleanup;
+}
+
+if (handle)
+VIR_STEAL_PTR(*handle, rethandle);
+else
+rethandle = NULL;
+
+ret = 0;
+
+ cleanup:
+if (rethandle)
+dlclose(rethandle);
+return ret;
+}
+
+
 void *
 virDriverLoadModule(const char *name)
 {
-char *modfile = NULL, *regfunc = NULL, *fixedname = NULL;
+char *modfile = NULL;
+char *fixedname = NULL;
+char *regfunc = NULL;
 char *tmp;
 void *handle = NULL;
-int (*regsym)(void);
-
-VIR_DEBUG("Module load %s", name);

 if (!(modfile = virFileFindResourceFull(name,
 "libvirt_driver_",
@@ -61,19 +151,6 @@ virDriverLoadModule(const char *name)
 "LIBVIRT_DRIVER_DIR")))
 return NULL;

-if (access(modfile, R_OK) < 0) {
-VIR_INFO("Module %s not accessible", modfile);
-goto cleanup;
-}
-
-virUpdateSelfLastChanged(modfile);
-
-handle = dlopen(modfile, RTLD_NOW | RTLD_GLOBAL);
-if (!handle) {
-VIR_ERROR(_("failed to load module %s %s"), modfile, dlerror());
-goto cleanup;
-}
-
 if (VIR_STRDUP_QUIET(fixedname, name) < 0) {
 VIR_ERROR(_("out of memory"));
 goto cleanup;
@@ -88,29 +165,13 @@ virDriverLoadModule(const char *name)
 if (virAsprintfQuiet(, "%sRegister", fixedname) < 0)
 goto cleanup;

-regsym = dlsym(handle, regfunc);
-if (!regsym) {
-VIR_ERROR(_("Missing module registration symbol %s"), regfunc);
-goto cleanup;
-}
-
-if ((*regsym)() < 0) {
-VIR_ERROR(_("Failed module registration %s"), regfunc);
-goto cleanup;
-}
-
-VIR_FREE(modfile);
-VIR_FREE(regfunc);
-VIR_FREE(fixedname);
-return handle;
+virDriverLoadModuleFull(modfile, regfunc, );

  cleanup:
 VIR_FREE(modfile);
-VIR_FREE(regfunc);
 VIR_FREE(fixedname);
-if (handle)
-dlclose(handle);
-return NULL;
+VIR_FREE(regfunc);
+return handle;
 }


diff --git a/src/driver.h b/src/driver.h
index e4e382b63..885e8843e 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -100,5 +100,8 @@ int virSetSharedSecretDriver(virSecretDriverPtr driver) 
ATTRIBUTE_RETURN_CHECK;
 int virSetSharedStorageDriver(virStorageDriverPtr driver) 
ATTRIBUTE_RETURN_CHECK;

 void *virDriverLoadModule(const char *name);
+int virDriverLoadModuleFull(const char *name,
+const char *regfunc,
+ 

[libvirt] [PATCH 07/11] storage: Turn driver backends into (static) modules

[Peter Krempa]

Compile the storage driver into modules rather than by compiling all
files together. All modules are still linked together statically.
---
 src/Makefile.am | 114 
 1 file changed, 98 insertions(+), 16 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 2f32d4197..b71209a9d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1674,52 +1674,134 @@ noinst_LTLIBRARIES += libvirt_driver_storage.la
 #libvirt_la_BUILT_LIBADD += libvirt_driver_storage.la
 endif ! WITH_DRIVER_MODULES
 libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_SOURCES)
-libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_FS_SOURCES)
+
+
+libvirt_storage_backend_fs_la_SOURCES = $(STORAGE_DRIVER_FS_SOURCES)
+libvirt_storage_backend_fs_la_CFLAGS = \
+   -I$(srcdir)/conf \
+   $(AM_CFLAGS)
+
+noinst_LTLIBRARIES += libvirt_storage_backend_fs.la
+libvirt_driver_storage_impl_la_LIBADD += libvirt_storage_backend_fs.la
 endif WITH_STORAGE

 if WITH_STORAGE_LVM
-libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_LVM_SOURCES)
+libvirt_storage_backend_logical_la_SOURCES = \
+   $(STORAGE_DRIVER_LVM_SOURCES)
+libvirt_storage_backend_logical_la_CFLAGS = \
+   -I$(srcdir)/conf \
+   $(AM_CFLAGS)
+
+noinst_LTLIBRARIES += libvirt_storage_backend_logical.la
+libvirt_driver_storage_impl_la_LIBADD += \
+   libvirt_storage_backend_logical.la
 endif WITH_STORAGE_LVM

 if WITH_STORAGE_ISCSI
-libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_ISCSI_SOURCES)
+libvirt_storage_backend_iscsi_la_SOURCES = \
+   $(STORAGE_DRIVER_ISCSI_SOURCES)
+libvirt_storage_backend_iscsi_la_CFLAGS = \
+   -I$(srcdir)/conf \
+   -I$(srcdir)/secret \
+   $(AM_CFLAGS)
+
+noinst_LTLIBRARIES += libvirt_storage_backend_iscsi.la
+libvirt_driver_storage_impl_la_LIBADD += \
+   libvirt_storage_backend_iscsi.la
 endif WITH_STORAGE_ISCSI

 if WITH_STORAGE_SCSI
-libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_SCSI_SOURCES)
+libvirt_storage_backend_scsi_la_SOURCES = $(STORAGE_DRIVER_SCSI_SOURCES)
+libvirt_storage_backend_scsi_la_CFLAGS = \
+   -I$(srcdir)/conf \
+   $(AM_CFLAGS)
+
+noinst_LTLIBRARIES += libvirt_storage_backend_scsi.la
+libvirt_driver_storage_impl_la_LIBADD += libvirt_storage_backend_scsi.la
 endif WITH_STORAGE_SCSI

 if WITH_STORAGE_MPATH
-libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_MPATH_SOURCES)
-libvirt_driver_storage_impl_la_CFLAGS += $(DEVMAPPER_CFLAGS)
-libvirt_driver_storage_impl_la_LIBADD += $(DEVMAPPER_LIBS)
+libvirt_storage_backend_mpath_la_SOURCES = \
+   $(STORAGE_DRIVER_MPATH_SOURCES)
+libvirt_storage_backend_mpath_la_LIBADD = $(DEVMAPPER_LIBS)
+libvirt_storage_backend_mpath_la_CFLAGS =  \
+   -I$(srcdir)/conf \
+   $(DEVMAPPER_CFLAGS) \
+   $(AM_CFLAGS)
+
+noinst_LTLIBRARIES += libvirt_storage_backend_mpath.la
+libvirt_driver_storage_impl_la_LIBADD += \
+   libvirt_storage_backend_mpath.la
 endif WITH_STORAGE_MPATH

 if WITH_STORAGE_DISK
-libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_DISK_SOURCES)
+libvirt_storage_backend_disk_la_SOURCES = $(STORAGE_DRIVER_DISK_SOURCES)
+libvirt_storage_backend_disk_la_CFLAGS =   \
+   -I$(srcdir)/conf \
+   $(AM_CFLAGS)
+
+noinst_LTLIBRARIES += libvirt_storage_backend_disk.la
+libvirt_driver_storage_impl_la_LIBADD += libvirt_storage_backend_disk.la
 endif WITH_STORAGE_DISK

 if WITH_STORAGE_RBD
-libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_RBD_SOURCES)
-libvirt_driver_storage_impl_la_LIBADD += $(LIBRBD_LIBS)
+libvirt_storage_backend_rbd_la_SOURCES = $(STORAGE_DRIVER_RBD_SOURCES)
+libvirt_storage_backend_rbd_la_LIBADD = $(LIBRBD_LIBS)
+libvirt_storage_backend_rbd_la_CFLAGS = \
+   -I$(srcdir)/conf \
+   -I$(srcdir)/secret \
+   $(AM_CFLAGS)
+
+noinst_LTLIBRARIES += libvirt_storage_backend_rbd.la
+libvirt_driver_storage_impl_la_LIBADD += libvirt_storage_backend_rbd.la
 endif WITH_STORAGE_RBD

 if WITH_STORAGE_SHEEPDOG
-libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_SHEEPDOG_SOURCES)
+libvirt_storage_backend_sheepdog_la_SOURCES = \
+   $(STORAGE_DRIVER_SHEEPDOG_SOURCES)
+libvirt_storage_backend_sheepdog_la_CFLAGS = \
+   -I$(srcdir)/conf \
+   $(AM_CFLAGS)
+
+noinst_LTLIBRARIES += libvirt_storage_backend_sheepdog.la
+libvirt_driver_storage_impl_la_LIBADD += \
+   libvirt_storage_backend_sheepdog.la
 endif WITH_STORAGE_SHEEPDOG

 if WITH_STORAGE_GLUSTER
-libvirt_driver_storage_impl_la_SOURCES += $(STORAGE_DRIVER_GLUSTER_SOURCES)
-libvirt_driver_storage_impl_la_CFLAGS += $(GLUSTERFS_CFLAGS)
-libvirt_driver_storage_impl_la_LIBADD += $(GLUSTERFS_LIBS)
+libvirt_storage_backend_gluster_la_SOURCES = \
+   $(STORAGE_DRIVER_GLUSTER_SOURCES)
+libvirt_storage_backend_gluster_la_LIBADD = $(GLUSTERFS_LIBS)
+libvirt_storage_backend_gluster_la_CFLAGS = \
+   -I$(srcdir)/conf \
+   $(GLUSTERFS_CFLAGS) \
+   $(AM_CFLAGS)
+
+noinst_LTLIBRARIES += 

[libvirt] [PATCH 09/11] tests: drivermodule: Make sure that all compiled storage backends can be loaded

[Peter Krempa]

Add a new storage driver registration function that will force the
backend code to fail if any of the storage backend modules can't be
loaded. This will make sure that they work and are present.
---
 src/storage/storage_backend.c | 16 
 src/storage/storage_backend.h |  2 +-
 src/storage/storage_driver.c  | 19 +--
 src/storage/storage_driver.h  |  1 +
 tests/virdrivermoduletest.c   |  2 +-
 tests/virstoragetest.c|  2 +-
 6 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 32f45e841..ce278b99c 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -87,7 +87,8 @@ static size_t virStorageFileBackendsCount;

 static int
 virStorageDriverLoadBackendModule(const char *name,
-  const char *regfunc)
+  const char *regfunc,
+  bool forceload)
 {
 char *modfile = NULL;
 int ret;
@@ -100,7 +101,14 @@ virStorageDriverLoadBackendModule(const char *name,
 "LIBVIRT_STORAGE_BACKEND_DIR")))
 return 1;

-ret = virDriverLoadModuleFull(modfile, regfunc, NULL);
+if ((ret = virDriverLoadModuleFull(modfile, regfunc, NULL)) != 0) {
+if (forceload) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to load storage backend module '%s'"),
+   name);
+ret = -1;
+}
+}

 VIR_FREE(modfile);

@@ -109,7 +117,7 @@ virStorageDriverLoadBackendModule(const char *name,


 # define VIR_STORAGE_BACKEND_REGISTER(func, module)
\
-if (virStorageDriverLoadBackendModule(module, #func) < 0)  
\
+if (virStorageDriverLoadBackendModule(module, #func, allbackends) < 0) 
\
 return -1
 #else
 # define VIR_STORAGE_BACKEND_REGISTER(func, module)
\
@@ -118,7 +126,7 @@ virStorageDriverLoadBackendModule(const char *name,
 #endif

 int
-virStorageBackendDriversRegister(void)
+virStorageBackendDriversRegister(bool allbackends ATTRIBUTE_UNUSED)
 {
 #if WITH_STORAGE_DIR || WITH_STORAGE_FS
 VIR_STORAGE_BACKEND_REGISTER(virStorageBackendFsRegister, "fs");
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index ca6c19c45..f433071f4 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -198,7 +198,7 @@ struct _virStorageFileBackend {
 virStorageFileBackendChown  storageFileChown;
 };

-int virStorageBackendDriversRegister(void);
+int virStorageBackendDriversRegister(bool allmodules);

 int virStorageBackendRegister(virStorageBackendPtr backend);
 int virStorageBackendFileRegister(virStorageFileBackendPtr backend);
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 7fafbcf75..0bc047f23 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2838,9 +2838,10 @@ static virStateDriver stateDriver = {
 .stateReload = storageStateReload,
 };

-int storageRegister(void)
+static int
+storageRegisterFull(bool allbackends)
 {
-if (virStorageBackendDriversRegister() < 0)
+if (virStorageBackendDriversRegister(allbackends) < 0)
 return -1;
 if (virSetSharedStorageDriver() < 0)
 return -1;
@@ -2850,6 +2851,20 @@ int storageRegister(void)
 }


+int
+storageRegister(void)
+{
+return storageRegisterFull(false);
+}
+
+
+int
+storageRegisterAll(void)
+{
+return storageRegisterFull(true);
+}
+
+
 /* --- file handlers cooperating with storage driver --- */
 static bool
 virStorageFileIsInitialized(const virStorageSource *src)
diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h
index 682c9ff82..f0aca3671 100644
--- a/src/storage/storage_driver.h
+++ b/src/storage/storage_driver.h
@@ -70,5 +70,6 @@ char *virStoragePoolObjBuildTempFilePath(virStoragePoolObjPtr 
pool,
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;

 int storageRegister(void);
+int storageRegisterAll(void);

 #endif /* __VIR_STORAGE_DRIVER_H__ */
diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c
index e440350c2..13d51a8e9 100644
--- a/tests/virdrivermoduletest.c
+++ b/tests/virdrivermoduletest.c
@@ -71,7 +71,7 @@ mymain(void)
 TEST("interface");
 #endif
 #ifdef WITH_STORAGE
-TEST("storage");
+TEST_FULL("storage", "storageRegisterAll");
 #endif
 #ifdef WITH_NODE_DEVICES
 TEST("nodedev");
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index d715fd762..36567753b 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -732,7 +732,7 @@ mymain(void)
 virStorageSourcePtr chain2; /* short for chain->backingStore */
 virStorageSourcePtr chain3; /* short for chain2->backingStore */

-if (virStorageBackendDriversRegister() < 0)
+if 

[libvirt] [PATCH 03/11] tests: drivermodule: Drop unused macro arguments

[Peter Krempa]

Refactors of the test resulted into the second argument of the 'TEST'
macro to be unused. Drop them.
---
 tests/virdrivermoduletest.c | 31 ++-
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c
index 870a07de6..09c1a614b 100644
--- a/tests/virdrivermoduletest.c
+++ b/tests/virdrivermoduletest.c
@@ -47,53 +47,50 @@ mymain(void)
 {
 int ret = 0;

-#define TEST(name, dep1)   \
+#define TEST(name) \
 do  {  \
 if (virTestRun("Test driver " # name, testDriverModule, name) < 0) \
 ret = -1;  \
 } while (0)

 #ifdef WITH_NETWORK
-# define USE_NETWORK "network"
-TEST("network", NULL);
-#else
-# define USE_NETWORK NULL
+TEST("network");
 #endif
 #ifdef WITH_INTERFACE
-TEST("interface", NULL);
+TEST("interface");
 #endif
 #ifdef WITH_STORAGE
-TEST("storage", NULL);
+TEST("storage");
 #endif
 #ifdef WITH_NODE_DEVICES
-TEST("nodedev", NULL);
+TEST("nodedev");
 #endif
 #ifdef WITH_SECRETS
-TEST("secret", NULL);
+TEST("secret");
 #endif
 #ifdef WITH_NWFILTER
-TEST("nwfilter", NULL);
+TEST("nwfilter");
 #endif
 #ifdef WITH_XEN
-TEST("xen", NULL);
+TEST("xen");
 #endif
 #ifdef WITH_LIBXL
-TEST("libxl", NULL);
+TEST("libxl");
 #endif
 #ifdef WITH_QEMU
-TEST("qemu", USE_NETWORK);
+TEST("qemu");
 #endif
 #ifdef WITH_LXC
-TEST("lxc", USE_NETWORK);
+TEST("lxc");
 #endif
 #ifdef WITH_UML
-TEST("uml", NULL);
+TEST("uml");
 #endif
 #ifdef WITH_VBOX
-TEST("vbox", NULL);
+TEST("vbox");
 #endif
 #ifdef WITH_BHYVE
-TEST("bhyve", NULL);
+TEST("bhyve");
 #endif

 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
-- 
2.11.0

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

[libvirt] [PATCH 06/11] storage: backend: Refactor registration of the backend drivers

[Peter Krempa]

Add APIs that allow to dynamically register driver backends so that the
list of available drivers does not need to be known during compile time.

This will allow us to modularize the storage driver on runtime.
---
 src/storage/storage_backend.c  | 111 ++---
 src/storage/storage_backend.h  |   5 ++
 src/storage/storage_backend_disk.c |   7 +++
 src/storage/storage_backend_disk.h |   4 +-
 src/storage/storage_backend_fs.c   |  27 
 src/storage/storage_backend_fs.h   |  11 +---
 src/storage/storage_backend_gluster.c  |  13 +++-
 src/storage/storage_backend_gluster.h  |   5 +-
 src/storage/storage_backend_iscsi.c|   7 +++
 src/storage/storage_backend_iscsi.h|   4 +-
 src/storage/storage_backend_logical.c  |   7 +++
 src/storage/storage_backend_logical.h  |   4 +-
 src/storage/storage_backend_mpath.c|   8 +++
 src/storage/storage_backend_mpath.h|   4 +-
 src/storage/storage_backend_rbd.c  |   7 +++
 src/storage/storage_backend_rbd.h  |   4 +-
 src/storage/storage_backend_scsi.c |   7 +++
 src/storage/storage_backend_scsi.h |   4 +-
 src/storage/storage_backend_sheepdog.c |   7 +++
 src/storage/storage_backend_sheepdog.h |   4 +-
 src/storage/storage_backend_vstorage.c |   7 +++
 src/storage/storage_backend_vstorage.h |   4 +-
 src/storage/storage_backend_zfs.c  |   7 +++
 src/storage/storage_backend_zfs.h  |   4 +-
 src/storage/storage_driver.c   |   2 +
 tests/virstoragetest.c |   4 ++
 26 files changed, 200 insertions(+), 78 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 500d7567d..d8099be36 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -72,67 +72,106 @@

 VIR_LOG_INIT("storage.storage_backend");

-static virStorageBackendPtr backends[] = {
-#if WITH_STORAGE_DIR
-,
-#endif
-#if WITH_STORAGE_FS
-,
-,
+#define VIR_STORAGE_BACKENDS_MAX 20
+
+static virStorageBackendPtr virStorageBackends[VIR_STORAGE_BACKENDS_MAX];
+static size_t virStorageBackendsCount;
+static virStorageFileBackendPtr 
virStorageFileBackends[VIR_STORAGE_BACKENDS_MAX];
+static size_t virStorageFileBackendsCount;
+
+#define VIR_STORAGE_BACKEND_REGISTER(name) 
\
+if (name() < 0)
\
+return -1
+
+int
+virStorageBackendDriversRegister(void)
+{
+#if WITH_STORAGE_DIR || WITH_STORAGE_FS
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendFsRegister);
 #endif
 #if WITH_STORAGE_LVM
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendLogicalRegister);
 #endif
 #if WITH_STORAGE_ISCSI
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendISCSIRegister);
 #endif
 #if WITH_STORAGE_SCSI
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendSCSIRegister);
 #endif
 #if WITH_STORAGE_MPATH
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendMpathRegister);
 #endif
 #if WITH_STORAGE_DISK
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendDiskRegister);
 #endif
 #if WITH_STORAGE_RBD
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendRBDRegister);
 #endif
 #if WITH_STORAGE_SHEEPDOG
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendSheepdogRegister);
 #endif
 #if WITH_STORAGE_GLUSTER
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendGlusterRegister);
 #endif
 #if WITH_STORAGE_ZFS
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendZFSRegister);
 #endif
 #if WITH_STORAGE_VSTORAGE
-,
+VIR_STORAGE_BACKEND_REGISTER(virStorageBackendVstorageRegister);
 #endif
-NULL
-};

+return 0;
+}
+#undef VIR_STORAGE_BACKEND_REGISTER

-static virStorageFileBackendPtr fileBackends[] = {
-#if WITH_STORAGE_FS
-,
-,
-#endif
-#if WITH_STORAGE_GLUSTER
-,
-#endif
-NULL
-};
+
+int
+virStorageBackendRegister(virStorageBackendPtr backend)
+{
+VIR_DEBUG("Registering storage backend '%s'",
+  virStorageTypeToString(backend->type));
+
+if (virStorageBackendsCount >= VIR_STORAGE_BACKENDS_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Too many drivers, cannot register storage backend 
'%s'"),
+   virStorageTypeToString(backend->type));
+return -1;
+}
+
+virStorageBackends[virStorageBackendsCount] = backend;
+virStorageBackendsCount++;
+return 0;
+}
+
+
+int
+virStorageBackendFileRegister(virStorageFileBackendPtr backend)
+{
+VIR_DEBUG("Registering storage file backend '%s' protocol '%s'",
+  virStorageTypeToString(backend->type),
+  virStorageNetProtocolTypeToString(backend->protocol));
+
+if (virStorageFileBackendsCount >= VIR_STORAGE_BACKENDS_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Too many drivers, cannot register storage file "
+ "backend '%s'"),
+   

[libvirt] [PATCH 11/11] news: Mention storage driver split

[Peter Krempa]

---
 docs/news.xml | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 69ed6a75e..041515253 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -43,6 +43,16 @@
   and network.
 
   
+  
+
+  storage: modularize the storage driver
+
+
+  Split up the storage driver backends into loadable modules so that
+  binary distributions don't have to compromise on shipping the storage
+  driver with all backends which may pull in too many dependencies.
+
+  
 
 
   
-- 
2.11.0

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

[libvirt] [PATCH 02/11] tests: storagepoolxml2xml: Remove compile conditionals

[Peter Krempa]

The XML2XML test should work properly even if the storage backend is
disabled, since it does not use it.
---
 tests/storagepoolxml2xmltest.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c
index 98a844926..79bdc26da 100644
--- a/tests/storagepoolxml2xmltest.c
+++ b/tests/storagepoolxml2xmltest.c
@@ -97,16 +97,10 @@ mymain(void)
 DO_TEST("pool-gluster");
 DO_TEST("pool-gluster-sub");
 DO_TEST("pool-scsi-type-scsi-host-stable");
-#ifdef WITH_STORAGE_ZFS
 DO_TEST("pool-zfs");
 DO_TEST("pool-zfs-sourcedev");
-#endif
-#ifdef WITH_STORAGE_RBD
 DO_TEST("pool-rbd");
-#endif
-#ifdef WITH_STORAGE_VSTORAGE
 DO_TEST("pool-vstorage");
-#endif

 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.11.0

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

[libvirt] [PATCH 00/11] storage: modularize the storage driver backends

[Peter Krempa]

Split up the storage driver backends into loadable modules so that
binary distributions don't have to compromise on shipping the storage
driver with all backends which may pull in too many dependencies.

Peter Krempa (11):
  configure: Fix configure output for RBD storage backend
  tests: storagepoolxml2xml: Remove compile conditionals
  tests: drivermodule: Drop unused macro arguments
  driver: Split/refactor driver module loading
  daemon: Refactor connection driver module loading
  storage: backend: Refactor registration of the backend drivers
  storage: Turn driver backends into (static) modules
  storage: Turn storage backends into dynamic modules
  tests: drivermodule: Make sure that all compiled storage backends can
be loaded
  spec: Modularize the storage driver
  news: Mention storage driver split

 daemon/libvirtd.c  | 136 +-
 docs/news.xml  |  10 ++
 libvirt.spec.in| 185 +-
 m4/virt-storage-rbd.m4 |   2 +-
 src/Makefile.am| 199 ++---
 src/driver.c   | 136 ++
 src/driver.h   |   6 +-
 src/libvirt_driver_modules.syms|   1 +
 src/storage/storage_backend.c  | 151 +++--
 src/storage/storage_backend.h  |   5 +
 src/storage/storage_backend_disk.c |   7 ++
 src/storage/storage_backend_disk.h |   4 +-
 src/storage/storage_backend_fs.c   |  27 +
 src/storage/storage_backend_fs.h   |  11 +-
 src/storage/storage_backend_gluster.c  |  13 ++-
 src/storage/storage_backend_gluster.h  |   5 +-
 src/storage/storage_backend_iscsi.c|   7 ++
 src/storage/storage_backend_iscsi.h|   4 +-
 src/storage/storage_backend_logical.c  |   7 ++
 src/storage/storage_backend_logical.h  |   4 +-
 src/storage/storage_backend_mpath.c|   8 ++
 src/storage/storage_backend_mpath.h|   4 +-
 src/storage/storage_backend_rbd.c  |   7 ++
 src/storage/storage_backend_rbd.h  |   4 +-
 src/storage/storage_backend_scsi.c |   7 ++
 src/storage/storage_backend_scsi.h |   4 +-
 src/storage/storage_backend_sheepdog.c |   7 ++
 src/storage/storage_backend_sheepdog.h |   4 +-
 src/storage/storage_backend_vstorage.c |   7 ++
 src/storage/storage_backend_vstorage.h |   4 +-
 src/storage/storage_backend_zfs.c  |   7 ++
 src/storage/storage_backend_zfs.h  |   4 +-
 src/storage/storage_driver.c   |  19 +++-
 src/storage/storage_driver.h   |   1 +
 tests/Makefile.am  |   4 +-
 tests/storagepoolxml2xmltest.c |   6 -
 tests/virdrivermoduletest.c|  52 +
 tests/virstoragetest.c |   4 +
 38 files changed, 788 insertions(+), 285 deletions(-)

-- 
2.11.0

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

Re: [libvirt] [PATCH 2/2] libxl: use init and dispose functions with libxl_physinfo

[Jim Fehlig]

Joao Martins wrote:
> On 02/02/2017 10:39 PM, Jim Fehlig wrote:
>> The typical pattern when calling libxl functions that populate a
>> structure is
>>
>>   libxl_foo foo;
>>   libxl_foo_init();
>>   libxl_get_foo(ctx, );
>>   ...
>>   libxl_foo_dispose();
>>
>> Fix several instances of libxl_physinfo missing the init and
>> dispose calls.
> Indeed,
> 
>> Signed-off-by: Jim Fehlig 
> 
> Reviewed-by: Joao Martins 
> 
> See also one comment/nit below, perhaps one libxl_physinfo_init could be moved
> slightly up..
> 
>> [...]
> 
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 3a69720..8951bef 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -4286,6 +4286,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn)
>>  if (virNodeGetFreeMemoryEnsureACL(conn) < 0)
>>  goto cleanup;
>>  
>> +libxl_physinfo_init(_info);
> 
> .. namely here? That is before virNodeGetFreeMemoryEnsureACL.

Nice catch. Moved as suggested in my local branch.

Any other comments on this small series? Would be nice to get these bug fixes
committed :-).

Regards,
Jim

> 
> Not that it matters much, as init/dispose in this case just zeroes out 
> phy_info
> region. But just perhaps consistency as you would end disposing an non
> initialized object.
> 
>>  if (libxl_get_physinfo(cfg->ctx, _info)) {
>>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("libxl_get_physinfo_info failed"));
>> @@ -4295,6 +4296,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn)
>>  ret = phy_info.free_pages * cfg->verInfo->pagesize;
>>  
>>   cleanup:
>> +libxl_physinfo_dispose(_info);
>>  virObjectUnref(cfg);
>>  return ret;
>>  }
>>

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

Re: [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting

[Jim Fehlig]

Joao Martins wrote:
> On 02/02/2017 10:31 PM, Jim Fehlig wrote:
>> When the libxl driver is initialized, it creates a virDomainDef
>> object for dom0 and adds it to the list of domains. Total memory
>> for dom0 was being set from the max_memkb field of libxl_dominfo
>> struct retrieved from libxl, but this field can be set to
>> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
>> explicitly set by the user.
>>
>> This patch adds some simple parsing of the Xen commandline,
>> looking for a dom0_mem parameter that also specifies a 'max' value.
>> If not specified, dom0 maximum memory is effectively all physical
>> host memory.
>>
>> Signed-off-by: Jim Fehlig 
>> ---
>>  src/libxl/libxl_conf.c   | 75 
>> 
>>  src/libxl/libxl_conf.h   |  3 ++
>>  src/libxl/libxl_driver.c |  2 +-
>>  3 files changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index b5186f2..bfe0e92 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -34,6 +34,7 @@
>>  #include "internal.h"
>>  #include "virlog.h"
>>  #include "virerror.h"
>> +#include "c-ctype.h"
>>  #include "datatypes.h"
>>  #include "virconf.h"
>>  #include "virfile.h"
>> @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr 
>> cfg,
>>  
>>  }
>>  
>> +/*
>> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' 
>> Xen
>> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
>> + * memory to 8G: dom0_mem=4G,max:8G
>> + *
>> + * If not constrained by the user, dom0 can effectively use all host memory.
>> + * This function returns the configured maximum memory for dom0 in 
>> kilobytes,
>> + * either the user-specified value or total physical memory as a default.
>> + */
>> +unsigned long long
>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
>> +{
>> +char **cmd_tokens = NULL;
>> +char **mem_tokens = NULL;
>> +size_t i;
>> +size_t j;
>> +unsigned long long ret;
>> +libxl_physinfo physinfo;
>> +
>> +if (cfg->verInfo->commandline == NULL ||
>> +!(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
>> +goto physmem;
>> +
>> +for (i = 0; cmd_tokens[i] != NULL; i++) {
>> +if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
>> +continue;
>> +
>> +if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
>> +break;
>> +for (j = 0; mem_tokens[j] != NULL; j++) {
>> +if (STRPREFIX(mem_tokens[j], "max:")) {
>> +char *p = mem_tokens[j] + 4;
>> +unsigned long long multiplier = 1;
>> +
>> +while (c_isdigit(*p))
>> +p++;
>> +if (virStrToLong_ull(mem_tokens[j] + 4, , 10, ) < 0)
>> +break;
>> +if (*p) {
>> +switch (*p) {
>> +case 'k':
>> +case 'K':
>> +multiplier = 1024;
>> +break;
>> +case 'm':
>> +case 'M':
>> +multiplier = 1024 * 1024;
>> +break;
>> +case 'g':
>> +case 'G':
>> +multiplier = 1024 * 1024 * 1024;
>> +break;
>> +}
>> +}
>> +ret = (ret * multiplier) / 1024;
>> +goto cleanup;
>> +}
>> +}
>> +}
>> +
>> + physmem:
>> +/* No 'max' specified in dom0_mem, so dom0 can use all physical memory 
>> */
>> +libxl_physinfo_init();
>> +libxl_get_physinfo(cfg->ctx, );
> Despite being an unlikely event, libxl_get_physinfo can fail here - I think 
> you
> need to check the return value here.

Bummer. I was hoping this function could always return a sane value for dom0 max
memory. But I'm not really sure what that would be if libxl_get_physinfo failed.

> 
>> +ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
>> +libxl_physinfo_dispose();
>> +
>> + cleanup:
>> +virStringListFree(cmd_tokens);
>> +virStringListFree(mem_tokens);
>> +return ret;
>> +}
>> +
>> +
>>  #ifdef LIBXL_HAVE_DEVICE_CHANNEL
>>  static int
>>  libxlPrepareChannel(virDomainChrDefPtr channel,
>> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
>> index 69d7885..c4ddbfe 100644
>> --- a/src/libxl/libxl_conf.h
>> +++ b/src/libxl/libxl_conf.h
>> @@ -173,6 +173,9 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver,
>>  int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>>const char *filename);
>>  
>> +unsigned long long
>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg);
>> +
>>  int
>>  libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
>>  int
>> diff --git 

Re: [libvirt] [PATCH 1/2] vz: fix handle leak in prlsdkHandleVmStateEvent

[Daniel P. Berrange]

On Wed, Feb 08, 2017 at 06:28:33PM +0300, Maxim Nestratov wrote:
> Every successful call of PrlEvent_GetParamByName allocates a handle,
> which has to be freed.
> 
> Signed-off-by: Maxim Nestratov 
> ---
>  src/vz/vz_sdk.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 727b572..ec954dd 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -2114,6 +2114,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver,
>  prlsdkSendEvent(driver, dom, lvEventType, lvEventTypeDetails);
>  
>   cleanup:
> +PrlHandle_Free(eventParam);
>  virObjectUnlock(dom);
>  return;
>  }

ACK

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

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

Re: [libvirt] [PATCH 2/2] vz: fix event handle leak in prlsdkHandlePerfEvent

[Daniel P. Berrange]

On Wed, Feb 08, 2017 at 06:28:34PM +0300, Maxim Nestratov wrote:
> When we happen to lose a domain but still get a performance event
> for it, we should also free the event handle.
> 
> Signed-off-by: Maxim Nestratov 
> ---
>  src/vz/vz_sdk.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index ec954dd..614dca1 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -2197,8 +2197,10 @@ prlsdkHandlePerfEvent(vzDriverPtr driver,
>  virDomainObjPtr dom = NULL;
>  vzDomObjPtr privdom = NULL;
>  
> -if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid)))
> +if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) {
> +PrlHandle_Free(event);
>  return;
> +}
>  
>  privdom = dom->privateData;
>  PrlHandle_Free(privdom->stats);

ACK

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

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

[libvirt] [PATCH 2/2] vz: fix event handle leak in prlsdkHandlePerfEvent

[Maxim Nestratov]

When we happen to lose a domain but still get a performance event
for it, we should also free the event handle.

Signed-off-by: Maxim Nestratov 
---
 src/vz/vz_sdk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index ec954dd..614dca1 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2197,8 +2197,10 @@ prlsdkHandlePerfEvent(vzDriverPtr driver,
 virDomainObjPtr dom = NULL;
 vzDomObjPtr privdom = NULL;
 
-if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid)))
+if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) {
+PrlHandle_Free(event);
 return;
+}
 
 privdom = dom->privateData;
 PrlHandle_Free(privdom->stats);
-- 
2.4.11

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

[libvirt] [PATCH] vz: cleanup: remove unused constant

[Maxim Nestratov]

PARALLELS_STATISTICS_DROP_COUNT isn't used anymore

Signed-off-by: Maxim Nestratov 
---
 src/vz/vz_sdk.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 614dca1..30eb743 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2187,8 +2187,6 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver,
 return;
 }
 
-#define PARALLELS_STATISTICS_DROP_COUNT 3
-
 static void
 prlsdkHandlePerfEvent(vzDriverPtr driver,
   PRL_HANDLE event,
-- 
2.4.11

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

[libvirt] [PATCH 1/2] vz: fix handle leak in prlsdkHandleVmStateEvent

[Maxim Nestratov]

Every successful call of PrlEvent_GetParamByName allocates a handle,
which has to be freed.

Signed-off-by: Maxim Nestratov 
---
 src/vz/vz_sdk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 727b572..ec954dd 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2114,6 +2114,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver,
 prlsdkSendEvent(driver, dom, lvEventType, lvEventTypeDetails);
 
  cleanup:
+PrlHandle_Free(eventParam);
 virObjectUnlock(dom);
 return;
 }
-- 
2.4.11

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

[libvirt] [PATCH 0/2] vz: fix two handle leaks

[Maxim Nestratov]

Maxim Nestratov (2):
  vz: fix handle leak in prlsdkHandleVmStateEvent
  vz: fix event handle leak in prlsdkHandlePerfEvent

 src/vz/vz_sdk.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.4.11

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

Re: [libvirt] [PATCH] xenconfig: fix xml conversion convert with no graphics

[Jim Fehlig]

Joao Martins wrote:
> If no graphics element is in XML xenFormatXLSpice will access
> graphics without checking it has one in the first place, leading to a
> segmentation fault.
> 
> Signed-off-by: Joao Martins 

ACK. I changed the commit description to "xenconfig: fix xml to xl.cfg
conversion with no graphics" and pushed the patch. Thanks!

Regards,
Jim

> ---
>  src/xenconfig/xen_xl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index 2c9174e..74f68b3 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -1168,7 +1168,7 @@ xenFormatXLSpice(virConfPtr conf, virDomainDefPtr def)
>  virDomainGraphicsListenDefPtr glisten;
>  virDomainGraphicsDefPtr graphics;
>  
> -if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> +if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && def->graphics) {
>  graphics = def->graphics[0];
>  
>  if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {

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

Re: [libvirt] [RFC PATCH 00/10] introduce push backups

[Martin Kletzander]

On Wed, Feb 08, 2017 at 06:11:30PM +0300, Maxim Nestratov wrote:

08-Feb-17 17:52, Martin Kletzander пишет:


On Wed, Jan 18, 2017 at 01:21:48PM +0300, Nikolay Shirokovskiy wrote:

By the way we came to agreement to create distinct API to support backup 
operations in [1]
and there is a discussion of pull backup series in [2]. The latter is blocked
as some necessary operations are still experimental in qemu.

[1] https://www.redhat.com/archives/libvir-list/2016-March/msg00937.html
[2] https://www.redhat.com/archives/libvir-list/2016-September/msg00192.html



I went through the conversations and I feel quite better about
understanding the code, would you mind sending a new version (no matter
whether it's an RFC or not) to the list so it's applicable (and
hopefully will draw more attention)?



Just to have more context. The continuation of the thread [1] went to
https://www.redhat.com/archives/libvir-list/2016-April/msg00401.html



Of course, with the great mailman =)  Thanks for pointing that out ;)


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

Re: [libvirt] [PATCH] qemu_security: Introduce ImageLabel APIs

[Peter Krempa]

On Wed, Feb 08, 2017 at 16:16:42 +0100, Michal Privoznik wrote:
> Just like we need wrappers over other virSecurityManager APIs, we
> need one for virSecurityManagerSetImageLabel and
> virSecurityManagerRestoreImageLabel. Otherwise we might end up
> relabelling device in wrong namespace.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c   |  7 +++---
>  src/qemu/qemu_security.c | 56 
> 
>  src/qemu/qemu_security.h |  8 +++
>  3 files changed, 67 insertions(+), 4 deletions(-)

ACK


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

[libvirt] [PATCH] qemu_security: Introduce ImageLabel APIs

[Michal Privoznik]

Just like we need wrappers over other virSecurityManager APIs, we
need one for virSecurityManagerSetImageLabel and
virSecurityManagerRestoreImageLabel. Otherwise we might end up
relabelling device in wrong namespace.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c   |  7 +++---
 src/qemu/qemu_security.c | 56 
 src/qemu/qemu_security.h |  8 +++
 3 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d3f765605..7c696963e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -31,6 +31,7 @@
 #include "qemu_parse_command.h"
 #include "qemu_capabilities.h"
 #include "qemu_migration.h"
+#include "qemu_security.h"
 #include "viralloc.h"
 #include "virlog.h"
 #include "virerror.h"
@@ -5094,8 +5095,7 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver,
 VIR_WARN("Failed to teardown cgroup for disk path %s",
  NULLSTR(elem->path));
 
-if (virSecurityManagerRestoreImageLabel(driver->securityManager,
-vm->def, elem) < 0)
+if (qemuSecurityRestoreImageLabel(driver, vm, elem) < 0)
 VIR_WARN("Unable to restore security label on %s", 
NULLSTR(elem->path));
 
 if (qemuDomainNamespaceTeardownDisk(driver, vm, elem) < 0)
@@ -5135,8 +5135,7 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver,
 if (qemuSetupImageCgroup(vm, elem) < 0)
 goto cleanup;
 
-if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def,
-elem) < 0)
+if (qemuSecuritySetImageLabel(driver, vm, elem) < 0)
 goto cleanup;
 
 ret = 0;
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index ceac5bf56..f2931976b 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -133,6 +133,62 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
 }
 
 
+int
+qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  virStorageSourcePtr src)
+{
+int ret = -1;
+
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+virSecurityManagerTransactionStart(driver->securityManager) < 0)
+goto cleanup;
+
+if (virSecurityManagerSetImageLabel(driver->securityManager,
+vm->def,
+src) < 0)
+goto cleanup;
+
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+virSecurityManagerTransactionCommit(driver->securityManager,
+vm->pid) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virSecurityManagerTransactionAbort(driver->securityManager);
+return ret;
+}
+
+
+int
+qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  virStorageSourcePtr src)
+{
+int ret = -1;
+
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+virSecurityManagerTransactionStart(driver->securityManager) < 0)
+goto cleanup;
+
+if (virSecurityManagerRestoreImageLabel(driver->securityManager,
+vm->def,
+src) < 0)
+goto cleanup;
+
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
+virSecurityManagerTransactionCommit(driver->securityManager,
+vm->pid) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virSecurityManagerTransactionAbort(driver->securityManager);
+return ret;
+}
+
+
 int
 qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
index cc373b3e1..54638908d 100644
--- a/src/qemu/qemu_security.h
+++ b/src/qemu/qemu_security.h
@@ -45,6 +45,14 @@ int qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  virDomainDiskDefPtr disk);
 
+int qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  virStorageSourcePtr src);
+
+int qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  virStorageSourcePtr src);
+
 int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 virDomainHostdevDefPtr hostdev);
-- 
2.11.0

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

Re: [libvirt] [RFC PATCH 00/10] introduce push backups

[Maxim Nestratov]

08-Feb-17 17:52, Martin Kletzander пишет:


On Wed, Jan 18, 2017 at 01:21:48PM +0300, Nikolay Shirokovskiy wrote:

By the way we came to agreement to create distinct API to support backup 
operations in [1]
and there is a discussion of pull backup series in [2]. The latter is blocked
as some necessary operations are still experimental in qemu.

[1] https://www.redhat.com/archives/libvir-list/2016-March/msg00937.html
[2] https://www.redhat.com/archives/libvir-list/2016-September/msg00192.html



I went through the conversations and I feel quite better about
understanding the code, would you mind sending a new version (no matter
whether it's an RFC or not) to the list so it's applicable (and
hopefully will draw more attention)?



Just to have more context. The continuation of the thread [1] went to
https://www.redhat.com/archives/libvir-list/2016-April/msg00401.html

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

Re: [libvirt] [PATCH 00/11] qemu: Big namespace cleanup

[Michal Privoznik]

On 02/08/2017 11:37 AM, Michal Privoznik wrote:
> Couple of these patches were requested in reviews to my previous namespace
> patches, couple of them are my own idea. At the same time, 3 bugs are fixed:
> 1) qemuSecurity wrappers are preferred over virSecurityManager
> 2) corresponding /dev entry is not created when attaching vhost scsi device
> 3) corresponding /dev entry is not created when doing blockjobs
> 
> Michal Privoznik (11):
>   qemuDomainAttachSCSIVHostDevice: Prefer qemuSecurity wrappers
>   syntax-check: Enforce qemuSecurity
>   qemuDomainAttachSCSIVHostDevice: manage /dev entry
>   qemu_security: Kill code duplication
>   qemu_security: Drop qemuSecuritySetRestoreAllLabelData struct
>   qemu_domain: Don't pass virDomainDeviceDefPtr to ns helpers
>   qemuDomainNamespaceSetupDisk: Drop useless @src variable
>   qemuDomainNamespace{Setup,Teardown}Disk: Don't pass pointer to full
> disk
>   qemuDomainDiskChainElement{Prepare,Revoke}: manage /dev entry
>   qemu_security: Introduce ImageLabel APIs
>   qemuDomainNamespaceSetupDisk: Simplify disk check
> 
>  cfg.mk   |   8 ++
>  src/qemu/qemu_domain.c   |  65 ++--
>  src/qemu/qemu_domain.h   |   4 +-
>  src/qemu/qemu_driver.c   |   2 +-
>  src/qemu/qemu_hotplug.c  |  20 +++--
>  src/qemu/qemu_security.c | 189 
> ---
>  src/qemu/qemu_security.h |   8 ++
>  7 files changed, 102 insertions(+), 194 deletions(-)
> 

FYI, I've pushed the ACKed patches. For a subset of not ACKed patches I
will post v2 in a minute.

Michal

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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Michal Privoznik]

On 02/08/2017 03:55 PM, Martin Kletzander wrote:
> On Wed, Feb 08, 2017 at 03:49:47PM +0100, Peter Krempa wrote:
>> On Wed, Feb 08, 2017 at 15:33:48 +0100, Michal Privoznik wrote:
>>> On 02/08/2017 02:59 PM, Martin Kletzander wrote:
>>> > On Wed, Feb 08, 2017 at 02:37:48PM +0100, Michal Privoznik wrote:
>>> >> On 02/08/2017 01:43 PM, Peter Krempa wrote:
>>> >>> On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
>>>  On 02/08/2017 01:23 PM, Peter Krempa wrote:
>>
>> [...]
>>
>>>
>>> This doesn't solve the syntax-check problem. But whatever, I will go
>>> with this and just drop the syntax-check patch. We have plenty of
>>> arguments for using macros here but since some don't like it I'm not
>>> going to push it. Lets just hope that we will take care to use
>>> qemuSecurity* wrappers instead of calling virSecurity APIs directly.
>>> Honestly, I don't care that much.
>>
>> You can do a macro:
>>
>> #define QEMU_SECURITY_WRAPPED(func) func
>>
>> And then use it in the functions that wrap the function like:
>>
>> int
>> qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> virDomainDiskDefPtr disk)
>> {
>>int ret = -1;
>>
>>if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>>virSecurityManagerTransactionStart(driver->securityManager) < 0)
>>goto cleanup;
>>
>>if
>> (QEMU_SECURITY_WRAPPED(virSecurityManagerRestoreDiskLabel)(driver->securityManager,
>>
>>  vm->def,
>>  disk)
>> < 0)
>>goto cleanup;
>>
>>if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>>virSecurityManagerTransactionCommit(driver->securityManager,
>>vm->pid) < 0)
>>goto cleanup;
>>
>>ret = 0;
>> cleanup:
>>virSecurityManagerTransactionAbort(driver->securityManager);
>>return ret;
>> }
>>
>> But the conditions are to use a very distinctive name of the macro and
>> the macro actually not doing much so that the readability of the code is
>> preserved, while having an anchor point for automatically looking up the
>> function names that were wrapped.
>>
>> Yet another option would be to do:
>>
>> #define QEMU_SECURITY_WRAPPED
>>
>> void QEMU_SECURITY_WRAPPED
>> qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
>>   virDomainObjPtr vm,
>>   bool migrated);
>>
>> In that case you need some 'sed' magic to figure out the function name
>> though.
>>
> 
> Why not just take the wrappers around virSecurity code, put them into
> qemu_security or similar and allow virSecurity* functions only in that
> file?  Clearly that file will only be wrappers, so it should help a lot.

Because so far we don't have a qemuSecurity wrapper for every
virSecurity API. Not sure whether we will need a wrapper for all of them
(e.g. virSecurityManagerSetImageFDLabel)

Michal

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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Martin Kletzander]

On Wed, Feb 08, 2017 at 03:49:47PM +0100, Peter Krempa wrote:

On Wed, Feb 08, 2017 at 15:33:48 +0100, Michal Privoznik wrote:

On 02/08/2017 02:59 PM, Martin Kletzander wrote:
> On Wed, Feb 08, 2017 at 02:37:48PM +0100, Michal Privoznik wrote:
>> On 02/08/2017 01:43 PM, Peter Krempa wrote:
>>> On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
 On 02/08/2017 01:23 PM, Peter Krempa wrote:


[...]



This doesn't solve the syntax-check problem. But whatever, I will go
with this and just drop the syntax-check patch. We have plenty of
arguments for using macros here but since some don't like it I'm not
going to push it. Lets just hope that we will take care to use
qemuSecurity* wrappers instead of calling virSecurity APIs directly.
Honestly, I don't care that much.


You can do a macro:

#define QEMU_SECURITY_WRAPPED(func) func

And then use it in the functions that wrap the function like:

int
qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk)
{
   int ret = -1;

   if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
   virSecurityManagerTransactionStart(driver->securityManager) < 0)
   goto cleanup;

   if 
(QEMU_SECURITY_WRAPPED(virSecurityManagerRestoreDiskLabel)(driver->securityManager,
 vm->def,
 disk) < 0)
   goto cleanup;

   if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
   virSecurityManagerTransactionCommit(driver->securityManager,
   vm->pid) < 0)
   goto cleanup;

   ret = 0;
cleanup:
   virSecurityManagerTransactionAbort(driver->securityManager);
   return ret;
}

But the conditions are to use a very distinctive name of the macro and
the macro actually not doing much so that the readability of the code is
preserved, while having an anchor point for automatically looking up the
function names that were wrapped.

Yet another option would be to do:

#define QEMU_SECURITY_WRAPPED

void QEMU_SECURITY_WRAPPED qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  bool migrated);

In that case you need some 'sed' magic to figure out the function name
though.



Why not just take the wrappers around virSecurity code, put them into
qemu_security or similar and allow virSecurity* functions only in that
file?  Clearly that file will only be wrappers, so it should help a lot.


Peter


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

Re: [libvirt] [RFC PATCH 00/10] introduce push backups

[Martin Kletzander]

On Wed, Jan 18, 2017 at 01:21:48PM +0300, Nikolay Shirokovskiy wrote:

By the way we came to agreement to create distinct API to support backup 
operations in [1]
and there is a discussion of pull backup series in [2]. The latter is blocked
as some necessary operations are still experimental in qemu.

[1] https://www.redhat.com/archives/libvir-list/2016-March/msg00937.html
[2] https://www.redhat.com/archives/libvir-list/2016-September/msg00192.html



I went through the conversations and I feel quite better about
understanding the code, would you mind sending a new version (no matter
whether it's an RFC or not) to the list so it's applicable (and
hopefully will draw more attention)?


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

Re: [libvirt] [PATCH 06/11] qemu_domain: Don't pass virDomainDeviceDefPtr to ns helpers

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:09 +0100, Michal Privoznik wrote:
> There is no need for this. None of the namespace helpers uses it.
> Historically it was used when calling secdriver APIs, but we
> don't to that anymore.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 28 +---
>  1 file changed, 5 insertions(+), 23 deletions(-)

ACK


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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Peter Krempa]

On Wed, Feb 08, 2017 at 15:33:48 +0100, Michal Privoznik wrote:
> On 02/08/2017 02:59 PM, Martin Kletzander wrote:
> > On Wed, Feb 08, 2017 at 02:37:48PM +0100, Michal Privoznik wrote:
> >> On 02/08/2017 01:43 PM, Peter Krempa wrote:
> >>> On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
>  On 02/08/2017 01:23 PM, Peter Krempa wrote:

[...]

> 
> This doesn't solve the syntax-check problem. But whatever, I will go
> with this and just drop the syntax-check patch. We have plenty of
> arguments for using macros here but since some don't like it I'm not
> going to push it. Lets just hope that we will take care to use
> qemuSecurity* wrappers instead of calling virSecurity APIs directly.
> Honestly, I don't care that much.

You can do a macro:

#define QEMU_SECURITY_WRAPPED(func) func

And then use it in the functions that wrap the function like:

int
qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 virDomainDiskDefPtr disk)
{
int ret = -1;

if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
virSecurityManagerTransactionStart(driver->securityManager) < 0)
goto cleanup;

if 
(QEMU_SECURITY_WRAPPED(virSecurityManagerRestoreDiskLabel)(driver->securityManager,
  vm->def,
  disk) < 0)
goto cleanup;

if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
virSecurityManagerTransactionCommit(driver->securityManager,
vm->pid) < 0)
goto cleanup;

ret = 0;
 cleanup:
virSecurityManagerTransactionAbort(driver->securityManager);
return ret;
}

But the conditions are to use a very distinctive name of the macro and
the macro actually not doing much so that the readability of the code is
preserved, while having an anchor point for automatically looking up the
function names that were wrapped.

Yet another option would be to do:

#define QEMU_SECURITY_WRAPPED

void QEMU_SECURITY_WRAPPED qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   bool migrated);

In that case you need some 'sed' magic to figure out the function name
though.

Peter


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

Re: [libvirt] [PATCH 02/11] syntax-check: Enforce qemuSecurity

[Michal Privoznik]

On 02/08/2017 02:32 PM, Peter Krempa wrote:
> On Wed, Feb 08, 2017 at 11:37:05 +0100, Michal Privoznik wrote:
>> Now that we have some qemuSecurity wrappers over
>> virSecurityManager APIs, lets make sure everybody sticks with
>> them. We have them for a reason and calling virSecurityManager
>> API directly instead of wrapper may lead into accidentally
>> labelling a file on the host instead of namespace.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  cfg.mk | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/cfg.mk b/cfg.mk
>> index 69e3f3a1a..6fb2fc961 100644
>> --- a/cfg.mk
>> +++ b/cfg.mk
>> @@ -585,6 +585,14 @@ sc_prohibit_unsigned_pid:
>>  halt='use signed type for pid values'   \
>>$(_sc_search_regexp)
>>  
>> +sc_prohibit_direct_secdriver:
>> +@for i in $$(grep -i ^WRAP.\( src/qemu/qemu_security.c |
>> \
>> +awk 'BEGIN {FS = "[^[:alnum:]]"} {print "virSecurityManager" $$2 }'); 
>> do\
>> +  grep -n $$i $$($(VC_LIST_EXCEPT) | grep -E '^src/qemu/') && \
>> +  { echo "$(ME): prefer qemuSecurity$${i#virSecurityManager} over $$i" 
>> 1>&2; exit 1; }  \
>> +done || :
> 
> This won't work without the "WRAP" stuff so you'll need to come up with
> something else.
> 

Without WRAP it's going to be super tricky as I'd have try to match
functions from qemu_security.h with those from security_manager.h. If
you have some bright idea, please do share it, because frankly I'm out
of them.

Michal

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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Michal Privoznik]

On 02/08/2017 02:59 PM, Martin Kletzander wrote:
> On Wed, Feb 08, 2017 at 02:37:48PM +0100, Michal Privoznik wrote:
>> On 02/08/2017 01:43 PM, Peter Krempa wrote:
>>> On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
 On 02/08/2017 01:23 PM, Peter Krempa wrote:
> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
>> Nearly all of these functions look the same. Except for a
>> different virSecurityManager API call. There is no need to copy
>> paste the code when we can use macros to generate it.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_security.c | 179
>> ---
>>  1 file changed, 44 insertions(+), 135 deletions(-)
>
> NACK, please don't partialy define function with macros.
>

 Why not? What is the downside?
>>>
> 
> I agree that macros that do a partial construct (start or end of a
> function) are really not readable.  Not talking about the navigation.
> 
>>> You'll never be able to navigate to the body of the function or ever
>>> find it try 'vim -t qemuSecurityRestoreHostdevLabel' or navigate to
>>> that after that patch.
>>
>> I don't think this is ultimate goal. A lot of our code is written in a
>> callback style: var->cb(blaah). You cannot jump to the actual function
>> either. If doing 'vim -t' is the ultimate goal then we should ban
>> callbacks too.
>>
>>>
>>> The downside of the code being totally unreadable is way worse than a
>>> few copied lines.
>>
>> Well, I was asked in other review to not copy the lines.
>> Also, the upside is that we can have a syntax-check rule that checks if
>> qemuSecurity wrapper is used instead of calling bare virSecurity...
>>
>> So what about:
>>
>> int
>> qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, ...)
>> {
>>   WRAP(RestoreHostdevLabel); /* macro that wraps
>> virSecurityManagerRestoreHostdevLabel */
>> }
>>
>> This way you can 'vim -t' into it. Although, the syntax-check rule is
>> going to be much more complex.
>>
> 
> How about simply:
> 
> int
> qemuNamespaceMountTransactionStart(virDomainObjPtr vm,
>   virSecurityManagerPtr secMgr)
> {
>if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>virSecurityManagerTransactionStart(secMgr) < 0)
>return -1;
> 
>return 0;
> }
> 
> int
> qemuNamespaceMountTransactionCommit(virDomainObjPtr vm,
>virSecurityManagerPtr secMgr)
> {
>if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>virSecurityManagerTransactionCommit(secMgr, vm->pid) < 0)
>return -1;
> 
>return 0;
> }
> 
> void
> qemuNamespaceMountTransactionAbort(virSecurityManagerPtr secMgr)
> {
>virSecurityManagerTransactionAbort(secMgr);
> }

This doesn't solve the syntax-check problem. But whatever, I will go
with this and just drop the syntax-check patch. We have plenty of
arguments for using macros here but since some don't like it I'm not
going to push it. Lets just hope that we will take care to use
qemuSecurity* wrappers instead of calling virSecurity APIs directly.
Honestly, I don't care that much.

Michal

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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Peter Krempa]

On Wed, Feb 08, 2017 at 14:59:13 +0100, Martin Kletzander wrote:
> On Wed, Feb 08, 2017 at 02:37:48PM +0100, Michal Privoznik wrote:
> > On 02/08/2017 01:43 PM, Peter Krempa wrote:

[...]

> 
> How about simply:
> 
> int
> qemuNamespaceMountTransactionStart(virDomainObjPtr vm,
>   virSecurityManagerPtr secMgr)
> {
>if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>virSecurityManagerTransactionStart(secMgr) < 0)
>return -1;
> 
>return 0;
> }
> 
> int
> qemuNamespaceMountTransactionCommit(virDomainObjPtr vm,
>virSecurityManagerPtr secMgr)
> {
>if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>virSecurityManagerTransactionCommit(secMgr, vm->pid) < 0)
>return -1;
> 
>return 0;
> }
> 
> void
> qemuNamespaceMountTransactionAbort(virSecurityManagerPtr secMgr)
> {
>virSecurityManagerTransactionAbort(secMgr);
> }
> 
> or anything similar with the naming being done by someone else than me.
> 
> You can also extract the securityManager out of the vm pointer if you
> want to make it even shorter.

Yes, this is exactly what I wanted to say in the parallel reply :)


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

Re: [libvirt] [PATCH v4] bhyve: add e1000 nic support

[Roman Bogorodskiy]

  Roman Bogorodskiy wrote:

> Recently e1000 NIC support was added to bhyve; implement that in
> the bhyve driver:
> 
>  - Add capability check by analyzing output of the 'bhyve -s 0,e1000'
>command
>  - Modify bhyveBuildNetArgStr() to support e1000 and also pass
>virConnectPtr so it could call bhyveDriverGetCaps() to check if this
>NIC is supported
>  - Modify command parsing code to add support for e1000 and adjust tests
>  - Add net-e1000 test
> ---
>  src/bhyve/bhyve_capabilities.c | 26 ++
>  src/bhyve/bhyve_capabilities.h |  1 +
>  src/bhyve/bhyve_command.c  | 31 
> +++---
>  src/bhyve/bhyve_parse_command.c| 10 ++-
>  tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args   |  8 ++
>  tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml| 30 +
>  .../bhyveargv2xmldata/bhyveargv2xml-virtio-net.xml |  2 ++
>  .../bhyveargv2xml-virtio-net4.xml  |  1 +
>  tests/bhyveargv2xmltest.c  |  1 +
>  .../bhyvexml2argvdata/bhyvexml2argv-net-e1000.args |  9 +++
>  .../bhyvexml2argv-net-e1000.ldargs |  3 +++
>  .../bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml  | 22 +++
>  tests/bhyvexml2argvtest.c  |  7 -
>  13 files changed, 145 insertions(+), 6 deletions(-)
>  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-e1000.args
>  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-e1000.xml
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.args
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.ldargs
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-net-e1000.xml

ping?

Roman Bogorodskiy


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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Martin Kletzander]

On Wed, Feb 08, 2017 at 02:37:48PM +0100, Michal Privoznik wrote:

On 02/08/2017 01:43 PM, Peter Krempa wrote:

On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:

On 02/08/2017 01:23 PM, Peter Krempa wrote:

On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:

Nearly all of these functions look the same. Except for a
different virSecurityManager API call. There is no need to copy
paste the code when we can use macros to generate it.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_security.c | 179 ---
 1 file changed, 44 insertions(+), 135 deletions(-)


NACK, please don't partialy define function with macros.



Why not? What is the downside?




I agree that macros that do a partial construct (start or end of a
function) are really not readable.  Not talking about the navigation.


You'll never be able to navigate to the body of the function or ever
find it try 'vim -t qemuSecurityRestoreHostdevLabel' or navigate to
that after that patch.


I don't think this is ultimate goal. A lot of our code is written in a
callback style: var->cb(blaah). You cannot jump to the actual function
either. If doing 'vim -t' is the ultimate goal then we should ban
callbacks too.



The downside of the code being totally unreadable is way worse than a
few copied lines.


Well, I was asked in other review to not copy the lines.
Also, the upside is that we can have a syntax-check rule that checks if
qemuSecurity wrapper is used instead of calling bare virSecurity...

So what about:

int
qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, ...)
{
  WRAP(RestoreHostdevLabel); /* macro that wraps
virSecurityManagerRestoreHostdevLabel */
}

This way you can 'vim -t' into it. Although, the syntax-check rule is
going to be much more complex.



How about simply:

int
qemuNamespaceMountTransactionStart(virDomainObjPtr vm,
  virSecurityManagerPtr secMgr)
{
   if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
   virSecurityManagerTransactionStart(secMgr) < 0)
   return -1;

   return 0;
}

int
qemuNamespaceMountTransactionCommit(virDomainObjPtr vm,
   virSecurityManagerPtr secMgr)
{
   if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
   virSecurityManagerTransactionCommit(secMgr, vm->pid) < 0)
   return -1;

   return 0;
}

void
qemuNamespaceMountTransactionAbort(virSecurityManagerPtr secMgr)
{
   virSecurityManagerTransactionAbort(secMgr);
}

or anything similar with the naming being done by someone else than me.

You can also extract the securityManager out of the vm pointer if you
want to make it even shorter.


Michal

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

Re: [libvirt] [PATCH 03/11] qemuDomainAttachSCSIVHostDevice: manage /dev entry

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:06 +0100, Michal Privoznik wrote:
> Again, one missed bit. This time without this commit there is no
> /dev entry when attaching vhost SCSI device.

Commit message is a bit confusing. So the /dev/ entry in the namespace
of the qemu process is not created when attaching scsi disks.

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 8 
>  1 file changed, 8 insertions(+)

ACK


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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Peter Krempa]

On Wed, Feb 08, 2017 at 14:37:48 +0100, Michal Privoznik wrote:
> On 02/08/2017 01:43 PM, Peter Krempa wrote:
> > On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
> >> On 02/08/2017 01:23 PM, Peter Krempa wrote:
> >>> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
>  Nearly all of these functions look the same. Except for a
>  different virSecurityManager API call. There is no need to copy
>  paste the code when we can use macros to generate it.
> 
>  Signed-off-by: Michal Privoznik 
>  ---
>   src/qemu/qemu_security.c | 179 
>  ---
>   1 file changed, 44 insertions(+), 135 deletions(-)
> >>>
> >>> NACK, please don't partialy define function with macros.
> >>>
> >>
> >> Why not? What is the downside?
> > 
> > You'll never be able to navigate to the body of the function or ever
> > find it try 'vim -t qemuSecurityRestoreHostdevLabel' or navigate to
> > that after that patch.
> 
> I don't think this is ultimate goal. A lot of our code is written in a
> callback style: var->cb(blaah). You cannot jump to the actual function
> either. If doing 'vim -t' is the ultimate goal then we should ban
> callbacks too.

Callbacks are way different from this case. Even if you have a callback
then a debugger prints the function name. The same applies for logs.

With a macro that defines a function you get a function name that is not
present in the source without pre-processing it. With the callback you
still have the symbol, while the call path may be opaque.

> 
> > 
> > The downside of the code being totally unreadable is way worse than a
> > few copied lines.
> 
> Well, I was asked in other review to not copy the lines.
> Also, the upside is that we can have a syntax-check rule that checks if
> qemuSecurity wrapper is used instead of calling bare virSecurity...

I don't think that the macros are a requirement to have a syntax check.

> 
> So what about:
> 
> int
> qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, ...)
> {
>WRAP(RestoreHostdevLabel); /* macro that wraps
> virSecurityManagerRestoreHostdevLabel */

I'd extract the "PROLOGUE" and "EPILOGUE" parts as a function and then
just call the wrapped function directly. I don't see a point to have a
macro here.

> }
> 
> This way you can 'vim -t' into it. Although, the syntax-check rule is
> going to be much more complex.

Just wrap everything and outlaw it outside of this code then.


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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Michal Privoznik]

On 02/08/2017 01:43 PM, Peter Krempa wrote:
> On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
>> On 02/08/2017 01:23 PM, Peter Krempa wrote:
>>> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
 Nearly all of these functions look the same. Except for a
 different virSecurityManager API call. There is no need to copy
 paste the code when we can use macros to generate it.

 Signed-off-by: Michal Privoznik 
 ---
  src/qemu/qemu_security.c | 179 
 ---
  1 file changed, 44 insertions(+), 135 deletions(-)
>>>
>>> NACK, please don't partialy define function with macros.
>>>
>>
>> Why not? What is the downside?
> 
> You'll never be able to navigate to the body of the function or ever
> find it try 'vim -t qemuSecurityRestoreHostdevLabel' or navigate to
> that after that patch.

I don't think this is ultimate goal. A lot of our code is written in a
callback style: var->cb(blaah). You cannot jump to the actual function
either. If doing 'vim -t' is the ultimate goal then we should ban
callbacks too.

> 
> The downside of the code being totally unreadable is way worse than a
> few copied lines.

Well, I was asked in other review to not copy the lines.
Also, the upside is that we can have a syntax-check rule that checks if
qemuSecurity wrapper is used instead of calling bare virSecurity...

So what about:

int
qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, ...)
{
   WRAP(RestoreHostdevLabel); /* macro that wraps
virSecurityManagerRestoreHostdevLabel */
}

This way you can 'vim -t' into it. Although, the syntax-check rule is
going to be much more complex.

Michal

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

Re: [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting

[Joao Martins]

On 02/02/2017 10:31 PM, Jim Fehlig wrote:
> When the libxl driver is initialized, it creates a virDomainDef
> object for dom0 and adds it to the list of domains. Total memory
> for dom0 was being set from the max_memkb field of libxl_dominfo
> struct retrieved from libxl, but this field can be set to
> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
> explicitly set by the user.
> 
> This patch adds some simple parsing of the Xen commandline,
> looking for a dom0_mem parameter that also specifies a 'max' value.
> If not specified, dom0 maximum memory is effectively all physical
> host memory.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_conf.c   | 75 
> 
>  src/libxl/libxl_conf.h   |  3 ++
>  src/libxl/libxl_driver.c |  2 +-
>  3 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b5186f2..bfe0e92 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -34,6 +34,7 @@
>  #include "internal.h"
>  #include "virlog.h"
>  #include "virerror.h"
> +#include "c-ctype.h"
>  #include "datatypes.h"
>  #include "virconf.h"
>  #include "virfile.h"
> @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>  
>  }
>  
> +/*
> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' Xen
> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
> + * memory to 8G: dom0_mem=4G,max:8G
> + *
> + * If not constrained by the user, dom0 can effectively use all host memory.
> + * This function returns the configured maximum memory for dom0 in kilobytes,
> + * either the user-specified value or total physical memory as a default.
> + */
> +unsigned long long
> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
> +{
> +char **cmd_tokens = NULL;
> +char **mem_tokens = NULL;
> +size_t i;
> +size_t j;
> +unsigned long long ret;
> +libxl_physinfo physinfo;
> +
> +if (cfg->verInfo->commandline == NULL ||
> +!(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
> +goto physmem;
> +
> +for (i = 0; cmd_tokens[i] != NULL; i++) {
> +if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
> +continue;
> +
> +if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
> +break;
> +for (j = 0; mem_tokens[j] != NULL; j++) {
> +if (STRPREFIX(mem_tokens[j], "max:")) {
> +char *p = mem_tokens[j] + 4;
> +unsigned long long multiplier = 1;
> +
> +while (c_isdigit(*p))
> +p++;
> +if (virStrToLong_ull(mem_tokens[j] + 4, , 10, ) < 0)
> +break;
> +if (*p) {
> +switch (*p) {
> +case 'k':
> +case 'K':
> +multiplier = 1024;
> +break;
> +case 'm':
> +case 'M':
> +multiplier = 1024 * 1024;
> +break;
> +case 'g':
> +case 'G':
> +multiplier = 1024 * 1024 * 1024;
> +break;
> +}
> +}
> +ret = (ret * multiplier) / 1024;
> +goto cleanup;
> +}
> +}
> +}
> +
> + physmem:
> +/* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
> +libxl_physinfo_init();
> +libxl_get_physinfo(cfg->ctx, );
Despite being an unlikely event, libxl_get_physinfo can fail here - I think you
need to check the return value here.

> +ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
> +libxl_physinfo_dispose();
> +
> + cleanup:
> +virStringListFree(cmd_tokens);
> +virStringListFree(mem_tokens);
> +return ret;
> +}
> +
> +
>  #ifdef LIBXL_HAVE_DEVICE_CHANNEL
>  static int
>  libxlPrepareChannel(virDomainChrDefPtr channel,
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 69d7885..c4ddbfe 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -173,6 +173,9 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver,
>  int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>const char *filename);
>  
> +unsigned long long
> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg);
> +
>  int
>  libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
>  int
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 921cc93..e54b3b7 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -615,7 +615,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
>  if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
>  goto cleanup;
>  vm->def->mem.cur_balloon = 

Re: [libvirt] [PATCH 02/11] syntax-check: Enforce qemuSecurity

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:05 +0100, Michal Privoznik wrote:
> Now that we have some qemuSecurity wrappers over
> virSecurityManager APIs, lets make sure everybody sticks with
> them. We have them for a reason and calling virSecurityManager
> API directly instead of wrapper may lead into accidentally
> labelling a file on the host instead of namespace.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  cfg.mk | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 69e3f3a1a..6fb2fc961 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -585,6 +585,14 @@ sc_prohibit_unsigned_pid:
>   halt='use signed type for pid values'   \
> $(_sc_search_regexp)
>  
> +sc_prohibit_direct_secdriver:
> + @for i in $$(grep -i ^WRAP.\( src/qemu/qemu_security.c |
> \
> + awk 'BEGIN {FS = "[^[:alnum:]]"} {print "virSecurityManager" $$2 }'); 
> do\
> +   grep -n $$i $$($(VC_LIST_EXCEPT) | grep -E '^src/qemu/') && \
> +   { echo "$(ME): prefer qemuSecurity$${i#virSecurityManager} over $$i" 
> 1>&2; exit 1; }  \
> +done || :

This won't work without the "WRAP" stuff so you'll need to come up with
something else.


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

Re: [libvirt] [PATCH 07/11] qemuDomainNamespaceSetupDisk: Drop useless @src variable

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:10 +0100, Michal Privoznik wrote:
> Since its introduction in 81df21507bef9 this variable was never
> used.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

ACK


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

Re: [libvirt] [PATCH 11/11] qemuDomainNamespaceSetupDisk: Simplify disk check

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:14 +0100, Michal Privoznik wrote:
> Firstly, instead of checking for next->path the
> virStorageSourceIsEmpty() function should be used which also
> takes disk type into account.
> Secondly, not every disk source passed has the correct type set
> (due to our laziness). Therefore, instead of checking for
> virStorageSourceIsBlockLocal() and also S_ISBLK() the former can
> be refined to just virStorageSourceIsLocalStorage().
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)

ACK


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

Re: [libvirt] [PATCH 10/11] qemu_security: Introduce ImageLabel APIs

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:13 +0100, Michal Privoznik wrote:
> Just like we need wrappers over other virSecurityManager APIs, we
> need one for virSecurityManagerSetImageLabel and
> virSecurityManagerRestoreImageLabel.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c   | 7 +++
>  src/qemu/qemu_security.c | 3 +++
>  src/qemu/qemu_security.h | 8 
>  3 files changed, 14 insertions(+), 4 deletions(-)
>

[...]

> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index 06bff2470..131be6e4b 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -95,5 +95,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
>  WRAP1(SetDiskLabel, virDomainDiskDefPtr)
>  WRAP1(RestoreDiskLabel, virDomainDiskDefPtr)
>  
> +WRAP1(SetImageLabel, virStorageSourcePtr)
> +WRAP1(RestoreImageLabel, virStorageSourcePtr)
> +
>  WRAP2(SetHostdevLabel, virDomainHostdevDefPtr)
>  WRAP2(RestoreHostdevLabel, virDomainHostdevDefPtr)

Obviously NACK to this.


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

Re: [libvirt] [PATCH 09/11] qemuDomainDiskChainElement{Prepare, Revoke}: manage /dev entry

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:12 +0100, Michal Privoznik wrote:
> Again, one missed bit. This time without this commit there is no
> /dev entry when doing disk snapshots.

Snapshots and block-copy. Both of them start using a new image file.

> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 

ACK


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

Re: [libvirt] [PATCH 08/11] qemuDomainNamespace{Setup, Teardown}Disk: Don't pass pointer to full disk

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:11 +0100, Michal Privoznik wrote:
> These functions do not need to see the whole virDomainDiskDef.
> Moreover, they are going to be called from places where we don't
> have access to the full disk definition. Sticking with
> virStorageSource is more than enough.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c  | 8 
>  src/qemu/qemu_domain.h  | 4 ++--
>  src/qemu/qemu_driver.c  | 2 +-
>  src/qemu/qemu_hotplug.c | 6 +++---
>  4 files changed, 10 insertions(+), 10 deletions(-)

ACK


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/2] libxl: use init and dispose functions with libxl_physinfo

[Joao Martins]

On 02/02/2017 10:39 PM, Jim Fehlig wrote:
> The typical pattern when calling libxl functions that populate a
> structure is
> 
>   libxl_foo foo;
>   libxl_foo_init();
>   libxl_get_foo(ctx, );
>   ...
>   libxl_foo_dispose();
> 
> Fix several instances of libxl_physinfo missing the init and
> dispose calls.
Indeed,

> 
> Signed-off-by: Jim Fehlig 

Reviewed-by: Joao Martins 

See also one comment/nit below, perhaps one libxl_physinfo_init could be moved
slightly up..

> [...]

> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 3a69720..8951bef 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -4286,6 +4286,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn)
>  if (virNodeGetFreeMemoryEnsureACL(conn) < 0)
>  goto cleanup;
>  
> +libxl_physinfo_init(_info);

.. namely here? That is before virNodeGetFreeMemoryEnsureACL.

Not that it matters much, as init/dispose in this case just zeroes out phy_info
region. But just perhaps consistency as you would end disposing an non
initialized object.

>  if (libxl_get_physinfo(cfg->ctx, _info)) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("libxl_get_physinfo_info failed"));
> @@ -4295,6 +4296,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn)
>  ret = phy_info.free_pages * cfg->verInfo->pagesize;
>  
>   cleanup:
> +libxl_physinfo_dispose(_info);
>  virObjectUnref(cfg);
>  return ret;
>  }
> 

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

Re: [libvirt] [PATCH 05/11] qemu_security: Drop qemuSecuritySetRestoreAllLabelData struct

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:08 +0100, Michal Privoznik wrote:
> This struct is unused after 095f042ed68b01.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_security.c | 9 -
>  1 file changed, 9 deletions(-)

ACK


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

Re: [libvirt] [PATCH 1/2] libxl: honor autoballoon setting in libxl.conf

[Joao Martins]

On 02/02/2017 10:39 PM, Jim Fehlig wrote:
> libxlGetAutoballoonConf is supposed to honor user-specified
> autoballoon setting in libxl.conf. As written, the user-specified
> setting could be overwritten by the subsequent logic to check
> dom0_mem parameter. If user-specified setting is present and
> correct, accept it. Only fallback to checking Xen dom0_mem
> command line parameter if user-specfied setting is not present.
> 
> Signed-off-by: Jim Fehlig 
Nice catch!

Reviewed-by: Joao Martins 

> ---
>  src/libxl/libxl_conf.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b5186f2..ee1e639 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1355,8 +1355,11 @@ libxlGetAutoballoonConf(libxlDriverConfigPtr cfg,
>  regex_t regex;
>  int res;
>  
> -if (virConfGetValueBool(conf, "autoballoon", >autoballoon) < 0)
> +res = virConfGetValueBool(conf, "autoballoon", >autoballoon);
> +if (res < 0)
>  return -1;
> +else if (res == 1)
> +return 0;
>  
>  if ((res = regcomp(,
>"(^| )dom0_mem=((|min:|max:)[0-9]+[bBkKmMgG]?,?)+($| 
> )",
> 

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

Re: [libvirt] [resend v2 0/7] Support cache tune in libvirt

[Marcelo Tosatti]

On Wed, Feb 08, 2017 at 10:19:07AM +0800, Eli Qiao wrote:
> 
> 
> --  
> Eli Qiao
> Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> 
> 
> On Tuesday, 7 February 2017 at 7:56 PM, Marcelo Tosatti wrote:
> 
> > On Tue, Feb 07, 2017 at 02:43:13PM +0800, Eli Qiao wrote:
> > >  
> > >  
> > > --  
> > > Eli Qiao
> > > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> > >  
> > >  
> > > On Tuesday, 7 February 2017 at 3:03 AM, Marcelo Tosatti wrote:
> > >  
> > > > On Mon, Feb 06, 2017 at 01:33:09PM -0200, Marcelo Tosatti wrote:
> > > > > On Mon, Feb 06, 2017 at 10:23:35AM +0800, Eli Qiao wrote:
> > > > > > This series patches are for supportting CAT featues, which also
> > > > > > called cache tune in libvirt.
> > > > > >  
> > > > > > First to expose cache information which could be tuned in 
> > > > > > capabilites XML.
> > > > > > Then add new domain xml element support to add cacahe bank which 
> > > > > > will apply
> > > > > > on this libvirt domain.
> > > > > >  
> > > > > > This series patches add a util file `resctrl.c/h`, an interface to 
> > > > > > talk with
> > > > > > linux kernel's sys fs.
> > > > > >  
> > > > > > There are still some TODOs such as expose new public interface to 
> > > > > > get free
> > > > > > cache information.
> > > > > >  
> > > > > > Some discussion about this feature support can be found from:
> > > > > > https://www.redhat.com/archives/libvir-list/2017-January/msg00644.html
> > > > > >  
> > > > > >  
> > > > >  
> > > > >  
> > > > >  
> > > > > Two comments:
> > > > >  
> > > > > 1) Please perform appropriate filesystem locking when accessing
> > > > > resctrlfs, as described at:
> > > > >  
> > > > > http://www.spinics.net/lists/linux-tip-commits/msg36754.html
> > > > >  
> > > >  
> > > >  
> > >  
> > >  
> > > Sure.  
> > > > >  
> > > > > 2)  
> > > > >  
> > > > > 
> > > > >  
> > > > > [b4c270b5-e0f9-4106-a446-69032872ed7d]# cat tasks  
> > > > > 8654
> > > > > [b4c270b5-e0f9-4106-a446-69032872ed7d]# pstree -p | grep qemu
> > > > > |-qemu-kvm(8654)-+-{qemu-kvm}(8688)
> > > > > | |-{qemu-kvm}(8692)
> > > > > | `-{qemu-kvm}(8693)
> > > > >  
> > > > > Should add individual vcpus to the "tasks" file, not the main QEMU
> > > > > process.
> > > > >  
> > > > > The NFV usecase requires exclusive CAT allocation for the vcpu which
> > > > > runs the sensitive workload.
> > > > >  
> > > > > Perhaps:
> > > > >  
> > > > >   
> > > > >  
> > > > > Adds all vcpus that are pinned to the socket which cachebank with
> > > > > host_id=1.
> > > > >  
> > > > >  > > > > vcpus=2,3/>  
> > > > >  
> > > > > Adds PID of vcpus 2 and 3 to resctrl directory created for this
> > > > > allocation.
> > > > >  
> > > > >  
> > > >  
> > > >  
> > > >  
> > > >  
> > >  
> > > Hmm.. in this case, we need to figure out what’s the pid of vcpus=2 and 
> > > vcpu=3 and added them to the resctrl directory.
> > >  
> >  
> >  
> > Yes and only the pids of vcpus=2 and vcpus=3, not of any other vcpus.
> >  
> > > currently, I create a resctrl directory(called resctrl domain) for a VM 
> > > so just put all task ids to it.
> > >  
> > > this is my though:
> > >  
> > > let say the vm has vcpus=0 1 2 3, and you want to let 0, 1 benefit cache 
> > > on host_id=0, and 2, 3 on host_id=1
> > >  
> > > you will do:
> > >  
> > > 1)
> > > pin vcpus 0, 1 on the cpus of socket 0  
> > > pin vcpus 2, 3 on the cpus of socket 1
> > > this can be done in vcputune
> > >  
> > > 2) define cache tune like this:
> > > 
> > > 
> > >  
> > > in libvirt:
> > > we create a resctrl directory naming with the VM’s uuid
> > > and set schemata for each socket 0, and socket 1, put all qemu tasks ids 
> > > into tasks file, this will work fine.  
> > >  
> >  
> >  
> > No, please don't do this.
> >  
> > > please be note that in a resctrl directory, we can define schemata for 
> > > each socket id separately.
> >  
> > Please do not put vcpus automatically into the reservations.
> > Its necessary to have certain vcpus in a reservation and some not.
> >  
> > For example: 2 vcpu guest, vcpu0 pinned to socket 0 cpu0,  
> > vcpu1 pinned to socket 0 cpu1.
> >  
> >  
> > 
> >  
> > We want _only_ vcpu1 to be part of this reservation, and not vcpu0  
> > (want vcpu0 to use the default group, ie. schemata file at  
> >  
> > /sys/fs/resctrl/schemata
> >  
> > So please have the ability to add vcpus to the XML syntax:
> >  
> >  > vcpus='1'/>
> >  
> > or
> >  
> >  > vcpus='2,3'/>
> >  
> Yep, get your mean, make sense.
> 
> if there’s exist method can get vcpus’ pid?  

qemu_process.c:pid_t vcpupid = qemuDomainGetVcpuPid(vm, vcpuid);

> > This also allows different sizes to be specified.
> >  
> >  
> > > > 3) CDP / non-CDP convertion.
> > > >  
> > > > In case the size determination has been performed with non-CDP,
> > > > to emulate such allocation on a CDP host,
> > > > it would be good to allow both code and data allocations to share
> > > > the CBM space:  
> > > >  
> > > >  
> > >  
> > > IOM, I don’t 

Re: [libvirt] [PATCH 01/11] qemuDomainAttachSCSIVHostDevice: Prefer qemuSecurity wrappers

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:04 +0100, Michal Privoznik wrote:
> Since we have qemuSecurity wrappers over
> virSecurityManagerSetHostdevLabel and
> virSecurityManagerRestoreHostdevLabel we ought to use them
> instead of calling secdriver APIs directly.

Also it possibly would be worth mentioning that without those wrappers
the labelling won't be done in the correct namespace and thus won't
apply to the nodes seen by qemu itself.

I presume that that bug actually motivated you do do so.

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index e272df356..dd6e31823 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2552,8 +2552,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
>  goto cleanup;
>  teardowncgroup = true;
>  
> -if (virSecurityManagerSetHostdevLabel(driver->securityManager,
> -  vm->def, hostdev, NULL) < 0)
> +if (qemuSecuritySetHostdevLabel(driver, vm, hostdev) < 0)
>  goto cleanup;
>  teardownlabel = true;
>  
> @@ -2612,8 +2611,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
>  if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0)
>  VIR_WARN("Unable to remove host device cgroup ACL on hotplug 
> fail");
>  if (teardownlabel &&
> -virSecurityManagerRestoreHostdevLabel(driver->securityManager,
> -  vm->def, hostdev, NULL) < 
> 0)
> +qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0)
>  VIR_WARN("Unable to restore host device labelling on hotplug 
> fail");
>  if (releaseaddr)
>  qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);

ACK with commit message fixed.


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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Peter Krempa]

On Wed, Feb 08, 2017 at 13:37:48 +0100, Michal Privoznik wrote:
> On 02/08/2017 01:23 PM, Peter Krempa wrote:
> > On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
> >> Nearly all of these functions look the same. Except for a
> >> different virSecurityManager API call. There is no need to copy
> >> paste the code when we can use macros to generate it.
> >>
> >> Signed-off-by: Michal Privoznik 
> >> ---
> >>  src/qemu/qemu_security.c | 179 
> >> ---
> >>  1 file changed, 44 insertions(+), 135 deletions(-)
> > 
> > NACK, please don't partialy define function with macros.
> > 
> 
> Why not? What is the downside?

You'll never be able to navigate to the body of the function or ever
find it try 'vim -t qemuSecurityRestoreHostdevLabel' or navigate to
that after that patch.

The downside of the code being totally unreadable is way worse than a
few copied lines.

(YU|NA)CK



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

Re: [libvirt] [PATCH] conf: fix use-after-free when sending event message

[Peter Krempa]

On Wed, Feb 08, 2017 at 20:33:10 +0800, Wang King wrote:
> 
> If there is a process with a client which registers event callbacks,
> and it calls libvirt's API which uses the same virConnectPtr in that
> callback function. When this process exit abnormally lead to client
> disconnect, there is a possibility that the main thread is refer to
> virServerClient just after the virServerClient been freed by job
> thread of libvirtd.
> 
> Following is the backtrace:
> #0 0x7fda223d66d8 in virClassIsDerivedFrom
> (klass=0xdeadbeef,parent=0x7fda24c81b40)
> #1 0x7fda223d6a1e in virObjectIsClass
> (anyobj=anyobj@entry=0x7fd9e575b400,klass=)
> #2 0x7fda223d6a44 in virObjectLock (anyobj=anyobj@entry=0x7fd9e575b400)
> #3 0x7fda22507f71 in virNetServerClientSendMessage
> (client=client@entry=0x7fd9e575b400, msg=msg@entry=0x7fd9ec30de90)
> #4 0x7fda230d714d in remoteDispatchObjectEventSend
> (client=0x7fd9e575b400, program=0x7fda24c844e0, procnr=procnr@entry=348,
> proc=0x7fda2310e5e0 , data=data@entry=0x7ffc3857fdb0)
> #5 0x7fda230dd71b in remoteRelayDomainEventTunable (conn=,
> dom=0x7fda27cd7660, params=0x7fda27f3aae0, nparams=1, opaque=0x7fd9e6c99e00)
> #6 0x7fda224484cb in virDomainEventDispatchDefaultFunc
> (conn=0x7fda27cd0120, event=0x7fda2736ea00, cb=0x7fda230dd610 ,
> cbopaque=0x7fd9e6c99e00)
> #7 0x7fda22446871 in virObjectEventStateDispatchCallbacks (callbacks=,
> callbacks=, event=0x7fda2736ea00, state=0x7fda24ca3960)
> #8 virObjectEventStateQueueDispatch (callbacks=0x7fda24c65800,
> queue=0x7ffc3857fe90, state=0x7fda24ca3960)
> #9 virObjectEventStateFlush (state=0x7fda24ca3960)
> #10 virObjectEventTimer (timer=, opaque=0x7fda24ca3960)
> #11 0x7fda223ae8b9 in virEventPollDispatchTimeouts ()
> #12 virEventPollRunOnce ()
> #13 0x7fda223ad1d2 in virEventRunDefaultImpl ()
> #14 0x7fda225046cd in virNetDaemonRun (dmn=dmn@entry=0x7fda24c775c0)
> #15 0x7fda230d6351 in main (argc=, argv=)
> 
> (gdb) p *(virNetServerClientPtr)0x7fd9e575b400
> $2 = {parent = {parent = {u = {dummy_align1 = 140573849338048, dummy_align2
> = 0x7fd9e65ac0c0, s = {magic = 3864707264, refs = 32729}}, klass =
> 0x7fda0078}, lock = {lock = {__data = {__lock = 0,
> __count = 0, __owner = 0, __nusers = 0, __kind = 0, __spins = 0, __list =
> {__prev = 0x0, __next = 0x0}}, __size = '\000' , __align = 0}}}, wantClose =
> false,
> delayedClose = false, sock = 0x0, auth = 0, readonly = false, tlsCtxt = 0x0,
> tls = 0x0, sasl = 0x0, sockTimer = 0, identity = 0x0, nrequests = 0,
> nrequests_max = 0, rx = 0x0, tx = 0x0, filters = 0x0,
> nextFilterID = 0, dispatchFunc = 0x0, dispatchOpaque = 0x0, privateData =
> 0x0, privateDataFreeFunc = 0x0, privateDataPreExecRestart = 0x0,
> privateDataCloseFunc = 0x0, keepalive = 0x0}
> ---
> src/rpc/virnetserverclient.c | 6 ++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 81da82c..562516f 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -1021,6 +1021,12 @@ void virNetServerClientClose(virNetServerClientPtr
> client)
> client->sock = NULL;
> }
> 
> + if (client->privateData &&
> + client->privateDataFreeFunc) {
> + client->privateDataFreeFunc(client->privateData);
> + client->privateData = NULL;
> + }

This patch is corrupted. Please post patches using git-send-email or
other way that does not mangle the code.

> +
> virObjectUnlock(client);
> }
> 
> -- 
> 2.8.3
> 

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



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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Michal Privoznik]

On 02/08/2017 01:23 PM, Peter Krempa wrote:
> On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
>> Nearly all of these functions look the same. Except for a
>> different virSecurityManager API call. There is no need to copy
>> paste the code when we can use macros to generate it.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_security.c | 179 
>> ---
>>  1 file changed, 44 insertions(+), 135 deletions(-)
> 
> NACK, please don't partialy define function with macros.
> 

Why not? What is the downside?

Michal

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

[libvirt] [PATCH] conf: fix use-after-free when sending event message

[Wang King]

If there is a process with a client which registers event callbacks,
and it calls libvirt's API which uses the same virConnectPtr in that
callback function. When this process exit abnormally lead to client
disconnect, there is a possibility that the main thread is refer to
virServerClient just after the virServerClient been freed by job
thread of libvirtd.

Following is the backtrace:
#0 0x7fda223d66d8 in virClassIsDerivedFrom 
(klass=0xdeadbeef,parent=0x7fda24c81b40)
#1 0x7fda223d6a1e in virObjectIsClass 
(anyobj=anyobj@entry=0x7fd9e575b400,klass=)

#2 0x7fda223d6a44 in virObjectLock (anyobj=anyobj@entry=0x7fd9e575b400)
#3 0x7fda22507f71 in virNetServerClientSendMessage 
(client=client@entry=0x7fd9e575b400, msg=msg@entry=0x7fd9ec30de90)
#4 0x7fda230d714d in remoteDispatchObjectEventSend 
(client=0x7fd9e575b400, program=0x7fda24c844e0, procnr=procnr@entry=348, 
proc=0x7fda2310e5e0 , data=data@entry=0x7ffc3857fdb0)
#5 0x7fda230dd71b in remoteRelayDomainEventTunable (conn=, 
dom=0x7fda27cd7660, params=0x7fda27f3aae0, nparams=1, opaque=0x7fd9e6c99e00)
#6 0x7fda224484cb in virDomainEventDispatchDefaultFunc 
(conn=0x7fda27cd0120, event=0x7fda2736ea00, cb=0x7fda230dd610 , 
cbopaque=0x7fd9e6c99e00)
#7 0x7fda22446871 in virObjectEventStateDispatchCallbacks 
(callbacks=, callbacks=, event=0x7fda2736ea00, state=0x7fda24ca3960)
#8 virObjectEventStateQueueDispatch (callbacks=0x7fda24c65800, 
queue=0x7ffc3857fe90, state=0x7fda24ca3960)

#9 virObjectEventStateFlush (state=0x7fda24ca3960)
#10 virObjectEventTimer (timer=, opaque=0x7fda24ca3960)
#11 0x7fda223ae8b9 in virEventPollDispatchTimeouts ()
#12 virEventPollRunOnce ()
#13 0x7fda223ad1d2 in virEventRunDefaultImpl ()
#14 0x7fda225046cd in virNetDaemonRun (dmn=dmn@entry=0x7fda24c775c0)
#15 0x7fda230d6351 in main (argc=, argv=)

(gdb) p *(virNetServerClientPtr)0x7fd9e575b400
$2 = {parent = {parent = {u = {dummy_align1 = 140573849338048, 
dummy_align2 = 0x7fd9e65ac0c0, s = {magic = 3864707264, refs = 32729}}, 
klass = 0x7fda0078}, lock = {lock = {__data = {__lock = 0,
__count = 0, __owner = 0, __nusers = 0, __kind = 0, __spins = 0, __list 
= {__prev = 0x0, __next = 0x0}}, __size = '\000' , __align = 0}}}, 
wantClose = false,
delayedClose = false, sock = 0x0, auth = 0, readonly = false, tlsCtxt = 
0x0, tls = 0x0, sasl = 0x0, sockTimer = 0, identity = 0x0, nrequests = 
0, nrequests_max = 0, rx = 0x0, tx = 0x0, filters = 0x0,
nextFilterID = 0, dispatchFunc = 0x0, dispatchOpaque = 0x0, privateData 
= 0x0, privateDataFreeFunc = 0x0, privateDataPreExecRestart = 0x0, 
privateDataCloseFunc = 0x0, keepalive = 0x0}

---
src/rpc/virnetserverclient.c | 6 ++
1 file changed, 6 insertions(+)

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 81da82c..562516f 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1021,6 +1021,12 @@ void 
virNetServerClientClose(virNetServerClientPtr client)

client->sock = NULL;
}

+ if (client->privateData &&
+ client->privateDataFreeFunc) {
+ client->privateDataFreeFunc(client->privateData);
+ client->privateData = NULL;
+ }
+
virObjectUnlock(client);
}

--
2.8.3

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

[libvirt] [PATCH] xenconfig: fix xml conversion convert with no graphics

[Joao Martins]

If no graphics element is in XML xenFormatXLSpice will access
graphics without checking it has one in the first place, leading to a
segmentation fault.

Signed-off-by: Joao Martins 
---
 src/xenconfig/xen_xl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 2c9174e..74f68b3 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -1168,7 +1168,7 @@ xenFormatXLSpice(virConfPtr conf, virDomainDefPtr def)
 virDomainGraphicsListenDefPtr glisten;
 virDomainGraphicsDefPtr graphics;
 
-if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
+if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && def->graphics) {
 graphics = def->graphics[0];
 
 if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
-- 
2.1.4

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

Re: [libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Peter Krempa]

On Wed, Feb 08, 2017 at 11:37:07 +0100, Michal Privoznik wrote:
> Nearly all of these functions look the same. Except for a
> different virSecurityManager API call. There is no need to copy
> paste the code when we can use macros to generate it.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_security.c | 179 
> ---
>  1 file changed, 44 insertions(+), 135 deletions(-)

NACK, please don't partialy define function with macros.


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

Re: [libvirt] [PATCH] Remove additional newline from virsh-domain-monitor.c

[Peter Krempa]

On Wed, Feb 08, 2017 at 14:20:49 +0530, Nitesh Konkar wrote:
> Currently "virsh domstats domainName" result ends
> with two blanklines. This shall remove the additional
> blank line.
> 
> Signed-off-by: Nitesh Konkar 
> ---
>  tools/virsh-domain-monitor.c | 1 -
>  1 file changed, 1 deletion(-)

NACK,

if you use virsh domstats to report stats for multiple VMs then the
output would not be separated any more:

Domain: 'hp'
  state.state=5
  state.reason=0
  balloon.maximum=1024000
  vcpu.current=2
  vcpu.maximum=10
  block.count=1
  block.0.name=hda
  block.0.path=/tmp/ble.img
Domain: 'rhel7.2-2'
  state.state=5
  state.reason=0
  balloon.maximum=1048576
  vcpu.current=2
  vcpu.maximum=8
  block.count=0
Domain: 'rhel7.2'
  state.state=5
  state.reason=0
  balloon.maximum=2097152
  vcpu.current=2
  vcpu.maximum=2
  block.count=1
  block.0.name=vda
  block.0.path=/var/lib/libvirt/images/rhel7.2.qcow2
  block.0.allocation=9665384448
  block.0.capacity=9663676416
  block.0.physical=9665380352

The only option would be if you skip it for the last entry.




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

[libvirt] [PATCH 06/11] qemu_domain: Don't pass virDomainDeviceDefPtr to ns helpers

[Michal Privoznik]

There is no need for this. None of the namespace helpers uses it.
Historically it was used when calling secdriver APIs, but we
don't to that anymore.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 28 +---
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 54e63878f..067b7a42f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7643,7 +7643,6 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver,
 struct qemuDomainAttachDeviceMknodData {
 virQEMUDriverPtr driver;
 virDomainObjPtr vm;
-virDomainDeviceDefPtr devDef;
 const char *file;
 const char *target;
 struct stat sb;
@@ -7747,7 +7746,6 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 static int
 qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
- virDomainDeviceDefPtr devDef,
  const char *file,
  unsigned int ttl)
 {
@@ -7767,7 +7765,6 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
driver,
 
 data.driver = driver;
 data.vm = vm;
-data.devDef = devDef;
 data.file = file;
 
 if (lstat(file, ) < 0) {
@@ -7840,8 +7837,7 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
driver,
 }
 
 if (isLink &&
-qemuDomainAttachDeviceMknodRecursive(driver, vm, devDef,
- target, ttl -1) < 0)
+qemuDomainAttachDeviceMknodRecursive(driver, vm, target, ttl -1) < 0)
 goto cleanup;
 
 ret = 0;
@@ -7858,13 +7854,11 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
driver,
 static int
 qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
-virDomainDeviceDefPtr devDef,
 const char *file)
 {
 long symloop_max = sysconf(_SC_SYMLOOP_MAX);
 
-return qemuDomainAttachDeviceMknodRecursive(driver, vm, devDef,
-file, symloop_max);
+return qemuDomainAttachDeviceMknodRecursive(driver, vm, file, symloop_max);
 }
 
 
@@ -7888,7 +7882,6 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 static int
 qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
  virDomainObjPtr vm,
- virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
  const char *file)
 {
 if (virProcessRunInMountNamespace(vm->pid,
@@ -7905,7 +7898,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  virDomainDiskDefPtr disk)
 {
-virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_DISK, .data.disk = 
disk};
 virStorageSourcePtr next;
 const char *src = NULL;
 struct stat sb;
@@ -7935,7 +7927,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
 
 if (qemuDomainAttachDeviceMknod(driver,
 vm,
-,
 next->path) < 0)
 goto cleanup;
 }
@@ -7966,7 +7957,6 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 virDomainHostdevDefPtr hostdev)
 {
-virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_HOSTDEV, .data.hostdev 
= hostdev};
 int ret = -1;
 char *path = NULL;
 
@@ -7984,7 +7974,6 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,
 
 if (qemuDomainAttachDeviceMknod(driver,
 vm,
-,
 path) < 0)
 goto cleanup;
 ret = 0;
@@ -7999,7 +7988,6 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr 
driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
 {
-virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_HOSTDEV, .data.hostdev 
= hostdev};
 int ret = -1;
 char *path = NULL;
 
@@ -8015,7 +8003,7 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr 
driver,
 goto cleanup;
 }
 
-if (qemuDomainDetachDeviceUnlink(driver, vm, , path) < 0)
+if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0)
 goto cleanup;
 
 ret = 0;
@@ -8030,7 +8018,6 @@ qemuDomainNamespaceSetupChardev(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 virDomainChrDefPtr chr)
 {
-virDomainDeviceDef dev = {.type = VIR_DOMAIN_DEVICE_CHR, .data.chr = chr};
 const char *path;
 int ret = -1;
 
@@ -8044,7 +8031,6 @@ 

[libvirt] [PATCH 07/11] qemuDomainNamespaceSetupDisk: Drop useless @src variable

[Michal Privoznik]

Since its introduction in 81df21507bef9 this variable was never
used.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 067b7a42f..8ec9601d2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7899,7 +7899,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
  virDomainDiskDefPtr disk)
 {
 virStorageSourcePtr next;
-const char *src = NULL;
 struct stat sb;
 int ret = -1;
 
@@ -7914,14 +7913,14 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
 
 if (stat(next->path, ) < 0) {
 virReportSystemError(errno,
- _("Unable to access %s"), src);
+ _("Unable to access %s"), next->path);
 goto cleanup;
 }
 
 if (!S_ISBLK(sb.st_mode)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Disk source %s must be a block device"),
-   src);
+   next->path);
 goto cleanup;
 }
 
-- 
2.11.0

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

[libvirt] [PATCH 09/11] qemuDomainDiskChainElement{Prepare, Revoke}: manage /dev entry

[Michal Privoznik]

Again, one missed bit. This time without this commit there is no
/dev entry when doing disk snapshots.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5db8b60c5..9e34d73be 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5090,14 +5090,17 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr 
driver,
  virDomainObjPtr vm,
  virStorageSourcePtr elem)
 {
-if (virSecurityManagerRestoreImageLabel(driver->securityManager,
-vm->def, elem) < 0)
-VIR_WARN("Unable to restore security label on %s", 
NULLSTR(elem->path));
-
 if (qemuTeardownImageCgroup(vm, elem) < 0)
 VIR_WARN("Failed to teardown cgroup for disk path %s",
  NULLSTR(elem->path));
 
+if (virSecurityManagerRestoreImageLabel(driver->securityManager,
+vm->def, elem) < 0)
+VIR_WARN("Unable to restore security label on %s", 
NULLSTR(elem->path));
+
+if (qemuDomainNamespaceTeardownDisk(driver, vm, elem) < 0)
+VIR_WARN("Unable to remove /dev entry for %s", NULLSTR(elem->path));
+
 if (virDomainLockImageDetach(driver->lockManager, vm, elem) < 0)
 VIR_WARN("Unable to release lock on %s", NULLSTR(elem->path));
 }
@@ -5126,6 +5129,9 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver,
 if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, elem) < 0)
 goto cleanup;
 
+if (qemuDomainNamespaceSetupDisk(driver, vm, elem) < 0)
+goto cleanup;
+
 if (qemuSetupImageCgroup(vm, elem) < 0)
 goto cleanup;
 
-- 
2.11.0

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

[libvirt] [PATCH 05/11] qemu_security: Drop qemuSecuritySetRestoreAllLabelData struct

[Michal Privoznik]

This struct is unused after 095f042ed68b01.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_security.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index b2155afcf..06bff2470 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -31,15 +31,6 @@
 
 VIR_LOG_INIT("qemu.qemu_process");
 
-struct qemuSecuritySetRestoreAllLabelData {
-bool set;
-virQEMUDriverPtr driver;
-virDomainObjPtr vm;
-const char *stdin_path;
-bool migrated;
-};
-
-
 #define PROLOGUE(F, type)   \
 int \
 qemuSecurity##F(virQEMUDriverPtr driver,\
-- 
2.11.0

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

[libvirt] [PATCH 02/11] syntax-check: Enforce qemuSecurity

[Michal Privoznik]

Now that we have some qemuSecurity wrappers over
virSecurityManager APIs, lets make sure everybody sticks with
them. We have them for a reason and calling virSecurityManager
API directly instead of wrapper may lead into accidentally
labelling a file on the host instead of namespace.

Signed-off-by: Michal Privoznik 
---
 cfg.mk | 8 
 1 file changed, 8 insertions(+)

diff --git a/cfg.mk b/cfg.mk
index 69e3f3a1a..6fb2fc961 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -585,6 +585,14 @@ sc_prohibit_unsigned_pid:
halt='use signed type for pid values'   \
  $(_sc_search_regexp)
 
+sc_prohibit_direct_secdriver:
+   @for i in $$(grep -i ^WRAP.\( src/qemu/qemu_security.c |
\
+   awk 'BEGIN {FS = "[^[:alnum:]]"} {print "virSecurityManager" $$2 }'); 
do\
+ grep -n $$i $$($(VC_LIST_EXCEPT) | grep -E '^src/qemu/') && \
+ { echo "$(ME): prefer qemuSecurity$${i#virSecurityManager} over $$i" 
1>&2; exit 1; }  \
+done || :
+
+
 # Many of the function names below came from this filter:
 # git grep -B2 '\<_('|grep -E '\.c- *[[:alpha:]_][[:alnum:]_]* ?\(.*[,;]$' \
 # |sed 's/.*\.c-  *//'|perl -pe 's/ ?\(.*//'|sort -u \
-- 
2.11.0

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

[libvirt] [PATCH 03/11] qemuDomainAttachSCSIVHostDevice: manage /dev entry

[Michal Privoznik]

Again, one missed bit. This time without this commit there is no
/dev entry when attaching vhost SCSI device.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_hotplug.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index dd6e31823..778c8ef20 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2532,6 +2532,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
 char *devstr = NULL;
 bool teardowncgroup = false;
 bool teardownlabel = false;
+bool teardowndevice = false;
 bool releaseaddr = false;
 
 if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
@@ -2548,6 +2549,10 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
 return -1;
 }
 
+if (qemuDomainNamespaceSetupHostdev(driver, vm, hostdev) < 0)
+goto cleanup;
+teardowndevice = true;
+
 if (qemuSetupHostdevCgroup(vm, hostdev) < 0)
 goto cleanup;
 teardowncgroup = true;
@@ -2613,6 +2618,9 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
 if (teardownlabel &&
 qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0)
 VIR_WARN("Unable to restore host device labelling on hotplug 
fail");
+if (teardowndevice &&
+qemuDomainNamespaceTeardownHostdev(driver, vm, hostdev) < 0)
+VIR_WARN("Unable to remove host device from /dev");
 if (releaseaddr)
 qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
 }
-- 
2.11.0

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

[libvirt] [PATCH 01/11] qemuDomainAttachSCSIVHostDevice: Prefer qemuSecurity wrappers

[Michal Privoznik]

Since we have qemuSecurity wrappers over
virSecurityManagerSetHostdevLabel and
virSecurityManagerRestoreHostdevLabel we ought to use them
instead of calling secdriver APIs directly.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_hotplug.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e272df356..dd6e31823 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2552,8 +2552,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
 goto cleanup;
 teardowncgroup = true;
 
-if (virSecurityManagerSetHostdevLabel(driver->securityManager,
-  vm->def, hostdev, NULL) < 0)
+if (qemuSecuritySetHostdevLabel(driver, vm, hostdev) < 0)
 goto cleanup;
 teardownlabel = true;
 
@@ -2612,8 +2611,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
 if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0)
 VIR_WARN("Unable to remove host device cgroup ACL on hotplug 
fail");
 if (teardownlabel &&
-virSecurityManagerRestoreHostdevLabel(driver->securityManager,
-  vm->def, hostdev, NULL) < 0)
+qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0)
 VIR_WARN("Unable to restore host device labelling on hotplug 
fail");
 if (releaseaddr)
 qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
-- 
2.11.0

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

[libvirt] [PATCH 11/11] qemuDomainNamespaceSetupDisk: Simplify disk check

[Michal Privoznik]

Firstly, instead of checking for next->path the
virStorageSourceIsEmpty() function should be used which also
takes disk type into account.
Secondly, not every disk source passed has the correct type set
(due to our laziness). Therefore, instead of checking for
virStorageSourceIsBlockLocal() and also S_ISBLK() the former can
be refined to just virStorageSourceIsLocalStorage().

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a4ee652db..7c696963e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7911,7 +7911,8 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
 return 0;
 
 for (next = src; next; next = next->backingStore) {
-if (!next->path || !virStorageSourceIsBlockLocal(src)) {
+if (virStorageSourceIsEmpty(next) ||
+!virStorageSourceIsLocalStorage(next)) {
 /* Not creating device. Just continue. */
 continue;
 }
@@ -7922,12 +7923,8 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
-if (!S_ISBLK(sb.st_mode)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Disk source %s must be a block device"),
-   next->path);
-goto cleanup;
-}
+if (!S_ISBLK(sb.st_mode))
+continue;
 
 if (qemuDomainAttachDeviceMknod(driver,
 vm,
-- 
2.11.0

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

[libvirt] [PATCH 08/11] qemuDomainNamespace{Setup, Teardown}Disk: Don't pass pointer to full disk

[Michal Privoznik]

These functions do not need to see the whole virDomainDiskDef.
Moreover, they are going to be called from places where we don't
have access to the full disk definition. Sticking with
virStorageSource is more than enough.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c  | 8 
 src/qemu/qemu_domain.h  | 4 ++--
 src/qemu/qemu_driver.c  | 2 +-
 src/qemu/qemu_hotplug.c | 6 +++---
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8ec9601d2..5db8b60c5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7896,7 +7896,7 @@ qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 int
 qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
- virDomainDiskDefPtr disk)
+ virStorageSourcePtr src)
 {
 virStorageSourcePtr next;
 struct stat sb;
@@ -7905,8 +7905,8 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
 if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
 return 0;
 
-for (next = disk->src; next; next = next->backingStore) {
-if (!next->path || !virStorageSourceIsBlockLocal(disk->src)) {
+for (next = src; next; next = next->backingStore) {
+if (!next->path || !virStorageSourceIsBlockLocal(src)) {
 /* Not creating device. Just continue. */
 continue;
 }
@@ -7939,7 +7939,7 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
 int
 qemuDomainNamespaceTeardownDisk(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
 virDomainObjPtr vm ATTRIBUTE_UNUSED,
-virDomainDiskDefPtr disk ATTRIBUTE_UNUSED)
+virStorageSourcePtr src ATTRIBUTE_UNUSED)
 {
 /* While in hotplug case we create the whole backing chain,
  * here we must limit ourselves. The disk we want to remove
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 39731826e..5cfa3e114 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -810,11 +810,11 @@ int qemuDomainCreateNamespace(virQEMUDriverPtr driver,
 
 int qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
- virDomainDiskDefPtr disk);
+ virStorageSourcePtr src);
 
 int qemuDomainNamespaceTeardownDisk(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
-virDomainDiskDefPtr disk);
+virStorageSourcePtr src);
 
 int qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 37ccfdf6b..89bc833de 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15915,7 +15915,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
 
 if (disk->mirror->format &&
 disk->mirror->format != VIR_STORAGE_FILE_RAW &&
-(qemuDomainNamespaceSetupDisk(driver, vm, disk) < 0 ||
+(qemuDomainNamespaceSetupDisk(driver, vm, disk->src) < 0 ||
  qemuSetupDiskCgroup(vm, disk) < 0 ||
  qemuSecuritySetDiskLabel(driver, vm, disk) < 0))
 goto cleanup;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 778c8ef20..2f209f12b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -110,7 +110,7 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
 vm, disk) < 0)
 goto cleanup;
 
-if (qemuDomainNamespaceSetupDisk(driver, vm, disk) < 0)
+if (qemuDomainNamespaceSetupDisk(driver, vm, disk->src) < 0)
 goto rollback_lock;
 
 if (qemuSecuritySetDiskLabel(driver, vm, disk) < 0)
@@ -132,7 +132,7 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
  virDomainDiskGetSource(disk));
 
  rollback_namespace:
-if (qemuDomainNamespaceTeardownDisk(driver, vm, disk) < 0)
+if (qemuDomainNamespaceTeardownDisk(driver, vm, disk->src) < 0)
 VIR_WARN("Unable to remove /dev entry for %s",
  virDomainDiskGetSource(disk));
 
@@ -3649,7 +3649,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
 if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
 VIR_WARN("Unable to release lock on %s", src);
 
-if (qemuDomainNamespaceTeardownDisk(driver, vm, disk) < 0)
+if (qemuDomainNamespaceTeardownDisk(driver, vm, disk->src) < 0)
 VIR_WARN("Unable to remove /dev entry for %s", src);
 
 dev.type = VIR_DOMAIN_DEVICE_DISK;
-- 
2.11.0

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

[libvirt] [PATCH 04/11] qemu_security: Kill code duplication

[Michal Privoznik]

Nearly all of these functions look the same. Except for a
different virSecurityManager API call. There is no need to copy
paste the code when we can use macros to generate it.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_security.c | 179 ---
 1 file changed, 44 insertions(+), 135 deletions(-)

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 35cdf50b0..b2155afcf 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -40,33 +40,49 @@ struct qemuSecuritySetRestoreAllLabelData {
 };
 
 
-int
-qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
-virDomainObjPtr vm,
-const char *stdin_path)
-{
-int ret = -1;
+#define PROLOGUE(F, type)   \
+int \
+qemuSecurity##F(virQEMUDriverPtr driver,\
+virDomainObjPtr vm, \
+type var)   \
+{   \
+int ret = -1;   \
+\
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && \
+virSecurityManagerTransactionStart(driver->securityManager) < 0)\
+goto cleanup;   \
 
-if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
-virSecurityManagerTransactionStart(driver->securityManager) < 0)
-goto cleanup;
-
-if (virSecurityManagerSetAllLabel(driver->securityManager,
-  vm->def,
-  stdin_path) < 0)
-goto cleanup;
-
-if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
-virSecurityManagerTransactionCommit(driver->securityManager,
-vm->pid) < 0)
-goto cleanup;
-
-ret = 0;
- cleanup:
-virSecurityManagerTransactionAbort(driver->securityManager);
-return ret;
+#define EPILOGUE\
+if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && \
+virSecurityManagerTransactionCommit(driver->securityManager,\
+vm->pid) < 0)   \
+goto cleanup;   \
+\
+ret = 0;\
+ cleanup:   \
+virSecurityManagerTransactionAbort(driver->securityManager);\
+return ret; \
 }
 
+#define WRAP1(F, type)  \
+PROLOGUE(F, type)   \
+if (virSecurityManager##F(driver->securityManager,  \
+  vm->def,  \
+  var) < 0) \
+goto cleanup;   \
+\
+EPILOGUE
+
+#define WRAP2(F, type)  \
+PROLOGUE(F, type)   \
+if (virSecurityManager##F(driver->securityManager,  \
+  vm->def,  \
+  var, NULL) < 0)   \
+goto cleanup;   \
+\
+EPILOGUE
+
+WRAP1(SetAllLabel, const char *)
 
 void
 qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
@@ -85,115 +101,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
 }
 
 
-int
-qemuSecuritySetDiskLabel(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDiskDefPtr disk)
-{
-int ret = -1;
+WRAP1(SetDiskLabel, virDomainDiskDefPtr)
+WRAP1(RestoreDiskLabel, virDomainDiskDefPtr)
 
-if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
-virSecurityManagerTransactionStart(driver->securityManager) < 0)
-goto cleanup;
-
-if (virSecurityManagerSetDiskLabel(driver->securityManager,
-   vm->def,
- 

[libvirt] [PATCH 00/11] qemu: Big namespace cleanup

[Michal Privoznik]

Couple of these patches were requested in reviews to my previous namespace
patches, couple of them are my own idea. At the same time, 3 bugs are fixed:
1) qemuSecurity wrappers are preferred over virSecurityManager
2) corresponding /dev entry is not created when attaching vhost scsi device
3) corresponding /dev entry is not created when doing blockjobs

Michal Privoznik (11):
  qemuDomainAttachSCSIVHostDevice: Prefer qemuSecurity wrappers
  syntax-check: Enforce qemuSecurity
  qemuDomainAttachSCSIVHostDevice: manage /dev entry
  qemu_security: Kill code duplication
  qemu_security: Drop qemuSecuritySetRestoreAllLabelData struct
  qemu_domain: Don't pass virDomainDeviceDefPtr to ns helpers
  qemuDomainNamespaceSetupDisk: Drop useless @src variable
  qemuDomainNamespace{Setup,Teardown}Disk: Don't pass pointer to full
disk
  qemuDomainDiskChainElement{Prepare,Revoke}: manage /dev entry
  qemu_security: Introduce ImageLabel APIs
  qemuDomainNamespaceSetupDisk: Simplify disk check

 cfg.mk   |   8 ++
 src/qemu/qemu_domain.c   |  65 ++--
 src/qemu/qemu_domain.h   |   4 +-
 src/qemu/qemu_driver.c   |   2 +-
 src/qemu/qemu_hotplug.c  |  20 +++--
 src/qemu/qemu_security.c | 189 ---
 src/qemu/qemu_security.h |   8 ++
 7 files changed, 102 insertions(+), 194 deletions(-)

-- 
2.11.0

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

[libvirt] [PATCH 10/11] qemu_security: Introduce ImageLabel APIs

[Michal Privoznik]

Just like we need wrappers over other virSecurityManager APIs, we
need one for virSecurityManagerSetImageLabel and
virSecurityManagerRestoreImageLabel.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c   | 7 +++
 src/qemu/qemu_security.c | 3 +++
 src/qemu/qemu_security.h | 8 
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9e34d73be..a4ee652db 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -31,6 +31,7 @@
 #include "qemu_parse_command.h"
 #include "qemu_capabilities.h"
 #include "qemu_migration.h"
+#include "qemu_security.h"
 #include "viralloc.h"
 #include "virlog.h"
 #include "virerror.h"
@@ -5094,8 +5095,7 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver,
 VIR_WARN("Failed to teardown cgroup for disk path %s",
  NULLSTR(elem->path));
 
-if (virSecurityManagerRestoreImageLabel(driver->securityManager,
-vm->def, elem) < 0)
+if (qemuSecurityRestoreImageLabel(driver, vm, elem) < 0)
 VIR_WARN("Unable to restore security label on %s", 
NULLSTR(elem->path));
 
 if (qemuDomainNamespaceTeardownDisk(driver, vm, elem) < 0)
@@ -5135,8 +5135,7 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver,
 if (qemuSetupImageCgroup(vm, elem) < 0)
 goto cleanup;
 
-if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def,
-elem) < 0)
+if (qemuSecuritySetImageLabel(driver, vm, elem) < 0)
 goto cleanup;
 
 ret = 0;
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 06bff2470..131be6e4b 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -95,5 +95,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver,
 WRAP1(SetDiskLabel, virDomainDiskDefPtr)
 WRAP1(RestoreDiskLabel, virDomainDiskDefPtr)
 
+WRAP1(SetImageLabel, virStorageSourcePtr)
+WRAP1(RestoreImageLabel, virStorageSourcePtr)
+
 WRAP2(SetHostdevLabel, virDomainHostdevDefPtr)
 WRAP2(RestoreHostdevLabel, virDomainHostdevDefPtr)
diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
index cc373b3e1..54638908d 100644
--- a/src/qemu/qemu_security.h
+++ b/src/qemu/qemu_security.h
@@ -45,6 +45,14 @@ int qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  virDomainDiskDefPtr disk);
 
+int qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  virStorageSourcePtr src);
+
+int qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  virStorageSourcePtr src);
+
 int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 virDomainHostdevDefPtr hostdev);
-- 
2.11.0

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