[libvirt] [RFC][PATCHv1 0/5] libvirt - show per cpu accounting information
When running 'virt-top -1' , "show percpu statistics mode",
virt-top shows a fake value.
When running 'while true; do echo hello;done' on a 4vcpu guest,
==
00.0
10.0
20.0
30.0
4 25.0 25.0=
5 25.0 25.0=#
6 25.0 25.0=
7 25.0 25.0=
==
All cpus just used equally ;)
This is because there is no interface to get per-cpu usage of domain.
This patch adds an interface virDomainPcpuStats() to get per-cpu
statistics, cpuTime in nanoseconds.
Here is a test result with a python script using new interface.
==
[root@bluextal python]# ./virt-cpuacct.py
({'cpuTime': 0L}, {'cpuTime': 0L}, {'cpuTime': 0L}, {'cpuTime': 0L},
{'cpuTime': 4679204346L}, {'cpuTime': 2103820380L}, {'cpuTime': 8904513019L},
{'cpuTime': 7424701195L})
[root@bluextal python]# ./virt-cpuacct.py
({'cpuTime': 0L}, {'cpuTime': 0L}, {'cpuTime': 0L}, {'cpuTime': 0L},
{'cpuTime': 57010689139L}, {'cpuTime': 26152907202L}, {'cpuTime':
53759693931L}, {'cpuTime': 43074348218L})
==
Although I added a new interface, I still wonder what interface is better...
any comments are welcome.
Thanks,
-Kame
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC][PATCHv1 1/5] libvirt - show cpuacct cgroup's percpu statistics.
cpuacct cgroup provides per cpu cputime. Add an interface
for that.
Signed-off-by: KAMEZAWA Hiroyuki
---
src/libvirt_private.syms |1 +
src/util/cgroup.c| 27 +++
src/util/cgroup.h|2 ++
3 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ba7739d..f7a1ee7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -70,6 +70,7 @@ virCgroupFree;
virCgroupGetBlkioWeight;
virCgroupGetCpuShares;
virCgroupGetCpuacctUsage;
+virCgroupGetCpuacctUsagePercpu;
virCgroupGetFreezerState;
virCgroupGetMemoryHardLimit;
virCgroupGetMemorySoftLimit;
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index afe8731..58ea805 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -1329,6 +1329,33 @@ int virCgroupGetCpuacctUsage(virCgroupPtr group,
unsigned long long *usage)
"cpuacct.usage", usage);
}
+int virCgroupGetCpuacctUsagePercpu(virCgroupPtr group,
+ int nr, unsigned long long *usage)
+{
+int i, rc;
+char *strval = NULL;
+char *startp, *endp;
+unsigned long long int value;
+
+memset(usage, 0, sizeof(unsigned long long) * nr);
+
+rc = virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_CPUACCT,
+ "cpuacct.usage_percpu", &strval);
+if (rc)
+goto out;
+
+for (i = 0, startp = strval; i < nr; i++) {
+if (virStrToLong_ull(startp, &endp, 10, &value))
+break;
+startp = endp;
+usage[i] = value;
+}
+rc = i;
+out:
+VIR_FREE(strval);
+return rc;
+}
+
int virCgroupSetFreezerState(virCgroupPtr group, const char *state)
{
return virCgroupSetValueStr(group,
diff --git a/src/util/cgroup.h b/src/util/cgroup.h
index 8ae756d..31cbcba 100644
--- a/src/util/cgroup.h
+++ b/src/util/cgroup.h
@@ -100,6 +100,8 @@ int virCgroupSetCpuShares(virCgroupPtr group, unsigned long
long shares);
int virCgroupGetCpuShares(virCgroupPtr group, unsigned long long *shares);
int virCgroupGetCpuacctUsage(virCgroupPtr group, unsigned long long *usage);
+int virCgroupGetCpuacctUsagePercpu(virCgroupPtr group,
+ int nr, unsigned long long *usage);
int virCgroupSetFreezerState(virCgroupPtr group, const char *state);
int virCgroupGetFreezerState(virCgroupPtr group, char **state);
--
1.7.4.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC][PATCHv1 2/5] libvirt - new API for getting percpu statistics of VM
Per (host) cpu activity of VMs are very insterested numbers
when running VMs on large SMPs. virt-top -1 mode tries to
provide the information. (But it's not implemented.)
This patch adds a libvirt interface to get per cpu statistics
of each nodes. This patch just adds an interface and driver
entry points. So,
- doesn't include patches for python.
- doesn't include any driver.
Following patches will add some drivers and codes for python.
Signed-off-by: KAMEZAWA Hiroyuki
---
include/libvirt/libvirt.h.in | 13 ++
src/driver.h |6
src/esx/esx_driver.c |1 +
src/libvirt.c| 55 ++
src/libvirt_public.syms |4 +++
src/libxl/libxl_driver.c |1 +
src/lxc/lxc_driver.c |1 +
src/openvz/openvz_driver.c |1 +
src/phyp/phyp_driver.c |1 +
src/qemu/qemu_driver.c |1 +
src/remote/remote_driver.c |1 +
src/test/test_driver.c |1 +
src/uml/uml_driver.c |1 +
src/vbox/vbox_tmpl.c |1 +
src/vmware/vmware_driver.c |1 +
src/xen/xen_driver.c |1 +
src/xenapi/xenapi_driver.c |1 +
17 files changed, 91 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 5783303..6b9292c 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -400,6 +400,13 @@ struct _virDomainMemoryStat {
typedef virDomainMemoryStatStruct *virDomainMemoryStatPtr;
+typedef struct _virDomainPcpuStat virDomainPcpuStatStruct;
+
+struct _virDomainPcpuStat {
+unsigned long long cpuTime;
+};
+
+typedef virDomainPcpuStatStruct *virDomainPcpuStatPtr;
/* Domain core dump flags. */
typedef enum {
@@ -923,6 +930,12 @@ int virDomainMemoryStats (virDomainPtr
dom,
virDomainMemoryStatPtr stats,
unsigned int nr_stats,
unsigned int flags);
+
+int virDomainPcpuStats (virDomainPtr dom,
+virDomainPcpuStatPtr stats,
+unsigned int nr_stats,
+unsigned int flags);
+
int virDomainBlockPeek (virDomainPtr dom,
const char *path,
unsigned long long offset,
diff --git a/src/driver.h b/src/driver.h
index a8b79e6..ee1dac7 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -291,6 +291,11 @@ typedef int
(virDomainPtr domain,
struct _virDomainMemoryStat *stats,
unsigned int nr_stats);
+typedef int
+(*virDrvDomainPcpuStats)
+(virDomainPtr domain,
+ struct _virDomainPcpuStat *stats,
+ unsigned int nr_stats);
typedef int
(*virDrvDomainBlockPeek)
@@ -599,6 +604,7 @@ struct _virDriver {
virDrvDomainBlockStats domainBlockStats;
virDrvDomainInterfaceStats domainInterfaceStats;
virDrvDomainMemoryStats domainMemoryStats;
+virDrvDomainPcpuStats domainPcpuStats;
virDrvDomainBlockPeek domainBlockPeek;
virDrvDomainMemoryPeek domainMemoryPeek;
virDrvDomainGetBlockInfodomainGetBlockInfo;
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 50c631b..34a31f0 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -4632,6 +4632,7 @@ static virDriver esxDriver = {
NULL,/* domainBlockStats */
NULL,/* domainInterfaceStats */
NULL,/* domainMemoryStats */
+NULL,/* domainPcpuStats */
NULL,/* domainBlockPeek */
NULL,/* domainMemoryPeek */
NULL,/* domainGetBlockInfo */
diff --git a/src/libvirt.c b/src/libvirt.c
index 0da9885..24ef621 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -4595,6 +4595,61 @@ error:
}
/**
+ * virDomainPcpuStats:
+ * @dom: pointer to the domain object
+ * @stats: nr_stats-sized array of stat structures (returned)
+ * @nr_stats: number of cpu statistics requested
+ * @flags: unused, always pass 0
+ *
+ * This function provides per-cpu statistics for the domain. 'cpu' here means
+ * not vcpu.
+ *
+ * Up to 'nr_stats' elements of 'stats' will be populated with cpu statistics
+ * from the domain. Only statistics supported by the domain, the driver, and
+ * this version of libvirt will be returned.
+ *
+ * Now, only cpuTime per cpu is reported in nanoseconds.
+ *
+ * Returns: The number of stats provided or -1 in case of failure.
+ */
+int virDomainPcpuStats (virDomainPtr dom, virDomainPcpuStatPtr stats,
+
[libvirt] [RFC][PATCHv1 3/5] Add a remote driver for virDomainPcpuStats()
Signed-off-by: KAMEZAWA Hiroyuki
---
daemon/remote.c | 61 +++
daemon/remote_dispatch_args.h |1 +
daemon/remote_dispatch_prototypes.h |8
daemon/remote_dispatch_ret.h|1 +
daemon/remote_dispatch_table.h |5 +++
src/remote/remote_driver.c | 39 +-
src/remote/remote_protocol.c| 31 ++
src/remote/remote_protocol.h| 27 +++
src/remote/remote_protocol.x| 18 ++-
9 files changed, 189 insertions(+), 2 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index 5de1055..9757f13 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1138,6 +1138,67 @@ remoteDispatchDomainMemoryStats(struct qemud_server
*server ATTRIBUTE_UNUSED,
}
static int
+remoteDispatchDomainPcpuStats(struct qemud_server *server ATTRIBUTE_UNUSED,
+ struct qemud_client *client ATTRIBUTE_UNUSED,
+ virConnectPtr conn,
+ remote_message_header *hdr ATTRIBUTE_UNUSED,
+ remote_error *rerr,
+ remote_domain_pcpu_stats_args *args,
+ remote_domain_pcpu_stats_ret *ret)
+{
+virDomainPtr dom;
+struct _virDomainPcpuStat *stats;
+unsigned int nr_stats, i;
+
+if (!conn) {
+remoteDispatchFormatError(rerr, "%s", _("connection not open"));
+return -1;
+}
+
+if (args->nr_stats > REMOTE_DOMAIN_PCPU_STATS_MAX) {
+remoteDispatchFormatError(rerr, "%s",
+ _("nr_stats > REMOTE_DOMAIN_PCPU_STATS_MAX"));
+return -1;
+}
+
+dom = get_nonnull_domain(conn, args->dom);
+if (dom == NULL) {
+remoteDispatchConnError(rerr, conn);
+return -1;
+}
+
+/* Allocate stats array for making dispatch call */
+if (VIR_ALLOC_N(stats, args->nr_stats) < 0) {
+virDomainFree(dom);
+remoteDispatchOOMError(rerr);
+return -1;
+}
+
+nr_stats = virDomainPcpuStats(dom, stats, args->nr_stats, 0);
+if (nr_stats == -1) {
+VIR_FREE(stats);
+remoteDispatchConnError(rerr, conn);
+virDomainFree(dom);
+return -1;
+}
+virDomainFree(dom);
+
+/* Allocate return buffer */
+if (VIR_ALLOC_N(ret->stats.stats_val, nr_stats) < 0) {
+VIR_FREE(stats);
+remoteDispatchOOMError(rerr);
+return -1;
+}
+
+/* Copy the stats into the xdr return structure */
+for (i = 0; i < nr_stats; i++)
+ret->stats.stats_val[i].val = stats[i].cpuTime;
+ret->stats.stats_len = nr_stats;
+VIR_FREE(stats);
+return 0;
+}
+
+static int
remoteDispatchDomainBlockPeek(struct qemud_server *server ATTRIBUTE_UNUSED,
struct qemud_client *client ATTRIBUTE_UNUSED,
virConnectPtr conn,
diff --git a/daemon/remote_dispatch_args.h b/daemon/remote_dispatch_args.h
index f9537d7..42fc7b2 100644
--- a/daemon/remote_dispatch_args.h
+++ b/daemon/remote_dispatch_args.h
@@ -178,3 +178,4 @@
remote_domain_migrate_set_max_speed_args
val_remote_domain_migrate_set_max_speed_args;
remote_storage_vol_upload_args val_remote_storage_vol_upload_args;
remote_storage_vol_download_args val_remote_storage_vol_download_args;
+remote_domain_pcpu_stats_args val_remote_domain_pcpu_stats_args;
diff --git a/daemon/remote_dispatch_prototypes.h
b/daemon/remote_dispatch_prototypes.h
index 18bf41d..5dece04 100644
--- a/daemon/remote_dispatch_prototypes.h
+++ b/daemon/remote_dispatch_prototypes.h
@@ -498,6 +498,14 @@ static int remoteDispatchDomainOpenConsole(
remote_error *err,
remote_domain_open_console_args *args,
void *ret);
+static int remoteDispatchDomainPcpuStats(
+struct qemud_server *server,
+struct qemud_client *client,
+virConnectPtr conn,
+remote_message_header *hdr,
+remote_error *err,
+remote_domain_pcpu_stats_args *args,
+remote_domain_pcpu_stats_ret *ret);
static int remoteDispatchDomainPinVcpu(
struct qemud_server *server,
struct qemud_client *client,
diff --git a/daemon/remote_dispatch_ret.h b/daemon/remote_dispatch_ret.h
index 114e832..4c0b66d 100644
--- a/daemon/remote_dispatch_ret.h
+++ b/daemon/remote_dispatch_ret.h
@@ -140,3 +140,4 @@
remote_domain_is_updated_ret val_remote_domain_is_updated_ret;
remote_get_sysinfo_ret val_remote_get_sysinfo_ret;
remote_domain_get_blkio_parameters_ret
val_remote_domain_get_blkio_parameters_ret;
+remote_domain_pcpu_stats_ret val_remote_domain_pcpu_stats_ret;
diff --git a/daemon/remote_dispatch_table.h b/daemon/remote_dispatch_table.h
index b39f7c2..cb1fa05 100644
--- a/daemon/remote_dispatch_table.h
+++ b/daemon/remote_dispatch_table.h
@@ -1052,3 +1052,8 @@
.args_filter = (xdrproc_t) xdr_remote_storage_vol_download_args,
.ret_filter = (xdrproc
[libvirt] [RFC][PATCHv1 4/5] Add a python interface for virDomainPcpuStats.
Values of all cpus are returned by a tuple of Dict as:
[{cpuTime: }, {cpuTime: }} # when the number of cpu is 2.
Signed-off-by: KAMEZAWA Hiroyuki
---
python/generator.py |1 +
python/libvirt-override-api.xml |5
python/libvirt-override.c | 42 +++
3 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/python/generator.py b/python/generator.py
index 4fa4f65..d2695bb 100755
--- a/python/generator.py
+++ b/python/generator.py
@@ -307,6 +307,7 @@ skip_impl = (
'virDomainBlockStats',
'virDomainInterfaceStats',
'virDomainMemoryStats',
+'virDomainPcpuStats',
'virNodeGetCellsFreeMemory',
'virDomainGetSchedulerType',
'virDomainGetSchedulerParameters',
diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml
index 54deeb5..5ee3881 100644
--- a/python/libvirt-override-api.xml
+++ b/python/libvirt-override-api.xml
@@ -127,6 +127,11 @@
+
+ Extracts per cpu statistics for a domain
+
+
+
Returns the available memory for a list of cells
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 4a9b432..c2ff0aa 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -164,6 +164,47 @@ libvirt_virDomainMemoryStats(PyObject *self
ATTRIBUTE_UNUSED, PyObject *args) {
return info;
}
+static PyObject*
+libvirt_virDomainPcpuStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
+virDomainPtr domain;
+virNodeInfo nodeinfo;
+PyObject *pyobj_domain;
+PyObject *info, *info2;
+int nr_stats, i_retval, i;
+virDomainPcpuStatPtr stats;
+
+if (!PyArg_ParseTuple(args, (char *)"O:virDomainPcpuStats", &pyobj_domain))
+return (NULL);
+
+domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+i_retval = virNodeGetInfo(virDomainGetConnect(domain), &nodeinfo);
+LIBVIRT_END_ALLOW_THREADS;
+if (i_retval < 0)
+return VIR_PY_NONE;
+
+stats = malloc(sizeof(struct _virDomainPcpuStat) * nodeinfo.cpus);
+
+nr_stats = virDomainPcpuStats(domain, stats, nodeinfo.cpus, 0);
+
+if (nr_stats == -1)
+return VIR_PY_NONE;
+
+if ((info = PyTuple_New(nodeinfo.cpus)) == NULL)
+return VIR_PY_NONE;
+
+for (i = 0; i < nr_stats; i++) {
+if ((info2 = PyDict_New()) == NULL)
+return VIR_PY_NONE;
+PyDict_SetItem(info2, libvirt_constcharPtrWrap("cpuTime"),
+ PyLong_FromLongLong(stats[i].cpuTime));
+PyTuple_SetItem(info, i, info2);
+}
+return (info);
+}
+
+
static PyObject *
libvirt_virDomainGetSchedulerType(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args) {
@@ -3545,6 +3586,7 @@ static PyMethodDef libvirtMethods[] = {
{(char *) "virDomainBlockStats", libvirt_virDomainBlockStats,
METH_VARARGS, NULL},
{(char *) "virDomainInterfaceStats", libvirt_virDomainInterfaceStats,
METH_VARARGS, NULL},
{(char *) "virDomainMemoryStats", libvirt_virDomainMemoryStats,
METH_VARARGS, NULL},
+{(char *) "virDomainPcpuStats", libvirt_virDomainPcpuStats, METH_VARARGS,
NULL},
{(char *) "virNodeGetCellsFreeMemory", libvirt_virNodeGetCellsFreeMemory,
METH_VARARGS, NULL},
{(char *) "virDomainGetSchedulerType", libvirt_virDomainGetSchedulerType,
METH_VARARGS, NULL},
{(char *) "virDomainGetSchedulerParameters",
libvirt_virDomainGetSchedulerParameters, METH_VARARGS, NULL},
--
1.7.4.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC][PATCHv1 5/5] Add a qemu driver for virDomainPcpuStats()
Signed-off-by: KAMEZAWA Hiroyuki
---
src/qemu/qemu_conf.c |1 +
src/qemu/qemu_driver.c | 71 +++-
2 files changed, 71 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index bb5421b..89c5b4c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -302,6 +302,7 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
} else {
driver->cgroupControllers =
(1 << VIR_CGROUP_CONTROLLER_CPU) |
+(1 << VIR_CGROUP_CONTROLLER_CPUACCT) |
(1 << VIR_CGROUP_CONTROLLER_DEVICES) |
(1 << VIR_CGROUP_CONTROLLER_MEMORY) |
(1 << VIR_CGROUP_CONTROLLER_BLKIO);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0a78a70..6db4f8a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5011,6 +5011,75 @@ cleanup:
}
static int
+qemuDomainPcpuStats (virDomainPtr dom,
+ struct _virDomainPcpuStat *stats,
+ unsigned int nr_stats)
+{
+struct qemud_driver *driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+virCgroupPtr group = NULL;
+int ret = -1;
+unsigned long long *array = NULL;
+int i;
+
+
+qemuDriverLock(driver);
+if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUACCT)) {
+qemuDriverUnlock(driver);
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+"%s", _("cgroup CPUACCT controller is not mounted"));
+goto cleanup;
+}
+
+vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+qemuDriverUnlock(driver);
+
+if (!vm) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+virUUIDFormat(dom->uuid, uuidstr);
+qemuReportError(VIR_ERR_NO_DOMAIN,
+_("no domain with matching uuid '%s'"), uuidstr);
+goto cleanup;
+}
+
+if (VIR_ALLOC_N(array, nr_stats) < 0) {
+virReportOOMError();
+goto cleanup;
+}
+
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+"%s", _("domain is not running"));
+goto cleanup;
+}
+
+if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_("cannot find cgroup for domain %s"), vm->def->name);
+goto cleanup;
+}
+
+ret = virCgroupGetCpuacctUsagePercpu(group, nr_stats, array);
+if (ret <= 0) {
+ret = -1;
+goto cleanup;
+}
+
+for (i = 0; i < ret; i++)
+ stats[i].cpuTime = array[i];
+
+cleanup:
+if (group)
+virCgroupFree(&group);
+VIR_FREE(array);
+if (vm)
+virDomainObjUnlock(vm);
+return ret;
+}
+
+
+
+static int
qemudDomainBlockPeek (virDomainPtr dom,
const char *path,
unsigned long long offset, size_t size,
@@ -6981,7 +7050,7 @@ static virDriver qemuDriver = {
qemudDomainBlockStats, /* domainBlockStats */
qemudDomainInterfaceStats, /* domainInterfaceStats */
qemudDomainMemoryStats, /* domainMemoryStats */
-NULL, /* domainPcpuStats */
+qemuDomainPcpuStats, /* domainPcpuStats */
qemudDomainBlockPeek, /* domainBlockPeek */
qemudDomainMemoryPeek, /* domainMemoryPeek */
qemuDomainGetBlockInfo, /* domainGetBlockInfo */
--
1.7.4.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 7/8] schedinfo: Support new API for lxc driver
---
src/lxc/lxc_driver.c | 41 -
1 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 72c0a0a..5db1035 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -2244,21 +2244,28 @@ static char *lxcGetSchedulerType(virDomainPtr domain
ATTRIBUTE_UNUSED,
return schedulerType;
}
-static int lxcSetSchedulerParameters(virDomainPtr domain,
- virSchedParameterPtr params,
- int nparams)
+static int lxcSetSchedulerParametersFlags(virDomainPtr domain,
+ virSchedParameterPtr params,
+ int nparams,
+ unsigned int flags)
{
lxc_driver_t *driver = domain->conn->privateData;
int i;
virCgroupPtr group = NULL;
virDomainObjPtr vm = NULL;
+virDomainDefPtr persistentDef = NULL;
int ret = -1;
+bool isActive;
+
+virCheckFlags(VIR_DOMAIN_SCHED_PARAMS_LIVE |
+ VIR_DOMAIN_SCHED_PARAMS_PERSISTENT, -1);
if (driver->cgroup == NULL)
return -1;
lxcDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, domain->uuid);
+lxcDriverUnlock(driver);
if (vm == NULL) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -2268,9 +2275,20 @@ static int lxcSetSchedulerParameters(virDomainPtr domain,
goto cleanup;
}
+/* Could find cgroup for domain implies the domain is running. */
if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0)
goto cleanup;
+if (flags & VIR_DOMAIN_SCHED_PARAMS_PERSISTENT) {
+if (!vm->persistent) {
+lxcError(VIR_ERR_OPERATION_INVALID, "%s",
+_("cannot change persistent config of a transient
domain"));
+goto cleanup;
+}
+if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm)))
+goto cleanup;
+}
+
for (i = 0; i < nparams; i++) {
virSchedParameterPtr param = ¶ms[i];
@@ -2294,17 +2312,30 @@ static int lxcSetSchedulerParameters(virDomainPtr
domain,
}
vm->def->cputune.shares = params[i].value.ul;
+
+if (flags & VIR_DOMAIN_SCHED_PARAMS_PERSISTENT) {
+persistentDef->cputune.shares = params[i].value.ul;
+ret = virDomainSaveConfig(driver->configDir, persistentDef);
+goto cleanup;
+}
}
ret = 0;
cleanup:
-lxcDriverUnlock(driver);
virCgroupFree(&group);
if (vm)
virDomainObjUnlock(vm);
return ret;
}
+static int lxcSetSchedulerParameters(virDomainPtr domain,
+ virSchedParameterPtr params,
+ int nparams)
+{
+return lxcSetSchedulerParametersFlags(domain, params, nparams,
+ VIR_DOMAIN_SCHED_PARAMS_LIVE);
+}
+
static int lxcGetSchedulerParameters(virDomainPtr domain,
virSchedParameterPtr params,
int *nparams)
@@ -2860,7 +2891,7 @@ static virDriver lxcDriver = {
lxcGetSchedulerType, /* domainGetSchedulerType */
lxcGetSchedulerParameters, /* domainGetSchedulerParameters */
lxcSetSchedulerParameters, /* domainSetSchedulerParameters */
-NULL, /* domainSetSchedulerParametersFlags */
+lxcSetSchedulerParametersFlags, /* domainSetSchedulerParametersFlags */
NULL, /* domainMigratePrepare */
NULL, /* domainMigratePerform */
NULL, /* domainMigrateFinish */
--
1.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC] Support changing scheduler setting persistently
This patch series introduce a new API virDomainSetSchedulerParametersFlags to change the scheduler setting persistently. The new API accepts two flags, VIR_DOMAIN_SCHED_PARAMS_LIVE and VIR_DOMAIN_SCHED_PARAMS_PERSISTENT, "LIVE" flag affects the running domain config, "PERSISTENT" affects both the running and persistent domain config, both flags require the domain is running. "LIVE" flags is used by default. It's not like "setvcpus", which accepts "--live", "--config", and "--current" flags, as scheduler setting relates with cgroup (QEMU/LXC), upper layer apps probably need to use virDomainGetSchedulerParameter and virDomainSetSchedulerParametersFlags together, such as virsh, virDomainGetSchedulerParameter looks up parameters, so support changing the scheduler setting on inactive domain probably is not good idea. Regards Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/8] schedinfo: Define new API virDomainSetSchedulerParametersFlags
Flags may include VIR_DOMAIN_SCHED_PARAMS_LIVE and
VIR_DOMAIN_SCHED_PARAMS_PERSISTENT, the first one means changing
the domain scheduler setting only for running domain config, and
the later one means changing both the running and persistent domain
config.
Both flags require domain is running, VIR_DOMAIN_SCHED_PARAMS_LIVE
is used by default.
---
include/libvirt/libvirt.h.in | 14 ++
src/libvirt_public.syms |5 +
2 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 04b7fbf..c87608c 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -280,6 +280,13 @@ struct _virSchedParameter {
typedef virSchedParameter *virSchedParameterPtr;
+/* Domain scheduler parameters setting flags. */
+typedef enum {
+/* Both these two flags works on running domain. */
+VIR_DOMAIN_SCHED_PARAMS_LIVE= (1 << 0), /* Affects active domain
config */
+VIR_DOMAIN_SCHED_PARAMS_PERSISTENT = (1 << 1), /* Affects both active and
persistent domain config. */
+} virDomainSchedParametersFlags;
+
/*
* Fetch scheduler parameters, caller allocates 'params' field of size
'nparams'
*/
@@ -293,6 +300,13 @@ int virDomainGetSchedulerParameters (virDomainPtr
domain,
int virDomainSetSchedulerParameters (virDomainPtr domain,
virSchedParameterPtr params,
int nparams);
+/*
+ * Change scheduler parameters according to flags.
+ */
+int virDomainSetSchedulerParametersFlags (virDomainPtr domain,
+ virSchedParameterPtr params,
+ int nparams,
+ unsigned int flags);
/**
* virDomainBlockStats:
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index b4aed41..03d08f1 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -436,4 +436,9 @@ LIBVIRT_0.9.0 {
virStorageVolUpload;
} LIBVIRT_0.8.8;
+LIBVIRT_0.9.1 {
+global:
+virDomainSetSchedulerParametersFlags;
+} LIBVIRT_0.9.0;
+
# define new API here using predicted next version number
--
1.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/8] schedinfo: Implemente the public API
---
src/libvirt.c | 60 +
1 files changed, 60 insertions(+), 0 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 0da9885..fae83fe 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -4401,6 +4401,66 @@ error:
return -1;
}
+/**
+ * virDomainSetSchedulerParameters:
+ * @domain: pointer to domain object
+ * @params: pointer to scheduler parameter objects
+ * @nparams: number of scheduler parameter
+ * (this value should be same or less than the returned value
+ * nparams of virDomainGetSchedulerType)
+ * @flags: an OR'ed set of virDomainMemoryModFlags
+ *
+ * Change the scheduler parameters according to the specified flags
+ *
+ * @flags may include VIR_DOMAIN_SCHED_PARAMS_LIVE or
+ * VIR_DOMAIN_SCHED_PARAMS_CONFIG, both flags may be set, and both flags
+ * requires the domain is running.
+ * If VIR_DOMAIN_SCHED_PARAMS_LIVE is set, the change affects a running
+ * domain and will fail if the domain is not active.
+ * If VIR_DOMAIN_SCHED_PARAMS_CONFIG is set, the change affects both the
+ * running and persistent domain config, and will fail for transient
+ * domains or the domain is not active.
+
+ * Returns -1 in case of error, 0 in case of success.
+ */
+int
+virDomainSetSchedulerParametersFlags(virDomainPtr domain,
+ virSchedParameterPtr params,
+ int nparams,
+ unsigned int flags)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d", params, nparams);
+
+virResetLastError();
+
+if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
+virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
+virDispatchError(NULL);
+return -1;
+}
+if (domain->conn->flags & VIR_CONNECT_RO) {
+virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+goto error;
+}
+conn = domain->conn;
+
+if (conn->driver->domainSetSchedulerParametersFlags) {
+int ret;
+ret = conn->driver->domainSetSchedulerParametersFlags (domain, params,
nparams, flags);
+if (ret < 0)
+goto error;
+return ret;
+}
+
+virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
+
+error:
+virDispatchError(domain->conn);
+return -1;
+}
+
/**
* virDomainBlockStats:
--
1.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 8/8] schedinfo: Modify schedinfo command to accepts flags
---
tools/virsh.c | 26 +++---
tools/virsh.pod |8 +++-
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 2e35021..e6e80b5 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1598,6 +1598,8 @@ static const vshCmdOptDef opts_schedinfo[] = {
{"set", VSH_OT_STRING, VSH_OFLAG_NONE, N_("parameter=value")},
{"weight", VSH_OT_INT, VSH_OFLAG_NONE, N_("weight for XEN_CREDIT")},
{"cap", VSH_OT_INT, VSH_OFLAG_NONE, N_("cap for XEN_CREDIT")},
+{"live", VSH_OT_BOOL, 0, N_("affect running domain")},
+{"persistent", VSH_OT_BOOL, 0, N_("affect both running domain and next
boot")},
{NULL, 0, 0, NULL}
};
@@ -1705,6 +1707,18 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
int update = 0;
int i, ret;
int ret_val = FALSE;
+int live = vshCommandOptBool(cmd, "live");
+int persistent = vshCommandOptBool(cmd, "persistent");
+int flags = 0;
+
+if (live)
+flags |= VIR_DOMAIN_SCHED_PARAMS_LIVE;
+if (persistent)
+flags |= VIR_DOMAIN_SCHED_PARAMS_PERSISTENT;
+
+/* neither option is specified */
+if (!live && !persistent)
+flags = -1;
if (!vshConnectionUsability(ctl, ctl->conn))
return FALSE;
@@ -1714,9 +1728,10 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
/* Print SchedulerType */
schedulertype = virDomainGetSchedulerType(dom, &nparams);
-if (schedulertype!= NULL){
+
+if (schedulertype != NULL){
vshPrint(ctl, "%-15s: %s\n", _("Scheduler"),
- schedulertype);
+ schedulertype);
VIR_FREE(schedulertype);
} else {
vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), _("Unknown"));
@@ -1743,7 +1758,12 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
/* Update parameters & refresh data */
if (update) {
-ret = virDomainSetSchedulerParameters(dom, params, nparams);
+if (flags == -1)
+ret = virDomainSetSchedulerParameters(dom, params, nparams);
+else {
+ret = virDomainSetSchedulerParametersFlags(dom, params,
nparams, flags);
+}
+
if (ret == -1)
goto cleanup;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 2a708f6..8c1005b 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -567,7 +567,8 @@ severed upon restore, as TCP timeouts may have expired.
=item B optional I<--set> B I
-=item B optional I<--weight> B optional I<--cap> B
I
+=item B optional I<--weight> B optional I<--cap> B
+I I<--live> I<--persistent>
Allows you to show (and set) the domain scheduler parameters. The parameters
available for each hypervisor are:
@@ -577,6 +578,11 @@ Xen (credit scheduler): weight, cap
ESX (allocation scheduler): reservation, limit, shares
+Both I<--live> and I<--persistent> may be set, and both of them requires the
domain
+is running, I<--live> affects the running domain and fails if the domain is not
+active. I<--persistent> affects both the active and persistent domain config,
and
+fails if the domain is transient or not active. I<--live> is the default flag.
+
B: The cpu_shares parameter has a valid value range of 0-262144; Negative
values are wrapped to positive, and larger values are capped at the maximum.
Therefore, -1 is a useful shorthand for 262144.
--
1.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/8] schedinfo: Expose the new public API for Python binding
---
python/generator.py |1 +
python/libvirt-override-api.xml |7 +++
python/libvirt-override.c | 95 +++
3 files changed, 103 insertions(+), 0 deletions(-)
diff --git a/python/generator.py b/python/generator.py
index 4fa4f65..a44a4d0 100755
--- a/python/generator.py
+++ b/python/generator.py
@@ -311,6 +311,7 @@ skip_impl = (
'virDomainGetSchedulerType',
'virDomainGetSchedulerParameters',
'virDomainSetSchedulerParameters',
+'virDomainSetSchedulerParametersFlags',
'virDomainSetBlkioParameters',
'virDomainGetBlkioParameters',
'virDomainSetMemoryParameters',
diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml
index 54deeb5..490217b 100644
--- a/python/libvirt-override-api.xml
+++ b/python/libvirt-override-api.xml
@@ -162,6 +162,13 @@
+
+ Change the scheduler parameters according to flags
+
+
+
+
+
Change the blkio tunables
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 4a9b432..35e823c 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -371,7 +371,101 @@ libvirt_virDomainSetSchedulerParameters(PyObject *self
ATTRIBUTE_UNUSED,
return VIR_PY_INT_SUCCESS;
}
+static PyObject *
+libvirt_virDomainSetSchedulerParametersFlags(PyObject *self ATTRIBUTE_UNUSED,
+ PyObject *args) {
+virDomainPtr domain;
+PyObject *pyobj_domain, *info;
+char *c_retval;
+int i_retval;
+int nparams, i;
+virSchedParameterPtr params;
+unsigned int flags;
+
+if (!PyArg_ParseTuple(args, (char *)"OO:virDomainSetScedulerParameters",
+ &pyobj_domain, &info, &flags))
+return(NULL);
+domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+c_retval = virDomainGetSchedulerType(domain, &nparams);
+LIBVIRT_END_ALLOW_THREADS;
+
+if (c_retval == NULL)
+return VIR_PY_INT_FAIL;
+free(c_retval);
+
+if ((params = malloc(sizeof(*params)*nparams)) == NULL)
+return VIR_PY_INT_FAIL;
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+i_retval = virDomainGetSchedulerParameters(domain, params, &nparams);
+LIBVIRT_END_ALLOW_THREADS;
+
+if (i_retval < 0) {
+free(params);
+return VIR_PY_INT_FAIL;
+}
+
+/* convert to a Python tuple of long objects */
+for (i = 0 ; i < nparams ; i++) {
+PyObject *key, *val;
+key = libvirt_constcharPtrWrap(params[i].field);
+val = PyDict_GetItem(info, key);
+Py_DECREF(key);
+
+if (val == NULL)
+continue;
+
+switch (params[i].type) {
+case VIR_DOMAIN_SCHED_FIELD_INT:
+params[i].value.i = (int)PyInt_AS_LONG(val);
+break;
+
+case VIR_DOMAIN_SCHED_FIELD_UINT:
+params[i].value.ui = (unsigned int)PyInt_AS_LONG(val);
+break;
+case VIR_DOMAIN_SCHED_FIELD_LLONG:
+params[i].value.l = (long long)PyLong_AsLongLong(val);
+break;
+
+case VIR_DOMAIN_SCHED_FIELD_ULLONG:
+params[i].value.ul = (unsigned long long)PyLong_AsLongLong(val);
+break;
+
+case VIR_DOMAIN_SCHED_FIELD_DOUBLE:
+params[i].value.d = (double)PyFloat_AsDouble(val);
+break;
+
+case VIR_DOMAIN_SCHED_FIELD_BOOLEAN:
+{
+/* Hack - Python's definition of Py_True breaks strict
+ * aliasing rules, so can't directly compare :-(
+ */
+PyObject *hacktrue = PyBool_FromLong(1);
+params[i].value.b = hacktrue == val ? 1 : 0;
+Py_DECREF(hacktrue);
+}
+break;
+
+default:
+free(params);
+return VIR_PY_INT_FAIL;
+}
+}
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+i_retval = virDomainSetSchedulerParametersFlags(domain, params, nparams,
flags);
+LIBVIRT_END_ALLOW_THREADS;
+if (i_retval < 0) {
+free(params);
+return VIR_PY_INT_FAIL;
+}
+
+free(params);
+return VIR_PY_INT_SUCCESS;
+}
/* FIXME: This is a place holder for the implementation. */
@@ -3549,6 +3643,7 @@ static PyMethodDef libvirtMethods[] = {
{(char *) "virDomainGetSchedulerType", libvirt_virDomainGetSchedulerType,
METH_VARARGS, NULL},
{(char *) "virDomainGetSchedulerParameters",
libvirt_virDomainGetSchedulerParameters, METH_VARARGS, NULL},
{(char *) "virDomainSetSchedulerParameters",
libvirt_virDomainSetSchedulerParameters, METH_VARARGS, NULL},
+{(char *) "virDomainSetSchedulerParametersFlags",
libvirt_virDomainSetSchedulerParametersFlags, METH_VARARGS, NULL},
{(char *) "virDomainSetBlkioParameters",
libvirt_virDomainSetBlkioParameters, METH_VARARGS, NULL},
{(char *) "virDomainGetBlkioParameters",
libvirt_virDoma
[libvirt] [PATCH 2/8] schedinfo: Define the internal API
---
src/driver.h |8
src/esx/esx_driver.c |1 +
src/libxl/libxl_driver.c |1 +
src/lxc/lxc_driver.c |1 +
src/openvz/openvz_driver.c |1 +
src/phyp/phyp_driver.c |1 +
src/qemu/qemu_driver.c |1 +
src/remote/remote_driver.c |6 --
src/test/test_driver.c |1 +
src/uml/uml_driver.c |1 +
src/vbox/vbox_tmpl.c |1 +
src/vmware/vmware_driver.c |1 +
src/xen/xen_driver.c |1 +
src/xenapi/xenapi_driver.c |1 +
14 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/src/driver.h b/src/driver.h
index e5f91ca..55ffab2 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -276,6 +276,13 @@ typedef int
int nparams);
typedef int
+(*virDrvDomainSetSchedulerParametersFlags)
+ (virDomainPtr domain,
+ virSchedParameterPtr params,
+ int nparams,
+ unsigned int flags);
+
+typedef int
(*virDrvDomainBlockStats)
(virDomainPtr domain,
const char *path,
@@ -593,6 +600,7 @@ struct _virDriver {
virDrvDomainGetSchedulerType domainGetSchedulerType;
virDrvDomainGetSchedulerParameters domainGetSchedulerParameters;
virDrvDomainSetSchedulerParameters domainSetSchedulerParameters;
+virDrvDomainSetSchedulerParametersFlags
domainSetSchedulerParametersFlags;
virDrvDomainMigratePrepare domainMigratePrepare;
virDrvDomainMigratePerform domainMigratePerform;
virDrvDomainMigrateFinish domainMigrateFinish;
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index deda372..5c7bd80 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -4629,6 +4629,7 @@ static virDriver esxDriver = {
esxDomainGetSchedulerType, /* domainGetSchedulerType */
esxDomainGetSchedulerParameters, /* domainGetSchedulerParameters */
esxDomainSetSchedulerParameters, /* domainSetSchedulerParameters */
+NULL,/* domainSetSchedulerParametersFlags */
esxDomainMigratePrepare, /* domainMigratePrepare */
esxDomainMigratePerform, /* domainMigratePerform */
esxDomainMigrateFinish, /* domainMigrateFinish */
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 3040914..ef3ea46 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2617,6 +2617,7 @@ static virDriver libxlDriver = {
libxlDomainGetSchedulerType,/* domainGetSchedulerType */
libxlDomainGetSchedulerParameters,/* domainGetSchedulerParameters */
libxlDomainSetSchedulerParameters,/* domainSetSchedulerParameters */
+NULL/* domainSetSchedulerParametersFlags */
NULL, /* domainMigratePrepare */
NULL, /* domainMigratePerform */
NULL, /* domainMigrateFinish */
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index e905302..72c0a0a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -2860,6 +2860,7 @@ static virDriver lxcDriver = {
lxcGetSchedulerType, /* domainGetSchedulerType */
lxcGetSchedulerParameters, /* domainGetSchedulerParameters */
lxcSetSchedulerParameters, /* domainSetSchedulerParameters */
+NULL, /* domainSetSchedulerParametersFlags */
NULL, /* domainMigratePrepare */
NULL, /* domainMigratePerform */
NULL, /* domainMigrateFinish */
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 4af28e9..d3d3365 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -1621,6 +1621,7 @@ static virDriver openvzDriver = {
NULL, /* domainGetSchedulerType */
NULL, /* domainGetSchedulerParameters */
NULL, /* domainSetSchedulerParameters */
+NULL, /* domainSetSchedulerParametersFlags */
NULL, /* domainMigratePrepare */
NULL, /* domainMigratePerform */
NULL, /* domainMigrateFinish */
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index ddbc103..dd90fca 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -4026,6 +4026,7 @@ static virDriver phypDriver = {
NULL, /* domainGetSchedulerType */
NULL, /* domainGetSchedulerParameters */
NULL, /* domainSetSchedulerParameters */
+NULL, /* domainSetSchedulerParametersFlags */
NULL, /* domainMigratePrepare */
NULL, /* domainMigratePerform */
NULL, /* domainMigrateFinish */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 04a5f65..f06dcea 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6924,6 +6924,7 @@ static virDriver qemuDriver = {
[libvirt] [PATCH 5/8] schedinfo: Implemente the remote protocol
---
daemon/remote.c | 71
daemon/remote_dispatch_args.h |1 +
daemon/remote_dispatch_prototypes.h |8
daemon/remote_dispatch_table.h |5 ++
src/remote/remote_driver.c | 77 +--
src/remote/remote_protocol.c| 15 +++
src/remote/remote_protocol.h| 13 ++
src/remote/remote_protocol.x|9 -
8 files changed, 194 insertions(+), 5 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index dd85ef1..3ad116b 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -921,6 +921,77 @@ remoteDispatchDomainSetSchedulerParameters (struct
qemud_server *server ATTRIBUT
}
static int
+remoteDispatchDomainSetSchedulerParametersFlags (struct qemud_server *server
ATTRIBUTE_UNUSED,
+ struct qemud_client *client
ATTRIBUTE_UNUSED,
+ virConnectPtr conn,
+ remote_message_header *hdr
ATTRIBUTE_UNUSED,
+ remote_error *rerr,
+
remote_domain_set_scheduler_parameters_flags_args *args,
+ void *ret ATTRIBUTE_UNUSED)
+{
+virDomainPtr dom;
+int i, r, nparams;
+virSchedParameterPtr params;
+unsigned int flags;
+
+nparams = args->params.params_len;
+
+if (nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) {
+remoteDispatchFormatError (rerr, "%s", _("nparams too large"));
+return -1;
+}
+if (VIR_ALLOC_N(params, nparams) < 0) {
+remoteDispatchOOMError(rerr);
+return -1;
+}
+
+/* Deserialise parameters. */
+for (i = 0; i < nparams; ++i) {
+if (virStrcpyStatic(params[i].field, args->params.params_val[i].field)
== NULL) {
+remoteDispatchFormatError(rerr, _("Field %s too big for
destination"),
+ args->params.params_val[i].field);
+return -1;
+}
+params[i].type = args->params.params_val[i].value.type;
+switch (params[i].type) {
+case VIR_DOMAIN_SCHED_FIELD_INT:
+params[i].value.i =
args->params.params_val[i].value.remote_sched_param_value_u.i; break;
+case VIR_DOMAIN_SCHED_FIELD_UINT:
+params[i].value.ui =
args->params.params_val[i].value.remote_sched_param_value_u.ui; break;
+case VIR_DOMAIN_SCHED_FIELD_LLONG:
+params[i].value.l =
args->params.params_val[i].value.remote_sched_param_value_u.l; break;
+case VIR_DOMAIN_SCHED_FIELD_ULLONG:
+params[i].value.ul =
args->params.params_val[i].value.remote_sched_param_value_u.ul; break;
+case VIR_DOMAIN_SCHED_FIELD_DOUBLE:
+params[i].value.d =
args->params.params_val[i].value.remote_sched_param_value_u.d; break;
+case VIR_DOMAIN_SCHED_FIELD_BOOLEAN:
+params[i].value.b =
args->params.params_val[i].value.remote_sched_param_value_u.b; break;
+}
+}
+
+dom = get_nonnull_domain (conn, args->dom);
+if (dom == NULL) {
+VIR_FREE(params);
+remoteDispatchConnError(rerr, conn);
+return -1;
+}
+
+flags = args->flags;
+
+r = virDomainSetSchedulerParametersFlags (dom, params, nparams, flags);
+VIR_FREE(params);
+if (r == -1) {
+remoteDispatchConnError(rerr, conn);
+virDomainFree(dom);
+return -1;
+}
+virDomainFree(dom);
+
+return 0;
+}
+
+
+static int
remoteDispatchDomainBlockStats (struct qemud_server *server ATTRIBUTE_UNUSED,
struct qemud_client *client ATTRIBUTE_UNUSED,
virConnectPtr conn,
diff --git a/daemon/remote_dispatch_args.h b/daemon/remote_dispatch_args.h
index f9537d7..5cb2572 100644
--- a/daemon/remote_dispatch_args.h
+++ b/daemon/remote_dispatch_args.h
@@ -178,3 +178,4 @@
remote_domain_migrate_set_max_speed_args
val_remote_domain_migrate_set_max_speed_args;
remote_storage_vol_upload_args val_remote_storage_vol_upload_args;
remote_storage_vol_download_args val_remote_storage_vol_download_args;
+remote_domain_set_scheduler_parameters_flags_args
val_remote_domain_set_scheduler_parameters_flags_args;
diff --git a/daemon/remote_dispatch_prototypes.h
b/daemon/remote_dispatch_prototypes.h
index 18bf41d..8b249ad 100644
--- a/daemon/remote_dispatch_prototypes.h
+++ b/daemon/remote_dispatch_prototypes.h
@@ -602,6 +602,14 @@ static int remoteDispatchDomainSetSchedulerParameters(
remote_error *err,
remote_domain_set_scheduler_parameters_args *args,
void *ret);
+static int remoteDispatchDomainSetSchedulerParametersFlags(
+struct qemud_server *server,
+struct qemud_client *client,
+virConnectPtr conn,
+remote_message_header *hdr,
+remote_error *err,
+
[libvirt] [PATCH 6/8] schedinfo: Support new API for qemu driver
---
src/qemu/qemu_driver.c | 64 +++-
1 files changed, 52 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f06dcea..1c71962 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4651,37 +4651,59 @@ cleanup:
return ret;
}
-static int qemuSetSchedulerParameters(virDomainPtr dom,
- virSchedParameterPtr params,
- int nparams)
+static int qemuSetSchedulerParametersFlags(virDomainPtr dom,
+ virSchedParameterPtr params,
+ int nparams,
+ unsigned int flags)
{
struct qemud_driver *driver = dom->conn->privateData;
int i;
virCgroupPtr group = NULL;
virDomainObjPtr vm = NULL;
+virDomainDefPtr persistentDef = NULL;
int ret = -1;
-qemuDriverLock(driver);
+virCheckFlags(VIR_DOMAIN_SCHED_PARAMS_LIVE |
+ VIR_DOMAIN_SCHED_PARAMS_PERSISTENT, -1);
+
if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
qemuReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cgroup CPU controller is not mounted"));
goto cleanup;
}
+qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+qemuDriverUnlock(driver);
if (vm == NULL) {
-qemuReportError(VIR_ERR_INTERNAL_ERROR,
-_("No such domain %s"), dom->uuid);
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+virUUIDFormat(dom->uuid, uuidstr);
+qemuReportError(VIR_ERR_NO_DOMAIN,
+_("No domain with uuid: %s"), uuidstr);
goto cleanup;
}
+/* Could find the cgroup for domain implies the domain is running. */
if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
-_("cannot find cgroup for domain %s"), vm->def->name);
+ _("cannot find cgroup for domain %s"), vm->def->name);
goto cleanup;
}
+if (qemuDomainObjBeginJob(vm) < 0)
+goto cleanup;
+
+if (flags & VIR_DOMAIN_SCHED_PARAMS_PERSISTENT) {
+if (!vm->persistent) {
+qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+_("cannot change persistent config of a transient
domain"));
+goto endjob;
+}
+if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm)))
+goto endjob;
+}
+
for (i = 0; i < nparams; i++) {
virSchedParameterPtr param = ¶ms[i];
@@ -4690,25 +4712,35 @@ static int qemuSetSchedulerParameters(virDomainPtr dom,
if (param->type != VIR_DOMAIN_SCHED_FIELD_ULLONG) {
qemuReportError(VIR_ERR_INVALID_ARG, "%s",
_("invalid type for cpu_shares tunable,
expected a 'ullong'"));
-goto cleanup;
+goto endjob;
}
rc = virCgroupSetCpuShares(group, params[i].value.ul);
if (rc != 0) {
virReportSystemError(-rc, "%s",
- _("unable to set cpu shares tunable"));
-goto cleanup;
+_("unable to set cpu shares tunable"));
+goto endjob;
}
vm->def->cputune.shares = params[i].value.ul;
+
+if (flags & VIR_DOMAIN_SCHED_PARAMS_PERSISTENT) {
+persistentDef->cputune.shares = params[i].value.ul;
+ret = virDomainSaveConfig(driver->configDir, persistentDef);
+goto endjob;
+}
} else {
qemuReportError(VIR_ERR_INVALID_ARG,
_("Invalid parameter `%s'"), param->field);
-goto cleanup;
+goto endjob;
}
}
ret = 0;
+endjob:
+if (qemuDomainObjEndJob(vm) == 0)
+vm = NULL;
+
cleanup:
virCgroupFree(&group);
if (vm)
@@ -4717,6 +4749,14 @@ cleanup:
return ret;
}
+static int qemuSetSchedulerParameters(virDomainPtr dom,
+ virSchedParameterPtr params,
+ int nparams)
+{
+return qemuSetSchedulerParametersFlags(dom, params, nparams,
+ VIR_DOMAIN_SCHED_PARAMS_LIVE);
+}
+
static int qemuGetSchedulerParameters(virDomainPtr dom,
virSchedParameterPtr params,
int *nparams)
@@ -6924,7 +6964,7 @@ static virDriver qemuDriver = {
qemuGetSchedulerType, /* domainGetSchedulerType */
qemuGetSchedulerParameters, /* domainGetSchedulerParameters */
qemuSetSchedulerParameters, /* domainSetSchedulerParameters *
Re: [libvirt] [RFC][PATCHv1 2/5] libvirt - new API for getting percpu statistics of VM
2011/4/15 KAMEZAWA Hiroyuki :
> Per (host) cpu activity of VMs are very insterested numbers
> when running VMs on large SMPs. virt-top -1 mode tries to
> provide the information. (But it's not implemented.)
>
> This patch adds a libvirt interface to get per cpu statistics
> of each nodes. This patch just adds an interface and driver
> entry points. So,
> - doesn't include patches for python.
> - doesn't include any driver.
>
> Following patches will add some drivers and codes for python.
>
> Signed-off-by: KAMEZAWA Hiroyuki
> ---
> include/libvirt/libvirt.h.in | 13 ++
> src/driver.h | 6
> src/esx/esx_driver.c | 1 +
> src/libvirt.c | 55
> ++
> src/libvirt_public.syms | 4 +++
> src/libxl/libxl_driver.c | 1 +
> src/lxc/lxc_driver.c | 1 +
> src/openvz/openvz_driver.c | 1 +
> src/phyp/phyp_driver.c | 1 +
> src/qemu/qemu_driver.c | 1 +
> src/remote/remote_driver.c | 1 +
> src/test/test_driver.c | 1 +
> src/uml/uml_driver.c | 1 +
> src/vbox/vbox_tmpl.c | 1 +
> src/vmware/vmware_driver.c | 1 +
> src/xen/xen_driver.c | 1 +
> src/xenapi/xenapi_driver.c | 1 +
> 17 files changed, 91 insertions(+), 0 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 5783303..6b9292c 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -400,6 +400,13 @@ struct _virDomainMemoryStat {
>
> typedef virDomainMemoryStatStruct *virDomainMemoryStatPtr;
>
> +typedef struct _virDomainPcpuStat virDomainPcpuStatStruct;
> +
> +struct _virDomainPcpuStat {
> + unsigned long long cpuTime;
> +};
> +
NACK to adding another public struct to the API. It's not expendable.
As a stylistic nit pleas don't use the term PCPU as this looks like
"Physical CPU". Just call it virDomainPerVcpuStat at least.
Also do you really need the absolute CPU time? As noted elsewhere,
this is in fact not implementable for ESX. But ESX can provide the
virtual CPU utilization in MHz and percent.
See the virNodeGetCpuTime series [1] for a better approach.
[1] https://www.redhat.com/archives/libvir-list/2011-April/msg00702.html
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC][PATCHv1 2/5] libvirt - new API for getting percpu statistics of VM
On Fri, 15 Apr 2011 09:43:15 +0200
Matthias Bolte wrote:
> 2011/4/15 KAMEZAWA Hiroyuki :
> > Per (host) cpu activity of VMs are very insterested numbers
> > when running VMs on large SMPs. virt-top -1 mode tries to
> > provide the information. (But it's not implemented.)
> >
> > This patch adds a libvirt interface to get per cpu statistics
> > of each nodes. This patch just adds an interface and driver
> > entry points. So,
> > - doesn't include patches for python.
> > - doesn't include any driver.
> >
> > Following patches will add some drivers and codes for python.
> >
> > Signed-off-by: KAMEZAWA Hiroyuki
> > ---
> > include/libvirt/libvirt.h.in | 13 ++
> > src/driver.h | 6
> > src/esx/esx_driver.c | 1 +
> > src/libvirt.c | 55
> > ++
> > src/libvirt_public.syms | 4 +++
> > src/libxl/libxl_driver.c | 1 +
> > src/lxc/lxc_driver.c | 1 +
> > src/openvz/openvz_driver.c | 1 +
> > src/phyp/phyp_driver.c | 1 +
> > src/qemu/qemu_driver.c | 1 +
> > src/remote/remote_driver.c | 1 +
> > src/test/test_driver.c | 1 +
> > src/uml/uml_driver.c | 1 +
> > src/vbox/vbox_tmpl.c | 1 +
> > src/vmware/vmware_driver.c | 1 +
> > src/xen/xen_driver.c | 1 +
> > src/xenapi/xenapi_driver.c | 1 +
> > 17 files changed, 91 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> > index 5783303..6b9292c 100644
> > --- a/include/libvirt/libvirt.h.in
> > +++ b/include/libvirt/libvirt.h.in
> > @@ -400,6 +400,13 @@ struct _virDomainMemoryStat {
> >
> > typedef virDomainMemoryStatStruct *virDomainMemoryStatPtr;
> >
> > +typedef struct _virDomainPcpuStat virDomainPcpuStatStruct;
> > +
> > +struct _virDomainPcpuStat {
> > + unsigned long long cpuTime;
> > +};
> > +
>
> NACK to adding another public struct to the API.
Oh, yes. I searched a sutiable existing API but cannot found.
Maybe adding new enum to Usui's work will be good but I need an array.
Hmm, returning an array of unsigned long long is ok ?
> It's not expendable.
> As a stylistic nit pleas don't use the term PCPU as this looks like
> "Physical CPU". Just call it virDomainPerVcpuStat at least.
>
Ah, no, this is PerPhysicalStat
> Also do you really need the absolute CPU time?
yes, for virt-top -1.
> As noted elsewhere,
> this is in fact not implementable for ESX. But ESX can provide the
> virtual CPU utilization in MHz and percent.
>
I need physical cpu utilization by domain.
> See the virNodeGetCpuTime series [1] for a better approach.
>
> [1] https://www.redhat.com/archives/libvir-list/2011-April/msg00702.html
>
Thank you.
-Kame
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] implement BOOT_TIMEOUT
Hello folks,
here's a second try at this wrt to Eric's comments.
BOOT_TIMEOUT changed to START_DELAY, updated wording in the conf file to better
reflect what this does and added a flag to skip sleeping for the first ever
started guest.
---
tools/libvirt-guests.init.sh |7 +++
tools/libvirt-guests.sysconf |3 +++
2 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/tools/libvirt-guests.init.sh b/tools/libvirt-guests.init.sh
index f247e5e..30f957a 100644
--- a/tools/libvirt-guests.init.sh
+++ b/tools/libvirt-guests.init.sh
@@ -42,6 +42,7 @@ URIS=default
ON_BOOT=start
ON_SHUTDOWN=suspend
SHUTDOWN_TIMEOUT=0
+START_DELAY=0
test -f "$sysconfdir"/sysconfig/libvirt-guests &&
. "$sysconfdir"/sysconfig/libvirt-guests
@@ -141,6 +142,7 @@ start() {
return 0
fi
+isfirst=true
while read uri list; do
configured=false
set -f
@@ -165,6 +167,11 @@ start() {
if "$guest_running"; then
gettext "already active"; echo
else
+if "$isfirst"; then
+isfirst=false
+else
+sleep $START_DELAY
+fi
retval run_virsh "$uri" start "$name" >/dev/null && \
gettext "done"; echo
fi
diff --git a/tools/libvirt-guests.sysconf b/tools/libvirt-guests.sysconf
index cd58728..37b258e 100644
--- a/tools/libvirt-guests.sysconf
+++ b/tools/libvirt-guests.sysconf
@@ -10,6 +10,9 @@
# libvirtd
#ON_BOOT=start
+# number of seconds to wait between each guest start
+#START_DELAY=0
+
# action taken on host shutdown
# - suspend all running guests are suspended using virsh managedsave
# - shutdown all running guests are asked to shutdown. Please be careful with
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] esx: Fix gcc 4.6 warning about initialized but unused variables
Hi Matthias, On Thu, Apr 14, 2011 at 07:18:27PM +0200, Matthias Bolte wrote: > This is a speculative patch as I don't have gcc 4.6 at hand to test it. I no longer get the warning with this patch applied. At first I was worried about the use of ATTRIBUTE_UNUSED (I assumed it meant "this var is unused, feel free to optimize it away"), but gcc documentation indeed says: "unused This attribute, attached to a variable, means that the variable is meant to be possibly unused. GCC will not produce a warning for this variable." So this all looks good to me (aka ack from me) Christophe pgpZ7yNoe46Ip.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] libvirt-guests - order of stopping/starting guests
Hi all, on a previous thread [1] Eric Blake mentioned: Without more code on the libvirt front, I think you're stuck renaming the guests to enforce naming order (assuming that libvirt even goes by sorted name order, as opposed to readdir() order where you have no control). I poked around the code and experimented a little bit and found the following. * When you first create new guest (or start already created guest) it is assigned a sequential ID, name and also UUID. IDs start from 1 and increase as long as libvirtd is running. * When libvirt-guests wants to start/shutdown guests it relies on the file /var/lib/libvirt/libvirt-guests. The format of this file is: URI libvirt-guests script will start the guests as written in the file. * The libvirt-guests file is initially created from the output of `virsh list' and `virsh dominfo' (to get the UUID). virsh list will list the guests ordered by their ID. * Order of starting/stopping guests is unreliable. The only thing that matters wrt start/shutdown order is the guest ID at the time when the libvirt-guests file is generated. By default the order is the order in which guests were created and it can change if you randomly stop/start guests while libvirtd is running . Names or UUIDs don't play here at all. I'm not exactly sure what IDs are used for but they change every time when a guest is stopped/started manually. Which in turn reflects the order in which libvirt-guests stops/starts all guests. Example: On a freshly installed system I created 5 guests in the following order: a1, b1, a2, c1, a11. Then I left all guests running. Upon stop/start of the libvirt-guests service the guests were stoped/started in the same order. Then I stopped libvirt-guests and libvirtd, renamed a1 to c2 and restarted both services. c2 was first in the list - name doesn't matter. virsh list shows IDs are 1 to 5 (c2 = 1). Then I did: virsh shutdown c2 && virsh start c2. c2 now has id of 6. Doing service libvirt-guests stop/start again now showed me a different order: b1, a2, c1, a11, c2 You see how the order in which guests are started/stopped is unreliable. If I were to stop c1 while all other guests were running and then start it again (e.g. after attaching a new disk image) then it will have the highest ID number and will become last in the list used for stopping/starting guests. I'm not sure if this can easily be fixed or it needs the concept of groups/relations implemented. [1] - https://www.redhat.com/archives/libvir-list/2011-April/msg00789.html Regards, Alexander. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt 0.9.0 crashes on first start since boot
Am 14.04.2011 17:34, schrieb Laine Stump: On 04/14/2011 06:47 AM, Thomas Treutner wrote: Hi, I've upgraded to 0.9.0 today on my Debian Squeeze boxes. Everything went fine, expect on one node (and only on that one, although the setup is identical), the first start of libvirtd since boot (and again, only that start) crashes with SEGV. Here are traces from gdb: http://pastebin.com/DiZrw0S5 http://pastebin.com/eacJRv07 If I delete the PID-file and start libvirtd again, it works fine. It doesn't seem to matter when the first start since boot happens, I've deactivated startup of libvirtd at boot time. Any ideas or further infos needed? Aside from Michal's fix to the error *recovery*, the more interesting question is why this is now failing. The bits of system log we can see in the pastebin shows that dnsmasq failed with an exit code of 2. From the manpage, this means: 2 - A problem with network access occurred (address in use, attempt to use privileged ports without permission). Do you have a system instance of dnsmasq already running? (perhaps it's already listening on all interfaces) Can you send the output of "dnsmasq -v; ps -AlF | grep dnsmasq"? Indeed: root@node05:~# dnsmasq -v; ps -AlF | grep dnsmasq Dnsmasq version 2.55 Copyright (c) 2000-2010 Simon Kelley Compile time options IPv6 GNU-getopt DBus I18N DHCP TFTP This software comes with ABSOLUTELY NO WARRANTY. Dnsmasq is free software, and you are welcome to redistribute it under the terms of the GNU General Public License, version 2 or 3. 5 S dnsmasq 2647 1 0 80 0 - 5981 -856 3 14:54 ? 00:00:00 /usr/sbin/dnsmasq -x /var/run/dnsmasq/dnsmasq.pid -u dnsmasq -7 /etc/dnsmasq.d,.dpkg-dist,.dpkg-old,.dpkg-new 0 S root 3544 3473 0 80 0 - 2180 -852 3 14:56 pts/0 00:00:00 grep dnsmasq I removed dnsmasq startup from the runlevel, now it works fine. I have seen dnsmasq errors for a long time, but I didn't really care too much about, as I don't need dnsmasq and the warnings didn't stop libvrit from working. I think I'll just deinstall dnsmasq. Also, does the directory /usr/local/var/lib/libvirt/dnsmasq exist? Jep: # ls /usr/local/var/lib/libvirt/dnsmasq default.leases thanks, -t -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt 0.9.0 crashes on first start since boot
Am 14.04.2011 17:34, schrieb Laine Stump: Do you have a system instance of dnsmasq already running? PS: It was the only node where dnsmasq was installed, for whatever reason. Seems to narrow down the problem pretty good. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] LPC2011 Virtualization Micro Conf
Hi, With the success of last year's Virtualization micro-conference track at Linux Plumbers 2010, I have accepted to organize a similar track for Linux Plumbers 2011 in Santa Rosa. Please see the official Linux Plumbers 2011 website for full details about the conference: http://www.linuxplumbersconf.org/2011/ The Linux Plumbers 2011 Virtualization track is focusing on general free software Linux Virtualization. It is not reserved for a specific hypervisor, but will focus on general virtualization issues and in particular collaboration amongst projects. This would include KVM, Xen, QEMU, containers etc. Deadline: - The deadline for submissions is April 30th. Please visit the following link to submit your proposal: http://www.linuxplumbersconf.org/2011/ocw/events/LPC2011MC/proposals Example topics: --- - Kernel and Hypervisor KVM/QEMU/Xen interaction - QEMU integration, sharing of code between the different projects - IO Performance and scalability - Live Migration - Managing and supporting enterprise storage - Support for new hardware features, and/or provide guest access to these features. - Guest agents - Virtualization management tools, libvirt, etc. - Desktop integration - Consumer Electronics device emulation - Custom platform configuration and coordination with the kernel Audience: - Virtualization hypervisor developers, developers of virtualization management tools and applications, embedded virtualization developers, vendors and others. Best regards, Jes -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Merge all returns paths from dispatcher into single path
On 04/14/2011 07:28 AM, Daniel P. Berrange wrote:
> The dispatcher functions have numerous places where they
> return to the caller. This leads to duplicated cleanup
> code, often resulting in memory leaks. It makes it harder
> to ensure that errors are dispatched before freeing objects,
> which may overwrite the original error.
>
Rather than review your entire patch again, I just reviewed this interdiff.
You really did get rid of all remoteDispatchOOMError callers; I'd be
fine if you squashed in deleting the declaration from
daemon/dispatch.[ch] as part of this patch, or as a followup.
> diff --git c/daemon/remote.c w/daemon/remote.c
> index 8f4d6a6..a25c095 100644
> --- c/daemon/remote.c
> +++ w/daemon/remote.c
> @@ -912,7 +912,7 @@ remoteDispatchDomainGetSchedulerParameters(struct
> qemud_server *server ATTRIBUTE
>
> remote_domain_get_scheduler_parameters_ret *ret)
> {
> virDomainPtr dom = NULL;
> -virSchedParameterPtr params;
> +virSchedParameterPtr params = NULL;
> int i, r, nparams;
> int rv = -1;
>
> @@ -927,10 +927,8 @@ remoteDispatchDomainGetSchedulerParameters(struct
> qemud_server *server ATTRIBUTE
> virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
> goto cleanup;
> }
> -if (VIR_ALLOC_N(params, nparams) < 0) {
> -virReportOOMError();
> -goto cleanup;
> -}
> +if (VIR_ALLOC_N(params, nparams) < 0)
> +goto no_memory;
>
> dom = get_nonnull_domain(conn, args->dom);
> if (dom == NULL) {
> @@ -1002,7 +1000,7 @@ remoteDispatchDomainSetSchedulerParameters(struct
> qemud_server *server ATTRIBUTE
> {
> virDomainPtr dom = NULL;
> int i, r, nparams;
> -virSchedParameterPtr params;
> +virSchedParameterPtr params = NULL;
> int rv = -1;
>
> if (!conn) {
> @@ -1960,7 +1958,7 @@ remoteDispatchDomainGetSecurityLabel(struct
> qemud_server *server ATTRIBUTE_UNUSE
> remote_domain_get_security_label_ret
> *ret)
> {
> virDomainPtr dom = NULL;
> -virSecurityLabelPtr seclabel;
> +virSecurityLabelPtr seclabel = NULL;
> int rv = -1;
>
> if (!conn) {
> @@ -3041,7 +3039,7 @@ remoteDispatchDomainSetMemoryParameters(struct
> qemud_server *server
> {
> virDomainPtr dom = NULL;
> int i, r, nparams;
> -virMemoryParameterPtr params;
> +virMemoryParameterPtr params = NULL;
> unsigned int flags;
> int rv = -1;
>
> @@ -3142,7 +3140,7 @@ remoteDispatchDomainGetMemoryParameters(struct
> qemud_server *server
> * ret)
> {
> virDomainPtr dom = NULL;
> -virMemoryParameterPtr params;
> +virMemoryParameterPtr params = NULL;
> int i, r, nparams;
> unsigned int flags;
> int rv = -1;
> @@ -3233,8 +3231,6 @@ remoteDispatchDomainGetMemoryParameters(struct
> qemud_server *server
> success:
> rv = 0;
>
> -no_memory:
> -virReportOOMError();
> cleanup:
> if (rv < 0) {
> remoteDispatchError(rerr);
> @@ -3246,6 +3242,10 @@ cleanup:
> virDomainFree(dom);
> VIR_FREE(params);
> return rv;
> +
> +no_memory:
> +virReportOOMError();
> +goto cleanup;
> }
Good, this addresses my review comments.
>
> static int
> @@ -3262,7 +3262,7 @@ remoteDispatchDomainSetBlkioParameters(struct
> qemud_server *server
> {
> virDomainPtr dom = NULL;
> int i, r, nparams;
> -virBlkioParameterPtr params;
> +virBlkioParameterPtr params = NULL;
> unsigned int flags;
> int rv = -1;
>
> @@ -3363,7 +3363,7 @@ remoteDispatchDomainGetBlkioParameters(struct
> qemud_server *server
> * ret)
> {
> virDomainPtr dom = NULL;
> -virBlkioParameterPtr params;
> +virBlkioParameterPtr params = NULL;
> int i, r, nparams;
> unsigned int flags;
> int rv = -1;
> @@ -4846,9 +4846,8 @@ remoteDispatchAuthSaslInit(struct qemud_server *server,
> virStrerror(errno, ebuf, sizeof ebuf));
> goto error;
> }
> -if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL) {
> +if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL)
> goto error;
> -}
Looks like the cleanups from one of your followup patches leaked in here
when you reverted your sasl changes, but no real harm leaving this hunk in.
>
> /* Get remote address in form IPADDR:PORT */
> sa.len = sizeof(sa.data.stor);
> @@ -4964,7 +4963,7 @@ authfail:
> error:
> PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_SASL);
> virMutexUnlock(&client->lock);
> -goto error;
> +return -1;
> }
>
>
> @@ -5126,6 +5125,7 @@ remoteDispatchAuthSaslStart(struct qemud_server *server,
> if (serverout) {
> if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) {
> virReportOOMError();
> +remoteDispatch
Re: [libvirt] [PATCH V3 2/2] enhance processWatchdogEvent()
On 04/14/2011 09:11 PM, Wen Congyang wrote:
> This patch do the following two things:
s/do/does/
> 1. hold an extra reference while handling watchdog event
>If the domain is not persistent, and qemu quits unexpectedly before
>calling processWatchdogEvent(), vm will be freed and the function
>processWatchdogEvent() will be dangerous.
>
> 2. unlock qemu driver and vm before returning from processWatchdogEvent()
>When the function processWatchdogEvent() failed, we only free wdEvent,
>but forget to unlock qemu driver and vm, free dumpfile.
>
>
> ---
> src/qemu/qemu_driver.c | 34 ++
> src/qemu/qemu_process.c |4
> 2 files changed, 26 insertions(+), 12 deletions(-)
Looks like your v2 caught my review comments correctly. But I found one
more issue:
> +++ b/src/qemu/qemu_process.c
> @@ -428,6 +428,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon
> ATTRIBUTE_UNUSED,
> if (VIR_ALLOC(wdEvent) == 0) {
> wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
> wdEvent->vm = vm;
> +/* Hold an extra reference because we can't allow 'vm' to be
> + * deleted before handling watchdog event is finished.
> + */
> +virDomainObjRef(vm);
> ignore_value(virThreadPoolSendJob(driver->workerPool, wdEvent));
Now that we have increased the ref count, we should decrease it if we
are unable to send a job to the thread pool. That is, replace the
ignore_value() with:
if (virThreadPoolSendJob(...) < 0) {
virDomainObjUnref(vm);
VIR_FREE(wdEvent);
}
ACK with that change squashed in.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] implement BOOT_TIMEOUT
On 04/15/2011 02:57 AM, Alexander Todorov wrote: > Hello folks, > here's a second try at this wrt to Eric's comments. > BOOT_TIMEOUT changed to START_DELAY, updated wording in the conf file to > better reflect what this does and added a flag to skip sleeping for the > first ever started guest. > > > --- > tools/libvirt-guests.init.sh |7 +++ > tools/libvirt-guests.sysconf |3 +++ > 2 files changed, 10 insertions(+), 0 deletions(-) Your mailer botched the patch (it used 2 instead of 1 leading space for each context line), and 'git apply' had a hard time applying it; I had to redo it by hand. ACK and pushed. Thanks for the contribution! I added you to AUTHORS; feel free to let me know (off-list if desired) if I need to adjust your listing. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] virsh memset
On 04/14/2011 06:04 AM, Zvi Dubitzky wrote: > Hi > > Does anybody know if 'virsh memset' is working ok with its parameters > with the late libvirt code > (>= 0.8.8 )? With what libcgroup version ? 'virsh memset' doesn't depend on cgroups. It relies on the guest honoring virtio ballooning requests. Guests like Fedora are wired to do this out of the box, but not all guests support it. I'm not sure whether the virtio drivers for Windows can do ballooning. If a guest can't honor ballooning, then you can't change current memory at runtime; however, with very recent git (0.9.1), 'virsh memset --persistent' can be used to modify the persistent storage to affect the memory usage on the next guest boot. Or are you referring to 'virsh memtune', which does indeed require cgroup support, but does not require any guest interaction? -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/6] Add support for injecting NMI to guest
On 04/12/2011 10:56 PM, Lai Jiangshan wrote: > > > This patch series implements a feature of injecting NMI to guest, > which is accessible via new virDomainInjectNMI API and > 'inject-nmi' command in virsh. > > Lai Jiangshan (6): > inject-nmi: Defining the public API > inject-nmi: Defining the internal API > inject-nmi: Implementing the public API > inject-nmi: Implementing the remote protocol > inject-nmi: Expose the new API in virsh > qemu,inject-nmi: Implement the driver methods This is v2 of: https://www.redhat.com/archives/libvir-list/2011-April/msg00036.html If you use 'git format-patch --subject-prefix=PATCHv2' (or send-email instead of format-patch), it makes it easier to see that this is a resend. Thanks for splitting it up; that helps review. My quick glance at 1-5 didn't turn up any glaring problems, but I'm still not sure we've got the semantics right. I'll reply to patch 6, and think we'll need another spin to get the API right. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] qemu, inject-nmi: Implement the driver methods
On 04/12/2011 11:01 PM, Lai Jiangshan wrote:
> +++ b/src/qemu/qemu_driver.c
> @@ -1701,6 +1701,48 @@ static int qemudDomainSetMaxMemory(virDomainPtr dom,
> unsigned long memory)
> return qemudDomainSetMemoryFlags(dom, memory, VIR_DOMAIN_MEM_MAXIMUM);
> }
>
> +static int qemuDomainInjectNMI(virDomainPtr domain, unsigned int flags)
> +{
> +struct qemud_driver *driver = domain->conn->privateData;
> +virDomainObjPtr vm = NULL;
> +int ret = -1;
> +qemuDomainObjPrivatePtr priv;
> +
Right here, you should have a virCheckFlags(0, -1) to enforce that we
don't honor any flags for now. At which point, you don't need to pass
flags on down to the monitor calls.
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -2513,3 +2513,32 @@ cleanup:
>
> return ret;
> }
> +
> +int qemuMonitorJSONInjectNMI(qemuMonitorPtr mon, unsigned int flags
> ATTRIBUTE_UNUSED)
> +{
Since neither monitor needed flags this low, you don't have to propogate
it any further than qemu_driver.c's virCheckFlags().
> +int ret;
> +virJSONValuePtr cmd;
> +virJSONValuePtr reply = NULL;
> +
> +/*
> + * FIXME: qmp nmi is not supported until qemu-0.16.0,
> + * use human-monitor-command instead temporary.
> + *
> + * FIXME: qemu's nmi command just injects NMI to a specified CPU,
> + * use "nmi 0" instead temporary.
> + */
> +cmd = qemuMonitorJSONMakeCommand("human-monitor-command",
> + "s:command-line", "nmi 0",
> + NULL);
We've already got a preferred form for issuing HMP commands from JSON.
Rather than building up human-monitor-command manually, you should
instead be using qemuMonitorTextInjectNMI; for example, see how
qemuMonitorJSONDriveDel falls back to hmp. This also covers the case of
a qemu binary that has JSON but not hmp giving a more useful error message.
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -2628,3 +2628,23 @@ int qemuMonitorTextArbitraryCommand(qemuMonitorPtr
> mon, const char *cmd,
>
> return ret;
> }
> +
> +int qemuMonitorTextInjectNMI(qemuMonitorPtr mon, unsigned int flags
> ATTRIBUTE_UNUSED)
> +{
> +const char *cmd = "nmi 0";
> +char *reply = NULL;
> +
> +/*
> + * FIXME: qemu's nmi command just injects NMI to a specified CPU,
> + * use "nmi 0" instead temporary.
> + */
This bothers me. Is it possible to inject NMI to a particular CPU in
bare-metal hardware? If so, then we ought to support that in the API.
I know what Dan said:
>>> +int virDomainSendEventNMI(virDomainPtr domain, unsigned int vcpu)
>>
>> Your proposal to qemu-devel to add inject-nmi for QMP does not
>> include any CPU index parameter anymore. Instead it will automatically
>> inject the NMI to all present CPUs. This libvirt API would appear to
>> be incompatible with that QMP design. For Xen, it appears the API
>> also does not allow a CPU index to be given - it just injects to the
>> first CPU AFAICT.
>>
>> So do we really need to have a 'unsigned int vcpu' parameter in the
>> libvirt API, or can we just leave it out and always inject to
>> CPU==0 for HMP ?
>>
>> eg simplify to
>>
>> int virDomainSendNMI(virDomainPtr domain)
but if there's ever any possibility that qemu might learn how to direct
an NMI to a particular vcpu, I wonder if we should instead have:
enum {
VIR_DOMAIN_INJECT_NMI_FIRST = 1,
VIR_DOMAIN_INJECT_NMI_ALL = 2,
}
/**
* virDomainInjectNMI:
* @domain: pointer to domain object, or NULL for Domain0
* @vcpu: which vcpu to send the NMI to
* @flags: the flags for controlling behaviours
*
* Send NMI to the guest. If @flags contains
* VIR_DOMAIN_INJECT_NMI_FIRST or VIR_DOMAIN_INJECT_NMI_ALL,
* then @vcpu is ignored, and the NMI is sent to the first
* possible vcpu or to all vcpus, respectively. Otherwise,
* the NMI is sent to the specified vcpu; it is an error if
* @vcpu does not correspond to a currently online processor.
*
* Not all hypervisors support fine-tuned control over which
* vcpu(s) can be targetted, and might succeed only for a
* particular value of @flags.
*
* Returns 0 in case of success, -1 in case of failure.
*/
int virDomainInjectNMI(virDomainPtr domain, unsigned int vcpu,
unsigned int flags);
Then xen would be hardcoded to require flags==_FIRST (and always ignore
vcpu), whereas qemu can honor a particular vcpu.
Or is the first vcpu always 0? That is, are there any hypervisors that
let you offline vcpu 0 while leaving vcpu1 up, so that FIRST would imply
1? Maybe we don't need a flag for FIRST, but document that vcpu is
ignored if ALL is passed, and then make xen error out if ALL is passed
or if vcpu != 0.
That said, I haven't looked at the proposed qemu side of the patches for
how the monitor command for nmi will be implemented in the first place,
and until that is in a formal qemu release, we may still need to be a
bit flexible here on the libvirt side. Do any other hypervisors allow
NMI injection, and with what
Re: [libvirt] [PATCH] esx: Fix gcc 4.6 warning about initialized but unused variables
On 04/15/2011 03:25 AM, Christophe Fergeau wrote: > Hi Matthias, > > On Thu, Apr 14, 2011 at 07:18:27PM +0200, Matthias Bolte wrote: >> This is a speculative patch as I don't have gcc 4.6 at hand to test it. > > I no longer get the warning with this patch applied. At first I was worried > about the use of ATTRIBUTE_UNUSED (I assumed it meant "this var is unused, > feel free to optimize it away"), but gcc documentation indeed says: > "unused > This attribute, attached to a variable, means that the variable is > meant to be possibly unused. GCC will not produce a warning for this > variable." > > So this all looks good to me (aka ack from me) ACK from me as well; I went ahead and pushed it under the build-breaker rule. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/9] maint: use lighter-weight function for straight appends
2011/4/14 Eric Blake : > It costs quite a few processor cycles to go through printf parsing > just to determine that we only meant to append. > - virBufferVSprintf(&buf, "%s", k); > - virBufferVSprintf(&buf, "%s", "="); > - virBufferVSprintf(&buf, "%s", v); > + virBufferVSprintf(&buf, "%s=%s", k, v); Nice one :) ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tests: Unit tests for internal hash APIs
This is a basic set of tests for testing removals of hash entries during
iteration.
---
More tests for all other hash APIs will come on Monday.
tests/Makefile.am |8 +++-
tests/hashdata.h | 33 +++
tests/hashtest.c | 157 +
3 files changed, 197 insertions(+), 1 deletions(-)
create mode 100644 tests/hashdata.h
create mode 100644 tests/hashtest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 5896442..063ab60 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -77,7 +77,8 @@ EXTRA_DIST = \
check_PROGRAMS = virshtest conftest sockettest \
nodeinfotest qparamtest virbuftest \
- commandtest commandhelper seclabeltest
+ commandtest commandhelper seclabeltest \
+ hashtest
if WITH_XEN
check_PROGRAMS += xml2sexprtest sexpr2xmltest \
@@ -159,6 +160,7 @@ TESTS = virshtest \
sockettest \
commandtest \
seclabeltest \
+ hashtest \
$(test_scripts)
if WITH_XEN
@@ -373,6 +375,10 @@ virbuftest_SOURCES = \
virbuftest.c testutils.h testutils.c
virbuftest_LDADD = $(LDADDS)
+hashtest_SOURCES = \
+ hashtest.c hashdata.h testutils.h testutils.c
+hashtest_LDADD = $(LDADDS)
+
if WITH_LIBVIRTD
eventtest_SOURCES = \
eventtest.c testutils.h testutils.c
diff --git a/tests/hashdata.h b/tests/hashdata.h
new file mode 100644
index 000..2782255
--- /dev/null
+++ b/tests/hashdata.h
@@ -0,0 +1,33 @@
+const char *uuids[] = {
+/* [ 46] */ "f17494ba-2353-4af0-b1ba-13680858edcc",
+"64ab4e78-1b6e-4b88-b47f-2def46c79a86",
+"f99b0d59-ecff-4cc6-a9d3-20159536edc8",
+/* [ 75] */ "e1bfa30f-bc0b-4a24-99ae-bed7f3f21a7b",
+"acda5fa0-58de-4e1e-aa65-2124d1e29c54",
+/* [ 76] */ "5f476c28-8f26-48e0-98de-85745fe2f304",
+/* [123] */ "8be1d21c-cd35-4c7c-8fee-4b5046c7a62b",
+"830f0d57-9f21-40e8-bb86-cbf41de23fd6",
+"57044958-1b8a-4c02-ab75-2298c6e44263",
+"d526cd6c-4a99-4d5f-abfb-fc9419edd9d0",
+/* [237] */ "3ab39f7f-4613-4da6-a216-c2d6acc441bb",
+"ae20cf3c-38b8-483c-baea-6fb0994dc30c",
+"cd204d90-2414-4b9e-9d4f-fed09c9a816f",
+/* [240] */ "ed2cc723-db4b-43aa-ab02-0e3161087499",
+/* [246] */ "8ada85bc-9bdf-4507-8334-849635ea0a01",
+"8a7d5deb-615f-4cd3-8977-b5fab8ec4d05",
+/* [247] */ "dc2173b0-48fe-4555-b190-8052be1120eb",
+"040e434d-68d8-41a9-b3a1-1bee239914c1",
+"d1a564b2-c7f3-4b76-8712-3b8f5aae6ded",
+"0e614f33-c1da-4cfe-b6d5-65ecd2d066f2"
+};
+
+const char *uuids_subset[] = {
+"64ab4e78-1b6e-4b88-b47f-2def46c79a86",
+"acda5fa0-58de-4e1e-aa65-2124d1e29c54",
+"830f0d57-9f21-40e8-bb86-cbf41de23fd6",
+"57044958-1b8a-4c02-ab75-2298c6e44263",
+"ae20cf3c-38b8-483c-baea-6fb0994dc30c",
+"040e434d-68d8-41a9-b3a1-1bee239914c1",
+"d1a564b2-c7f3-4b76-8712-3b8f5aae6ded",
+"8ada85bc-9bdf-4507-8334-849635ea0a01"
+};
diff --git a/tests/hashtest.c b/tests/hashtest.c
new file mode 100644
index 000..dff0181
--- /dev/null
+++ b/tests/hashtest.c
@@ -0,0 +1,157 @@
+#include
+
+#include
+#include
+#include
+
+#include "internal.h"
+#include "hash.h"
+#include "hashdata.h"
+#include "testutils.h"
+
+
+#define testError(...) \
+do {\
+fprintf(stderr, __VA_ARGS__); \
+/* Pad to line up with test name ... in virTestRun */ \
+fprintf(stderr, "%74s", "... ");\
+} while (0)
+
+
+static virHashTablePtr
+testHashInit(int size)
+{
+virHashTablePtr hash;
+int i;
+
+if (!(hash = virHashCreate(size, NULL)))
+return NULL;
+
+/* entires are added in reverse order so that they will be linked in
+ * collision list in the same order as in the uuids array
+ */
+for (i = ARRAY_CARDINALITY(uuids) - 1; i >= 0; i--) {
+if (virHashAddEntry(hash, uuids[i], (void *) uuids[i]) < 0) {
+virHashFree(hash);
+return NULL;
+}
+}
+
+return hash;
+}
+
+
+static int
+testHashCheckCount(virHashTablePtr hash, int count)
+{
+if (virHashSize(hash) != count) {
+testError("\nhash contains %d instead of %d elements\n",
+ virHashSize(hash), count);
+return -1;
+}
+
+return 0;
+}
+
+
+struct testInfo {
+void *data;
+int count;
+};
+
+
+const int testHashCountRemoveForEachSome =
+ARRAY_CARDINALITY(uuids) - ARRAY_CARDINALITY(uuids_subset);
+
+static void
+testHashRemoveForEachSome(void *payload ATTRIBUTE_UNUSED,
+ const void *name,
+ void *data)
+{
+virHashTablePtr hash = data;
+int i;
+
+for (i = 0; i < ARRAY_CARDINALITY(uuids_subset); i++) {
+if (STREQ(uuids_subset[i], name)) {
+if (virHashRemoveEntry(hash, name) < 0 && virTestGetVerbose()) {
+
Re: [libvirt] [PATCHv2 2/9] phyp: avoid a logic bug
2011/4/14 Eric Blake :
> Ever since commit ebc46f, the destroy function built two command
> variants but only used one. I went with the variant that matches
> the idiom used in the counterpart of phypBuildStoragePool.
>
> * src/phyp/phyp_driver.c (phypDestroyStoragePool): Avoid
> clobbering cmd. Fix error message typo.
> ---
> src/phyp/phyp_driver.c | 11 +--
> 1 files changed, 1 insertions(+), 10 deletions(-)
>
> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index 5742d95..7aa494d 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -2936,19 +2936,10 @@ phypDestroyStoragePool(virStoragePoolPtr pool)
> return -1;
> }
> cmd = virBufferContentAndReset(&buf);
> -
> - if (virAsprintf(&cmd,
> - "viosvrcmd -m %s --id %d -c "
> - "'rmsp %s'", managed_system, vios_id,
> - pool->name) < 0) {
> - virReportOOMError();
> - goto cleanup;
> - }
> -
> ret = phypExec(session, cmd, &exit_status, conn);
>
> if (exit_status < 0) {
> - VIR_ERROR(_("Unable to create Storage Pool: %s"), ret);
> + VIR_ERROR(_("Unable to destroy Storage Pool: %s"), ret);
> goto cleanup;
> }
I just noticed that the driver uses VIR_ERROR in many places where
PHYP_ERROR should be used to report an actual libvirt API level error.
But that a problem for another patch.
ACK, to this one.
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/9] phyp: avoid memory leak on failure
2011/4/14 Eric Blake : > * src/phyp/phyp_driver.c (phypUUIDTable_Init): Avoid memory leak > on error. > --- > src/phyp/phyp_driver.c | 46 +++--- > 1 files changed, 27 insertions(+), 19 deletions(-) > ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virsh: list required options first
On Tue, Apr 12, 2011 at 15:35:06 -0600, Eric Blake wrote: > The current state of virsh parsing is that: > > all lookup the volume by path (technically, the last two also attempt > a name lookup within a pool, whereas the first skips that step, but > the end result is the same); meanwhile: Is example virsh command line missing here? > complains about unexpected data. Why? Because the --pool option is > optional, so default was parsed as the --vol argument, and > /path/to/image.img doesn't match up with any remaining options that > require an argument. Therefore, the only way to specify pool is with > an explicit "--pool" argument (you can't specify it positionally). > However, named arguments can appear in any order, so: and here > have also always worked. Therefore, this patch has no functional > change on vol-info option parsing, but only on 'virsh help vol-info' > synopsis layout. However, it also allows the next patch to 1) enforce > that required options are always first (without this patch, the next > patch would fail the testsuite), and 2) allow the user to omit the > "--pool" argument. That is, the next patch makes it possible to do: and here > instead of the longer and here as well. > * tools/virsh.c (opts_vol_create_from, opts_vol_clone) > (opts_vol_upload, opts_vol_download, opts_vol_delete) > (opts_vol_wipe, opts_vol_info, opts_vol_dumpxml, opts_vol_key) > (opts_vol_path): List optional pool parameter after required > arguments. > --- > tools/virsh.c | 20 ++-- > 1 files changed, 10 insertions(+), 10 deletions(-) Makes sense. I went through all virsh.c and didn't find more options that would need fixing. The only options which don't follow this "optional after required" rule are bool options which can safely stay where they are. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 4/9] phyp: more return handling cleanup
2011/4/14 Eric Blake : > * src/phyp/phyp_driver.c (phypInterfaceDestroy) > (phypInterfaceDefineXML, phypInterfaceLookupByName) > (phypInterfaceIsActive, phypListInterfaces, phypNumOfInterfaces): > Clean up return handling of recent additions. > --- > src/phyp/phyp_driver.c | 130 > +--- > 1 files changed, 56 insertions(+), 74 deletions(-) > ACK. Mathias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 5/9] phyp: use consistent style for labels
2011/4/14 Eric Blake : > * src/phyp/phyp_driver.c: Match label style of rest of project. > (phypExec, phypUUIDTable_Pull): Drop an extra label. > --- > src/phyp/phyp_driver.c | 129 > +++- > 1 files changed, 62 insertions(+), 67 deletions(-) > ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 6/9] phyp: prefer memcpy over memmove when legal
2011/4/14 Eric Blake : > * src/phyp/phyp_driver.c (phypUUIDTable_AddLpar) > (phypGetLparUUID, phypGetStoragePoolUUID, phypVolumeGetXMLDesc) > (phypGetStoragePoolXMLDesc): Use faster method. > --- > src/phyp/phyp_driver.c | 10 +- > 1 files changed, 5 insertions(+), 5 deletions(-) > ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 7/9] phyp: use consistent return string handling
2011/4/14 Eric Blake : > Use the name 'ret' for all phypExec results, to make it easier > to wrap phypExec. Don't allow a possibly NULL ret through printf. > > * src/phyp/phyp_driver.c (phypBuildVolume, phypDestroyStoragePool) > (phypBuildStoragePool, phypBuildLpar): Avoid NULL dereference. > (phypInterfaceDestroy): Avoid redundant free. > (phypVolumeLookupByPath, phypVolumeGetPath): Use consistent > naming. > --- > src/phyp/phyp_driver.c | 37 + > 1 files changed, 17 insertions(+), 20 deletions(-) > ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virsh: list required options first
On 04/15/2011 02:26 PM, Jiri Denemark wrote: > On Tue, Apr 12, 2011 at 15:35:06 -0600, Eric Blake wrote: >> The current state of virsh parsing is that: >> >> all lookup the volume by path (technically, the last two also attempt >> a name lookup within a pool, whereas the first skips that step, but >> the end result is the same); meanwhile: > > Is example virsh command line missing here? Aargh. I wrote my commit message by using sample command prompts: # virsh ... But # is a comment character, and ate my message (and now I don't remember it off the top of my head). I'll have to figure that out again, and use $ prompts instead... >> * tools/virsh.c (opts_vol_create_from, opts_vol_clone) >> (opts_vol_upload, opts_vol_download, opts_vol_delete) >> (opts_vol_wipe, opts_vol_info, opts_vol_dumpxml, opts_vol_key) >> (opts_vol_path): List optional pool parameter after required >> arguments. >> --- >> tools/virsh.c | 20 ++-- >> 1 files changed, 10 insertions(+), 10 deletions(-) > > Makes sense. I went through all virsh.c and didn't find more options that > would need fixing. The only options which don't follow this "optional after > required" rule are bool options which can safely stay where they are. > > ACK The review would have been much easier with a non-broken commit message, but you caught on to my drift. I'll post my revised commit message once I remember what it was... -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 8/9] phyp: avoid memory leaks in command values
2011/4/14 Eric Blake :
> * src/phyp/phyp_driver.c (phypExecBuffer): New function. Use it
> throughout file for less code, and for plugging a few leaks.
> ---
>
> Amazing how much copy-and-paste code this consolidated.
>
> src/phyp/phyp_driver.c | 803
> ++--
> 1 files changed, 92 insertions(+), 711 deletions(-)
>
> @@ -1823,17 +1706,8 @@ phypCreateServerSCSIAdapter(virConnectPtr conn)
> virBufferVSprintf(&buf, " -r prof -i 'name=%s,lpar_id=%d,"
> "\"virtual_scsi_adapters=%s,%d/server/any/any/1\"'",
> vios_name, vios_id, ret, slot);
> - if (virBufferError(&buf)) {
> - virBufferFreeAndReset(&buf);
> - virReportOOMError();
> - goto cleanup;
> - }
> -
> - VIR_FREE(cmd);
> VIR_FREE(ret);
> -
> - cmd = virBufferContentAndReset(&buf);
> - ret = phypExec(session, cmd, &exit_status, conn);
> + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
> if (exit_status < 0 || ret == NULL)
> goto cleanup;
> @@ -1847,17 +1721,7 @@ phypCreateServerSCSIAdapter(virConnectPtr conn)
> virBufferVSprintf(&buf,
> " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"",
> vios_name, slot);
> - if (virBufferError(&buf)) {
> - virBufferFreeAndReset(&buf);
> - virReportOOMError();
> - goto cleanup;
> - }
> -
> - VIR_FREE(cmd);
> - VIR_FREE(ret);
Why are you killing the VIR_FREE(ret) here, in the hunk before you
left it in. It's necessary here too.
> - cmd = virBufferContentAndReset(&buf);
> - ret = phypExec(session, cmd, &exit_status, conn);
> + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
> if (exit_status < 0 || ret == NULL)
> goto cleanup;
> @@ -2000,18 +1841,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
>
> if (system_type == HMC)
> virBufferAddChar(&buf, '\'');
> -
> - if (virBufferError(&buf)) {
> - virBufferFreeAndReset(&buf);
> - virReportOOMError();
> - goto cleanup;
> - }
> -
> - VIR_FREE(cmd);
> - VIR_FREE(ret);
Same for this VIR_FREE(ret) here, removing it produces a leak, doesn't it.
> -
> - cmd = virBufferContentAndReset(&buf);
> - ret = phypExec(session, cmd, &exit_status, conn);
> + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
> if (exit_status < 0 || ret == NULL)
> goto cleanup;
> @@ -2029,17 +1859,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
> virBufferVSprintf(&buf,
> " slot_num,backing_device|grep %s|cut -d, -f1",
> dev->data.disk->src);
> - if (virBufferError(&buf)) {
> - virBufferFreeAndReset(&buf);
> - virReportOOMError();
> - goto cleanup;
> - }
> -
> - VIR_FREE(cmd);
> - VIR_FREE(ret);
And this VIR_FREE(ret) needs to stay too.
> -
> - cmd = virBufferContentAndReset(&buf);
> - ret = phypExec(session, cmd, &exit_status, conn);
> + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
> if (exit_status < 0 || ret == NULL)
> goto cleanup;
> @@ -2057,17 +1877,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
> " -r prof --filter lpar_ids=%d,profile_names=%s"
> " -F virtual_scsi_adapters|sed -e 's/\"//g'",
> vios_id, profile);
> - if (virBufferError(&buf)) {
> - virBufferFreeAndReset(&buf);
> - virReportOOMError();
> - goto cleanup;
> - }
> -
> - VIR_FREE(cmd);
> - VIR_FREE(ret);
And this VIR_FREE(ret) needs to stay too.
> -
> - cmd = virBufferContentAndReset(&buf);
> - ret = phypExec(session, cmd, &exit_status, conn);
> + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
> if (exit_status < 0 || ret == NULL)
> goto cleanup;
> @@ -2083,17 +1893,8 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
> "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'",
> domain_name, domain->id, ret, slot,
> vios_id, vios_name);
> - if (virBufferError(&buf)) {
> - virBufferFreeAndReset(&buf);
> - virReportOOMError();
> - goto cleanup;
> - }
> -
> - VIR_FREE(cmd);
> VIR_FREE(ret);
This one is left in correctly.
> -
> - cmd = virBufferContentAndReset(&buf);
> - ret = phypExec(session, cmd, &exit_status, conn);
> + ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
> if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
> goto cleanup;
> @@ -2107,17 +1908,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
> virBufferVSprintf(&buf,
> " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"",
> domain_name, slot);
> - if (virBufferError(&buf)) {
> - virBufferFreeAndReset(&buf);
> -
Re: [libvirt] [PATCHv2 9/9] phyp: another simplification
2011/4/14 Eric Blake :
> Rather than copying and pasting lots of code, factor it into a
> single helper function.
>
> * src/phyp/phyp_driver.c (phypExecInt): New function.
> (phypGetVIOSPartitionID, phypNumDomainsGeneric, phypGetLparID)
> (phypGetLparMem, phypGetLparCPUGeneric, phypGetRemoteSlot)
> (phypGetVIOSNextSlotNumber, phypAttachDevice)
> (phypGetStoragePoolSize, phypStoragePoolNumOfVolumes)
> (phypNumOfStoragePools, phypInterfaceDestroy)
> (phypInterfaceDefineXML, phypInterfaceLookupByName)
> (phypInterfaceIsActive, phypNumOfInterfaces): Use it.
> ---
> src/phyp/phyp_driver.c | 316
> ++--
> 1 files changed, 67 insertions(+), 249 deletions(-)
>
> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index bc24b76..98d5cd6 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -228,6 +228,26 @@ phypExecBuffer(LIBSSH2_SESSION *session, virBufferPtr
> buf, int *exit_status,
> return ret;
> }
>
> +/* Convenience wrapper function */
> +static int phypExecInt(LIBSSH2_SESSION *, virBufferPtr, virConnectPtr, int *)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
> +static int
> +phypExecInt(LIBSSH2_SESSION *session, virBufferPtr buf, virConnectPtr conn,
> + int *result)
> +{
> + char *str;
> + int ret;
> +
> + str = phypExecBuffer(session, buf, &ret, conn, true);
> + if (!str || ret) {
> + VIR_FREE(str);
> + return -1;
> + }
> + ret = virStrToLong_i(str, NULL, 10, result);
You made the parsing stricter by passing NULL as second argument to
virStrToLong_i. I don't expect it but this might be possible that this
breaks the behavior of the driver.
> + VIR_FREE(str);
> + return ret;
> +}
> +
The rest of the patch looks fine.
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 9/9] phyp: another simplification
On 04/15/2011 03:01 PM, Matthias Bolte wrote:
>> +str = phypExecBuffer(session, buf, &ret, conn, true);
>> +if (!str || ret) {
>> +VIR_FREE(str);
>> +return -1;
>> +}
>> +ret = virStrToLong_i(str, NULL, 10, result);
>
> You made the parsing stricter by passing NULL as second argument to
> virStrToLong_i. I don't expect it but this might be possible that this
> breaks the behavior of the driver.
That was an intentional decision of mine (I guess I should have
documented it better), since the rest of the code was getting the
character after the parsed integer but doing nothing with it. In most
cases, it was like the code _expected_ a newline after the integer (such
as the output of a sed -c run, where that holds true), but wasn't
enforcing that expectation.
Should I modify the commit message and push with the newer strict
behavior, or modify the code to keep the older relaxed behavior (but
this time add a VIR_WARN if garbage is found after the parse)?
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv9 1/4] libvirt/qemu - persistent modification of devices.
On 04/12/2011 08:49 PM, KAMEZAWA Hiroyuki wrote:
>
> Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags()
> doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's
> at(de)tatch-device --persistent cannot modify qemu config.
> (Xen allows it.)
>
> This patch is a base patch for adding support of devices in
> step by step manner. Following patches will add some device
> support.
>
> Signed-off-by: KAMEZAWA Hiroyuki
>
> Changelog: v8->v9
> - removed unnecessary comments.
It's taken quite a few iterations, but I've finally scheduled time to
start reviewing it.
> +static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
> +const char *xml,
> +unsigned int attach,
You missed one. You changed attach and detach, but not update. This
parameter can usefully forward to all three types of updates.
In fact, I think I'd like to go one further, and refactor things further
before we start adding persistent devices.
It seems like there are two steps in all three categories of functions -
obtain the locks, then do the work. I like how you've factored things
into one function that obtains the locks then forwards on to other
functions that do the work. Furthermore, I don't see any reason why we
can't support LIVE|CONFIG at once, provided we know for which devices we
can make a successful CONFIG change.
> + * When both of MODIFY_CONFIG and MODIFY_LIVE are passed at the same
> time,
> + * return error for now. We should support this later.
> + */
> +if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
> +qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +_("cannot modify active domain and its definition "
> + "at the same time."));
> +return -1;
> +}
You are right that it's not implemented yet, but no worse than what we
already have, so it's not a regression. But I think it is doable, by
rewriting this into one giant function:
1. obtain lock
2. if CURRENT, convert to LIVE or CONFIG
3. if LIVE but not active, error
4. if CONFIG but not persistent, error
5. if CONFIG call, make temporary vmdef and modify that, or quit if that
device is not supported yet
6. if LIVE, make live modification; if that errors out, quit
7. if CONFIG, make temporary vmdef permanent (hopefully it succeeds,
because we don't roll back the LIVE portion of LIVE|CONFIG)
8. clean up locks
I'll propose a followup patch that tries to capture this idiom in code.
> +static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
> +const char *xml,
> +unsigned int flags)
> +{
> +int ret = -1;
> +
> +virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
> + VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
This causes a change in behavior. Previously, we silently ignored
VIR_DOMAIN_DEVICE_MODIFY_FORCE, now we error out. But overall, that's a
good change (FORCE only made sense for ModifyDevice, not Attach), and it
goes to show that we don't use virCheckFlags on nearly enough APIs.
> +
> +if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
> +ret = qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
> +else if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
> +ret = qemudDomainAttachDevice(dom, xml);
For example, here your logic is wrong. You covered the CONFIG|LIVE case
with qemuDomainModifyDevicePersistent (by erroring out), but you _don't_
cover the CURRENT case (which should be either CONFIG or LIVE). But to
learn if the vm is active, you have to obtain the lock, and the way
you've written this, you've already forwarded into one of the two
routines. Rather, all the public entries should forward into the one
giant helper routine which grabs the locks, checks the flags, then calls
into the right callbacks.
> @@ -4141,14 +4242,19 @@ cleanup:
>
> static int qemudDomainDetachDeviceFlags(virDomainPtr dom,
> const char *xml,
> -unsigned int flags) {
> -if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
> -qemuReportError(VIR_ERR_OPERATION_INVALID,
> -"%s", _("cannot modify the persistent configuration
> of a domain"));
> -return -1;
> -}
> +unsigned int flags)
> +{
> +int ret = -1;
> +
> +virCheckFlags((VIR_DOMAIN_DEVICE_MODIFY_CONFIG |
> + VIR_DOMAIN_DEVICE_MODIFY_LIVE), -1);
Another change in noisily rejecting FORCE, but I think it's good.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 8/9] phyp: avoid memory leaks in command values
On 04/15/2011 02:56 PM, Matthias Bolte wrote:
> 2011/4/14 Eric Blake :
>> * src/phyp/phyp_driver.c (phypExecBuffer): New function. Use it
>> throughout file for less code, and for plugging a few leaks.
>> ---
>>
>> Amazing how much copy-and-paste code this consolidated.
>>
>> " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"",
>> vios_name, slot);
>> -if (virBufferError(&buf)) {
>> -virBufferFreeAndReset(&buf);
>> -virReportOOMError();
>> -goto cleanup;
>> -}
>> -
>> -VIR_FREE(cmd);
>> -VIR_FREE(ret);
>
> Why are you killing the VIR_FREE(ret) here, in the hunk before you
> left it in. It's necessary here too.
Accident of rebasing. I had previously inserted VIR_FREE(ret) earlier
in the function, before your patch to independently fix leaks of ret,
and missed that my rebase was deleting your instances.
>
> ACK, with that VIR_FREEs left in.
I've added those back in, and pushed 1-8.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] virsh: fix regression in parsing optional integer
On Tue, Apr 12, 2011 at 15:35:07 -0600, Eric Blake wrote: > Regression introduced in 0.8.5, commit c1564268. The command > 'virsh freecell 0' quit working when it changed from an optional > string to an optional integer. > > This patch introduces a slight change that specifying an option > twice is now detected as an error. > > * tools/virsh.c (vshCmddefGetData, vshCmddefGetOption) > (vshCommandCheckOpts): Alter parameters to use bitmaps. > (vshCmddefOptParse): New function. > (vshCommandParse): Update for better handling of positional > arguments. > (vshCmddefHelp): Allow unit tests to validate options. > --- > tools/virsh.c | 149 +++- > 1 files changed, 104 insertions(+), 45 deletions(-) 100iI hate command line parsing in virsh. ^[ The code looks like it does what it's supposed to do and I guess we should be fine with the limit for 32 arguments for a single virsh command :-) If not, there's clearly something wrong about the command which would need more. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/2] tests: test recent virsh option parsing changes
On Tue, Apr 12, 2011 at 16:01:00 -0600, Eric Blake wrote: > * tests/virsh-optparse: New file. > * tests/Makefile.am (test_scripts): Use it. > --- > > Hmm, I'd better take my own advice and test this stuff :) > > tests/Makefile.am|1 + > tests/virsh-optparse | 70 > ++ > 2 files changed, 71 insertions(+), 0 deletions(-) > create mode 100755 tests/virsh-optparse And you also introduce this test for option parsing, nice. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virsh: list required options first
On 04/15/2011 02:33 PM, Eric Blake wrote: >> Is example virsh command line missing here? > > Aargh. I wrote my commit message by using sample command prompts: > > # virsh ... > > But # is a comment character, and ate my message (and now I don't > remember it off the top of my head). I'll have to figure that out > again, and use $ prompts instead... > >> ACK > > The review would have been much easier with a non-broken commit message, > but you caught on to my drift. I'll post my revised commit message once > I remember what it was... Pushed with the following commit message: commit 6b75a1a5b0d10b42e3fd344b2067a176ee294f46 Author: Eric Blake Date: Tue Apr 12 14:58:02 2011 -0600 virsh: list required options first The current state of virsh parsing is that: $ virsh vol-info /path/to/image $ virsh vol-info --pool default /path/to/image $ virsh vol-info --pool default --vol /path/to/image all lookup the volume by path (technically, the last two also attempt a name lookup within a pool, whereas the first skips that step, but the end result is the same); meanwhile: $ virsh vol-info default /path/to/image complains about unexpected data. Why? Because the --pool option is optional, so default was parsed as the --vol argument, and /path/to/image.img doesn't match up with any remaining options that require an argument. For proof, note that: $ virsh vol-info default --vol /path/to/image complains about looking up 'default' - the parser mis-associated both arguments with --vol. Given the above, the only way to specify pool is with an explicit "--pool" argument (you can't specify it positionally). However, named arguments can appear in any order, so: $ virsh vol-info /path/to/image --pool default $ virsh vol-info --vol /path/to/image --pool default have also always worked. Therefore, this patch has no functional change on vol-info option parsing, but only on 'virsh help vol-info' synopsis layout. However, it also allows the next patch to 1) enforce that required options are always first (without this patch, the next patch would fail the testsuite), and 2) allow the user to omit the "--pool" argument. That is, the next patch makes it possible to do: $ virsh vol-info /path/to/image default which to date was not possible. * tools/virsh.c (opts_vol_create_from, opts_vol_clone) (opts_vol_upload, opts_vol_download, opts_vol_delete) (opts_vol_wipe, opts_vol_info, opts_vol_dumpxml, opts_vol_key) (opts_vol_path): List optional pool parameter after required arguments. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] virsh: fix regression in parsing optional integer
On 04/15/2011 03:35 PM, Jiri Denemark wrote: > On Tue, Apr 12, 2011 at 15:35:07 -0600, Eric Blake wrote: >> Regression introduced in 0.8.5, commit c1564268. The command >> 'virsh freecell 0' quit working when it changed from an optional >> string to an optional integer. >> >> This patch introduces a slight change that specifying an option >> twice is now detected as an error. >> >> * tools/virsh.c (vshCmddefGetData, vshCmddefGetOption) >> (vshCommandCheckOpts): Alter parameters to use bitmaps. >> (vshCmddefOptParse): New function. >> (vshCommandParse): Update for better handling of positional >> arguments. >> (vshCmddefHelp): Allow unit tests to validate options. >> --- >> tools/virsh.c | 149 >> +++- >> 1 files changed, 104 insertions(+), 45 deletions(-) > > 100iI hate command line parsing in virsh. > ^[ Me too. But it should be a lot better now. > > The code looks like it does what it's supposed to do and I guess we should be > fine with the limit for 32 arguments for a single virsh command :-) If not, > there's clearly something wrong about the command which would need more. > > ACK I tweaked the commit message, then pushed: commit b9973f526cb12b8e3d751ed19fb071b4a54ea1c0 Author: Eric Blake Date: Tue Apr 12 14:42:59 2011 -0600 virsh: fix regression in parsing optional integer Regression introduced in 0.8.5, commit c1564268. The command 'virsh freecell 0' quit working when it changed from an optional string to an optional integer. This patch introduces a slight change that specifying an option twice is now detected as an error. It also changes things so that a command that has more than 1 required option will not complain about missing options if one but not all of the options were given in long format, as in 'virsh vol-create --pool p file', as well as making positional parsing work for all optional options (each positional argument is associated with the earliest option that has not yet been seen by name). Optional boolean options can appear before required argument options, because they don't affect positional argument parsing, and obviously a required boolean option makes no sense. Technically, this patch renders VSH_OT_STRING and VSH_OT_DATA redundant; but cleaning that up can be a separate patch. No command should ever need more than 32 options, right? :) * tools/virsh.c (vshCmddefGetData, vshCmddefGetOption) (vshCommandCheckOpts): Alter parameters to use bitmaps. (vshCmddefOptParse): New function. (vshCommandParse): Update for better handling of positional arguments. (vshCmddefHelp): Allow unit tests to validate options. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/2] tests: test recent virsh option parsing changes
On 04/15/2011 03:37 PM, Jiri Denemark wrote: > On Tue, Apr 12, 2011 at 16:01:00 -0600, Eric Blake wrote: >> * tests/virsh-optparse: New file. >> * tests/Makefile.am (test_scripts): Use it. >> --- >> >> Hmm, I'd better take my own advice and test this stuff :) >> >> tests/Makefile.am|1 + >> tests/virsh-optparse | 70 >> ++ >> 2 files changed, 71 insertions(+), 0 deletions(-) >> create mode 100755 tests/virsh-optparse > > And you also introduce this test for option parsing, nice. > > ACK Thanks; pushed. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Unit tests for internal hash APIs
On 04/15/2011 02:04 PM, Jiri Denemark wrote:
> This is a basic set of tests for testing removals of hash entries during
> iteration.
> ---
> More tests for all other hash APIs will come on Monday.
Such as a test that gets 8 collisions into a single bucket to force hash
table growth, or a test of custom hasher/comparator functions?
>
> tests/Makefile.am |8 +++-
> tests/hashdata.h | 33 +++
> tests/hashtest.c | 157
> +
> 3 files changed, 197 insertions(+), 1 deletions(-)
> create mode 100644 tests/hashdata.h
> create mode 100644 tests/hashtest.c
>
> +++ b/tests/hashdata.h
> @@ -0,0 +1,33 @@
> +const char *uuids[] = {
> +/* [ 46] */ "f17494ba-2353-4af0-b1ba-13680858edcc",
> +"64ab4e78-1b6e-4b88-b47f-2def46c79a86",
> +"f99b0d59-ecff-4cc6-a9d3-20159536edc8",
> +/* [ 75] */ "e1bfa30f-bc0b-4a24-99ae-bed7f3f21a7b",
> +"acda5fa0-58de-4e1e-aa65-2124d1e29c54",
> +/* [ 76] */ "5f476c28-8f26-48e0-98de-85745fe2f304",
Looks suspiciously like you used gdb to dump an existing hash structure
on a machine with lots of VMs :) Works for me.
> +static virHashTablePtr
> +testHashInit(int size)
> +{
> +virHashTablePtr hash;
> +int i;
> +
> +if (!(hash = virHashCreate(size, NULL)))
> +return NULL;
> +
> +/* entires are added in reverse order so that they will be linked in
> + * collision list in the same order as in the uuids array
We're abusing an an internal detail. So good to have the comment - if
the test breaks because of another rewrite, but the only breakage is due
to a different link order in each bucket, then I'm okay modifying the
test at that point, and meanwhile it justifies our abuse (that is, no
change needed).
> +#define DO_TEST_DATA(name, cmd, data) \
> +DO_TEST_FULL(name "(" #data ")",\
> + cmd, \
> + testHash ## cmd ## data, \
> + testHashCount ## cmd ## data)
> +
> +DO_TEST_DATA("Remove in ForEach", RemoveForEach, Some);
> +DO_TEST_DATA("Remove in ForEach", RemoveForEach, All);
> +
> +return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
Looks like a good test; it certainly would have caught the previous bugs
before the most recent hash.c fixes.
ACK.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: include esx_vi.generated.* into dist file
2011/4/15 Matthias Bolte : > 2011/4/15 Wen Congyang : >> commit d4601696 introduces two more generated files: esx_vi.generated.h >> and esx_vi.generated.h. But we do not include them into dist file. >> It will break building if using dist file to build. >> >> --- >> src/Makefile.am | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/src/Makefile.am b/src/Makefile.am >> index dce866e..1eaa7d1 100644 >> --- a/src/Makefile.am >> +++ b/src/Makefile.am >> @@ -332,7 +332,9 @@ ESX_DRIVER_GENERATED = >> \ >> esx/esx_vi_types.generated.typedef \ >> esx/esx_vi_types.generated.typeenum \ >> esx/esx_vi_types.generated.typetostring \ >> - esx/esx_vi_types.generated.typefromstring >> + esx/esx_vi_types.generated.typefromstring \ >> + esx/esx_vi.generated.c \ >> + esx/esx_vi.generated.h >> >> ESX_DRIVER_EXTRA_DIST = \ >> esx/README \ >> -- >> 1.7.1 >> > > ACK. > > Matthias > I went ahead and pushed it. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] virsh memset
Sorry , I refer to 'virsh memtune' . Do you know if it is working ok for any late combination of libvirt and libcgroup ( e.g libvirt-0.8.8 ) . I could not operate it ., thanks Zvi Dubitzky Email:d...@il.ibm.com Eric Blake wrote on 15/04/2011 20:38:55: > From: Eric Blake > To: Zvi Dubitzky/Haifa/IBM@IBMIL > Cc: libvir-list@redhat.com > Date: 15/04/2011 20:39 > Subject: Re: [libvirt] virsh memset > > On 04/14/2011 06:04 AM, Zvi Dubitzky wrote: > > Hi > > > > Does anybody know if 'virsh memset' is working ok with its parameters > > with the late libvirt code > > (>= 0.8.8 )? With what libcgroup version ? > > 'virsh memset' doesn't depend on cgroups. It relies on the guest > honoring virtio ballooning requests. Guests like Fedora are wired to do > this out of the box, but not all guests support it. I'm not sure > whether the virtio drivers for Windows can do ballooning. If a guest > can't honor ballooning, then you can't change current memory at runtime; > however, with very recent git (0.9.1), 'virsh memset --persistent' can > be used to modify the persistent storage to affect the memory usage on > the next guest boot. > > Or are you referring to 'virsh memtune', which does indeed require > cgroup support, but does not require any guest interaction? > > -- > Eric Blake ebl...@redhat.com+1-801-349-2682 > Libvirt virtualization library http://libvirt.org > > [attachment "signature.asc" deleted by Zvi Dubitzky/Haifa/IBM] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 9/9] phyp: another simplification
2011/4/14 Eric Blake :
> Rather than copying and pasting lots of code, factor it into a
> single helper function.
>
> * src/phyp/phyp_driver.c (phypExecInt): New function.
> (phypGetVIOSPartitionID, phypNumDomainsGeneric, phypGetLparID)
> (phypGetLparMem, phypGetLparCPUGeneric, phypGetRemoteSlot)
> (phypGetVIOSNextSlotNumber, phypAttachDevice)
> (phypGetStoragePoolSize, phypStoragePoolNumOfVolumes)
> (phypNumOfStoragePools, phypInterfaceDestroy)
> (phypInterfaceDefineXML, phypInterfaceLookupByName)
> (phypInterfaceIsActive, phypNumOfInterfaces): Use it.
> ---
> src/phyp/phyp_driver.c | 316
> ++--
> 1 files changed, 67 insertions(+), 249 deletions(-)
Okay lets take a look at each instance if stricter parsing is safe or not.
> @@ -267,17 +284,7 @@ phypGetVIOSPartitionID(virConnectPtr conn)
> virBufferVSprintf(&buf, " -m %s", managed_system);
> virBufferAddLit(&buf, " -r lpar -F lpar_id,lpar_env"
> "|sed -n '/vioserver/ {\n s/,.*$//\n p\n}'");
> - ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - if (exit_status < 0 || ret == NULL)
> - goto cleanup;
> -
> - if (virStrToLong_i(ret, &char_ptr, 10, &id) == -1)
> - goto cleanup;
> -
> -cleanup:
> - VIR_FREE(ret);
> -
> + phypExecInt(session, &buf, conn, &id);
> return id;
> }
In this case I'm not sure that your change is safe. The executed
command looks like it requests a table with two columns
lpar_id,lpar_env and the uses some sed construct to pick the line that
contains "vioserver". Then the lpar_id is parsed from the beginning of
that line. I'm not sure that this sed construct just returns the
number from the beginning of the line. If that is the case that your
stricter parsing it safe. But it looks like that's not that case and
ignoring content after the number in the selected line is important.
> @@ -364,17 +368,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int
> type)
> virBufferVSprintf(&buf, " -m %s", managed_system);
> virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'",
> state);
> - ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - if (exit_status < 0 || ret == NULL)
> - goto cleanup;
> -
> - if (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1)
> - goto cleanup;
> -
> -cleanup:
> - VIR_FREE(ret);
> -
> + phypExecInt(session, &buf, conn, &ndom);
> return ndom;
> }
Here the stricter parsing will be safe as the last grep returns a count.
But I wonder about the last grep. I thought it would be there to count
the number of lines that start with a number, but it doesn't work:
$ printf "aa\n22\n33\n" | grep -c '^[0-9]*'
3
I expected it to print 2 here.
$ printf "aa\n22\n33\n\n" | grep -c '^[0-9]*'
4
So the '^[0-9]*' pattern matches every line. But
$ printf "aa\n22\n33\n\n" | grep -c '^[0-9]+'
0
So the last grep here is just a wc -l.
> @@ -1298,27 +1292,14 @@ phypGetLparID(LIBSSH2_SESSION * session, const char
> *managed_system,
> {
> phyp_driverPtr phyp_driver = conn->privateData;
> int system_type = phyp_driver->system_type;
> - int exit_status = 0;
> int lpar_id = -1;
> - char *char_ptr;
> - char *ret = NULL;
> virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> virBufferAddLit(&buf, "lssyscfg -r lpar");
> if (system_type == HMC)
> virBufferVSprintf(&buf, " -m %s", managed_system);
> virBufferVSprintf(&buf, " --filter lpar_names=%s -F lpar_id", name);
> - ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - if (exit_status < 0 || ret == NULL)
> - goto cleanup;
> -
> - if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1)
> - goto cleanup;
> -
> -cleanup:
> - VIR_FREE(ret);
> -
> + phypExecInt(session, &buf, conn, &lpar_id);
> return lpar_id;
> }
This one is safe too, as the requested output is just the lpar_id.
> @@ -1397,17 +1375,7 @@ phypGetLparMem(virConnectPtr conn, const char
> *managed_system, int lpar_id,
> virBufferVSprintf(&buf,
> " -r mem --level lpar -F %s --filter lpar_ids=%d",
> type ? "curr_mem" : "curr_max_mem", lpar_id);
> - ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - if (exit_status < 0 || ret == NULL)
> - goto cleanup;
> -
> - if (virStrToLong_i(ret, &char_ptr, 10, &memory) == -1)
> - goto cleanup;
> -
> -cleanup:
> - VIR_FREE(ret);
> -
> + phypExecInt(session, &buf, conn, &memory);
> return memory;
> }
This one is safe too, as the requested output is just the curr_mem or
curr_max_mem.
> @@ -1431,17 +1396,7 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char
> *managed_system,
> virBufferVSprintf(&buf,
> " -r proc --level lpar -F %s --filter lpar_ids=%d",
> type ? "curr_max_procs" : "curr_procs", lpar_id);
> - ret = phy
Re: [libvirt] [PATCHv2 9/9] phyp: another simplification
2011/4/15 Eric Blake :
> On 04/15/2011 03:01 PM, Matthias Bolte wrote:
>>> + str = phypExecBuffer(session, buf, &ret, conn, true);
>>> + if (!str || ret) {
>>> + VIR_FREE(str);
>>> + return -1;
>>> + }
>>> + ret = virStrToLong_i(str, NULL, 10, result);
>>
>> You made the parsing stricter by passing NULL as second argument to
>> virStrToLong_i. I don't expect it but this might be possible that this
>> breaks the behavior of the driver.
>
> That was an intentional decision of mine (I guess I should have
> documented it better), since the rest of the code was getting the
> character after the parsed integer but doing nothing with it. In most
> cases, it was like the code _expected_ a newline after the integer (such
> as the output of a sed -c run, where that holds true), but wasn't
> enforcing that expectation.
>
> Should I modify the commit message and push with the newer strict
> behavior, or modify the code to keep the older relaxed behavior (but
> this time add a VIR_WARN if garbage is found after the parse)?
>
See my other mail in this thread for a detailed analysis.
Yes, lets stick to the relaxed parsing and add a VIR_WARN.
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
