3:10 a.m.
This is v7. Dropped patches for Nics and added 2 sanity checks and
show correct error messages.
=
From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Date: Fri, 25 Mar 2011 16:59:55 +0900
Subject: [PATCHv7 1/4] libvirt/qemu - persistent modification of devices.
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 <kamezawa.hiroyu(a)jp.fujitsu.com>
---
src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 125 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index af897ad..a4fb1cd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4144,16 +4144,125 @@ cleanup:
return ret;
}
-static int qemudDomainAttachDeviceFlags(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"));
+/*
+ * Attach a device given by XML, the change will be persistent
+ * and domain XML definition file is updated.
+ */
+static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
+ virDomainDeviceDefPtr newdev)
+{
+
+ switch(newdev->type) {
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Sorry, the device is not supported for now"));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef,
+ virDomainDeviceDefPtr device)
+{
+ switch(device->type) {
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Sorry, the device is not supported for now"));
return -1;
}
+ return 0;
+}
+
+static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
+ const char *xml,
+ unsigned int attach,
+ unsigned int flags)
+{
+ struct qemud_driver *driver;
+ virDomainDeviceDefPtr device;
+ virDomainDefPtr vmdef;
+ virDomainObjPtr vm;
+ int ret = -1;
+
+ /*
+ * When both of MODIFY_CONFIG and MODIFY_LIVE is specified 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",
+ _("Now, cannot modify alive domain and its definition
"
+ "at the same time."));
+ return -1;
+ }
+
+ driver = dom->conn->privateData;
+ qemuDriverLock(driver);
+ vm = virDomainFindByName(&driver->domains, dom->name);
+ if (!vm) {
+ qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name :
'%s'"),
+ dom->name);
+ goto unlock_out;
+ }
+
+ if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
+ goto unlock_out;
+
+ if (virDomainObjIsActive(vm)) {
+ /*
+ * For now, just allow updating inactive domains. Further development
+ * will allow updating both active domain and its config file at
+ * the same time.
+ */
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("unsupported to update active domain's
definition."));
+ goto endjob;
+ }
+
+ vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
- return qemudDomainAttachDevice(dom, xml);
+ if (!vmdef)
+ goto endjob;
+
+ device = virDomainDeviceDefParse(driver->caps,
+ vmdef, xml, VIR_DOMAIN_XML_INACTIVE);
+ if (!device)
+ goto endjob;
+
+ if (attach)
+ ret = qemuDomainAttachDevicePersistent(vmdef, device);
+ else
+ ret = qemuDomainDetachDevicePersistent(vmdef, device);
+
+ if (!ret) /* save the result */
+ ret = virDomainSaveConfig(driver->configDir, vmdef);
+
+ virDomainDeviceDefFree(device);
+
+endjob:
+ if (qemuDomainObjEndJob(vm) == 0)
+ vm = NULL;
+unlock_out:
+ if (vm)
+ virDomainObjUnlock(vm);
+ qemuDriverUnlock(driver);
+ return ret;
+}
+
+static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
+ const char *xml,
+ unsigned int flags)
+{
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
+ return qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
+
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
+ return qemudDomainAttachDevice(dom, xml);
+
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("bad flag: %x not supported"), flags);
+
+ return -1;
}
@@ -4368,13 +4477,16 @@ 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;
- }
- return qemudDomainDetachDevice(dom, xml);
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
+ return qemuDomainModifyDevicePersistent(dom, xml, 0, flags);
+
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
+ return qemudDomainDetachDevice(dom, xml);
+
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("bad flag: %x not supported now"), flags);
+ return -1;
}
static int qemudDomainGetAutostart(virDomainPtr dom,
--
1.7.4.1
3:14 a.m.
New subject: [libvirt] [PATCHv7 2/4] libvirt/qemu - support persistent disk modification
From 4da9bdc62bb2b26610eb3b1ea8776d90af04dabc Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Date: Fri, 25 Mar 2011 17:08:36 +0900
Subject: [PATCHv7 2/4] libvirt/qemu - support persistent disk modification
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG
for qemu.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Changelog: v6->v7
- removed error message, which seems redundunt.
(another patch will add new checks.)
- Fixed a bug which PCI address is not assigned to a new controller.
---
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
src/conf/domain_conf.h | 3 ++-
src/libvirt_private.syms | 2 ++
src/qemu/qemu_driver.c | 41 ++++++++++++++++++++++++++++++++++++++++-
4 files changed, 66 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b681dc3..3e3f342 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4859,6 +4859,19 @@ virVirtualPortProfileFormat(virBufferPtr buf,
virBufferVSprintf(buf, "%s</virtualport>\n", indent);
}
+int virDomainDiskIndexByName(virDomainDefPtr def, const char *name)
+{
+ virDomainDiskDefPtr vdisk;
+ int i;
+
+ for (i = 0; i < def->ndisks; i++) {
+ vdisk = def->disks[i];
+ if (STREQ(vdisk->dst, name))
+ return i;
+ }
+ return -1;
+}
+
int virDomainDiskInsert(virDomainDefPtr def,
virDomainDiskDefPtr disk)
{
@@ -4930,6 +4943,15 @@ void virDomainDiskRemove(virDomainDefPtr def, size_t i)
}
}
+int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name)
+{
+ int i = virDomainDiskIndexByName(def, name);
+ if (i < 0)
+ return -1;
+ virDomainDiskRemove(def, i);
+ return 0;
+}
+
int virDomainControllerInsert(virDomainDefPtr def,
virDomainControllerDefPtr controller)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1e8223f..b791714 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1255,7 +1255,7 @@ int virDomainCpuSetParse(const char **str,
int maxcpu);
char *virDomainCpuSetFormat(char *cpuset,
int maxcpu);
-
+int virDomainDiskIndexByName(virDomainDefPtr def, const char *name);
int virDomainDiskInsert(virDomainDefPtr def,
virDomainDiskDefPtr disk);
void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
@@ -1263,6 +1263,7 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def);
void virDomainDiskRemove(virDomainDefPtr def, size_t i);
+int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
int virDomainControllerInsert(virDomainDefPtr def,
virDomainControllerDefPtr controller);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b4b6c63..7459c40 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -243,11 +243,13 @@ virDomainDiskDefFree;
virDomainDiskDeviceTypeToString;
virDomainDiskErrorPolicyTypeFromString;
virDomainDiskErrorPolicyTypeToString;
+virDomainDiskIndexByName;
virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
virDomainDiskIoTypeFromString;
virDomainDiskIoTypeToString;
virDomainDiskRemove;
+virDomainDiskRemoveByName;
virDomainDiskTypeFromString;
virDomainDiskTypeToString;
virDomainFSDefFree;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a4fb1cd..4d3b416 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4144,6 +4144,14 @@ cleanup:
return ret;
}
+static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci)
+{
+ if (!pci && virDomainDefAddImplicitControllers(vmdef))
+ return -1;
+ /* added controller requires PCI address */
+ return qemuDomainAssignPCIAddresses(vmdef);
+}
+
/*
* Attach a device given by XML, the change will be persistent
* and domain XML definition file is updated.
@@ -4151,21 +4159,52 @@ cleanup:
static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
virDomainDeviceDefPtr newdev)
{
+ virDomainDiskDefPtr disk;
+ bool pci;
+ int ret;
switch(newdev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ disk = newdev->data.disk;
+ if (virDomainDiskIndexByName(vmdef, disk->dst) >= 0) {
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("target %s already exists."), disk->dst);
+ return -1;
+ }
+ if (virDomainDiskInsert(vmdef, disk)) {
+ virReportOOMError();
+ return -1;
+ }
+ pci = (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO);
+ ret = qemuDomainDeviceAddressFixup(vmdef, pci);
+ if (ret < 0)
+ virDomainDiskRemoveByName(vmdef, disk->dst);
+ else
+ newdev->data.disk = NULL;
+ break;
default:
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Sorry, the device is not supported for now"));
return -1;
}
- return 0;
+ return ret;
}
static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef,
virDomainDeviceDefPtr device)
{
+ virDomainDiskDefPtr disk;
+
switch(device->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ disk = device->data.disk;
+ if (virDomainDiskRemoveByName(vmdef, disk->dst)) {
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("no target device %s"), disk->dst);
+ return -1;
+ }
+ break;
default:
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Sorry, the device is not supported for now"));
--
1.7.4.1
3:17 a.m.
New subject: [libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Date: Fri, 25 Mar 2011 17:10:58 +0900
Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are
PCI - confliction will be found but error report is not verbose.
Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
<source file='/var/lib/libvirt/images/test3.img'/>
<target dev="sdx" bus='scsi'/>
<address type='drive' controller='0' bus='0'
unit='0'/>
</disk>
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
<source file='/var/lib/libvirt/images/test3.img'/>
<target dev="sdy" bus='scsi'/>
<address type='drive' controller='0' bus='0'
unit='0'/>
</disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
---
src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 9 +++++++
4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3e3f342..4a54f62 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def)
virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
}
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque)
+{
+ virDomainDeviceInfoPtr checked = opaque;
+ /* skip to check confliction of alias */
+ if (info->type != checked->type)
+ return 0;
+ if (info->alias && checked->alias && strcmp(info->alias,
checked->alias))
+ return -1;
+ if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
+ return -1;
+ return 0;
+}
+
+int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
+ virDomainDeviceDefPtr dev)
+{
+ virDomainDeviceInfoPtr checked;
+
+ switch (dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ checked = &dev->data.disk->info;
+ break;
+ case VIR_DOMAIN_DEVICE_FS:
+ checked = &dev->data.fs->info;
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
+ checked = &dev->data.net->info;
+ break;
+ case VIR_DOMAIN_DEVICE_INPUT:
+ checked = &dev->data.input->info;
+ break;
+ case VIR_DOMAIN_DEVICE_SOUND:
+ checked = &dev->data.sound->info;
+ break;
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ checked = &dev->data.video->info;
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ checked = &dev->data.hostdev->info;
+ break;
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ checked = &dev->data.watchdog->info;
+ break;
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ checked = &dev->data.controller->info;
+ break;
+ case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */
+ return 0;
+ default:
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Unknown device type"));
+ return -1;
+ }
+ if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
+ return 0;
+ return virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch, checked);
+}
/* Generate a string representation of a device address
* @param address Device address to stringify
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b791714..53ccf32 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1194,6 +1194,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info);
void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
void virDomainDefClearPCIAddresses(virDomainDefPtr def);
void virDomainDefClearDeviceAliases(virDomainDefPtr def);
+int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
+ virDomainDeviceDefPtr dev);
typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
virDomainDeviceInfoPtr dev,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7459c40..ffdf9cf 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -220,6 +220,7 @@ virDomainCpuSetParse;
virDomainDefAddImplicitControllers;
virDomainDefClearDeviceAliases;
virDomainDefClearPCIAddresses;
+virDomainDefFindDeviceAddressConflict;
virDomainDefFormat;
virDomainDefFree;
virDomainDefParseFile;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4d3b416..e746576 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4144,6 +4144,9 @@ cleanup:
return ret;
}
+/*
+ * Checking device's definition meets qemu's (and qemu drivre's) limitation.
+ */
static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci)
{
if (!pci && virDomainDefAddImplicitControllers(vmdef))
@@ -4163,6 +4166,12 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
bool pci;
int ret;
+ if (virDomainDefFindDeviceAddressConflict(vmdef, newdev)) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("the device address already in use"));
+ return -1;
+ }
+
switch(newdev->type) {
case VIR_DOMAIN_DEVICE_DISK:
disk = newdev->data.disk;
--
1.7.4.1
9:41 p.m.
New subject: [libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
On Fri, Mar 25, 2011 at 05:17:03PM +0900, KAMEZAWA Hiroyuki wrote:
>From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00
2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Date: Fri, 25 Mar 2011 17:10:58 +0900
Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are
PCI - confliction will be found but error report is not verbose.
Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
<source file='/var/lib/libvirt/images/test3.img'/>
<target dev="sdx" bus='scsi'/>
<address type='drive' controller='0' bus='0'
unit='0'/>
</disk>
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
<source file='/var/lib/libvirt/images/test3.img'/>
<target dev="sdy" bus='scsi'/>
<address type='drive' controller='0' bus='0'
unit='0'/>
</disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
---
src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 9 +++++++
4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3e3f342..4a54f62 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def)
virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
}
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque)
+{
+ virDomainDeviceInfoPtr checked = opaque;
+ /* skip to check confliction of alias */
+ if (info->type != checked->type)
+ return 0;
+ if (info->alias && checked->alias && strcmp(info->alias,
checked->alias))
!STREQ instead of strcmp
+ return -1;
+ if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
Is it safe to memcmp an union like this? In the cases members of an
union are of different sizes, and we intent to memcmp an union member
that has a smaller size than the other members, then data in space
not used by the union member to be compared is also compared. This is
not a desired result.
+ return -1;
+ return 0;
+}
+
--
Thanks,
Hu Tao
9:40 p.m.
New subject: [libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
On Tue, 29 Mar 2011 10:41:57 +0800
Hu Tao <hutao(a)cn.fujitsu.com> wrote:
On Fri, Mar 25, 2011 at 05:17:03PM +0900, KAMEZAWA Hiroyuki wrote:
> >From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
> Date: Fri, 25 Mar 2011 17:10:58 +0900
> Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
>
> qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
> and virDomainDefAddImplicitControllers() at the end of its call.
>
> But PCI/Drive address confliction checks are
> PCI - confliction will be found but error report is not verbose.
> Drive - never done.
>
> For example, you can add following (unusual) 2 devices without errors.
>
> <disk type='file' device='disk'>
> <driver name='qemu' type='raw'/>
> <source file='/var/lib/libvirt/images/test3.img'/>
> <target dev="sdx" bus='scsi'/>
> <address type='drive' controller='0' bus='0'
unit='0'/>
> </disk>
>
> <disk type='file' device='disk'>
> <driver name='qemu' type='raw'/>
> <source file='/var/lib/libvirt/images/test3.img'/>
> <target dev="sdy" bus='scsi'/>
> <address type='drive' controller='0' bus='0'
unit='0'/>
> </disk>
>
> It's better to check drive address confliction before addition.
>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
> ---
> src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 2 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 9 +++++++
> 4 files changed, 71 insertions(+), 0 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3e3f342..4a54f62 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def)
> virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
> }
>
> +static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED,
> + virDomainDeviceInfoPtr info,
> + void *opaque)
> +{
> + virDomainDeviceInfoPtr checked = opaque;
> + /* skip to check confliction of alias */
> + if (info->type != checked->type)
> + return 0;
> + if (info->alias && checked->alias &&
strcmp(info->alias, checked->alias))
!STREQ instead of strcmp
ok.
> + return -1;
> + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
Is it safe to memcmp an union like this? In the cases members of an
union are of different sizes, and we intent to memcmp an union member
that has a smaller size than the other members, then data in space
not used by the union member to be compared is also compared. This is
not a desired result.
As far as I checked, it's zero cleared at allocation. Hmm, making this function
bigger ?
Thanks,
-Kame
9:56 p.m.
New subject: [libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
On Tue, Mar 29, 2011 at 11:40:44AM +0900, KAMEZAWA Hiroyuki wrote:
On Tue, 29 Mar 2011 10:41:57 +0800
Hu Tao <hutao(a)cn.fujitsu.com> wrote:
> On Fri, Mar 25, 2011 at 05:17:03PM +0900, KAMEZAWA Hiroyuki wrote:
> > >From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
> > Date: Fri, 25 Mar 2011 17:10:58 +0900
> > Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before
addition.
> >
> > qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
> > and virDomainDefAddImplicitControllers() at the end of its call.
> >
> > But PCI/Drive address confliction checks are
> > PCI - confliction will be found but error report is not verbose.
> > Drive - never done.
> >
> > For example, you can add following (unusual) 2 devices without errors.
> >
> > <disk type='file' device='disk'>
> > <driver name='qemu' type='raw'/>
> > <source file='/var/lib/libvirt/images/test3.img'/>
> > <target dev="sdx" bus='scsi'/>
> > <address type='drive' controller='0' bus='0'
unit='0'/>
> > </disk>
> >
> > <disk type='file' device='disk'>
> > <driver name='qemu' type='raw'/>
> > <source file='/var/lib/libvirt/images/test3.img'/>
> > <target dev="sdy" bus='scsi'/>
> > <address type='drive' controller='0' bus='0'
unit='0'/>
> > </disk>
> >
> > It's better to check drive address confliction before addition.
> >
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
> > ---
> > src/conf/domain_conf.c | 59
++++++++++++++++++++++++++++++++++++++++++++++
> > src/conf/domain_conf.h | 2 +
> > src/libvirt_private.syms | 1 +
> > src/qemu/qemu_driver.c | 9 +++++++
> > 4 files changed, 71 insertions(+), 0 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 3e3f342..4a54f62 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr
def)
> > virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
> > }
> >
> > +static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED,
> > + virDomainDeviceInfoPtr info,
> > + void *opaque)
> > +{
> > + virDomainDeviceInfoPtr checked = opaque;
> > + /* skip to check confliction of alias */
> > + if (info->type != checked->type)
> > + return 0;
> > + if (info->alias && checked->alias &&
strcmp(info->alias, checked->alias))
>
> !STREQ instead of strcmp
>
ok.
> > + return -1;
> > + if (!memcmp(&info->addr, &checked->addr,
sizeof(info->addr)))
>
> Is it safe to memcmp an union like this? In the cases members of an
> union are of different sizes, and we intent to memcmp an union member
> that has a smaller size than the other members, then data in space
> not used by the union member to be compared is also compared. This is
> not a desired result.
As far as I checked, it's zero cleared at allocation. Hmm, making this function
bigger ?
Yes it is safe if zero-cleared. Not worth to make this function
complicated.
--
Thanks,
Hu Tao
12:09 a.m.
New subject: [libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
>From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00
2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Date: Fri, 25 Mar 2011 17:10:58 +0900
Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are
PCI - confliction will be found but error report is not verbose.
Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
<source file='/var/lib/libvirt/images/test3.img'/>
<target dev="sdx" bus='scsi'/>
<address type='drive' controller='0' bus='0'
unit='0'/>
</disk>
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
<source file='/var/lib/libvirt/images/test3.img'/>
<target dev="sdy" bus='scsi'/>
<address type='drive' controller='0' bus='0'
unit='0'/>
</disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
---
src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 9 +++++++
4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3e3f342..4a54f62 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def)
virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
}
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque)
+{
+ virDomainDeviceInfoPtr checked = opaque;
+ /* skip to check confliction of alias */
+ if (info->type != checked->type)
+ return 0;
+ if (info->alias && checked->alias && strcmp(info->alias,
checked->alias))
+ return -1;
+ if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
+ return -1;
+ return 0;
+}
+
+int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
+ virDomainDeviceDefPtr dev)
+{
+ virDomainDeviceInfoPtr checked;
+
+ switch (dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ checked = &dev->data.disk->info;
+ break;
+ case VIR_DOMAIN_DEVICE_FS:
+ checked = &dev->data.fs->info;
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
+ checked = &dev->data.net->info;
+ break;
+ case VIR_DOMAIN_DEVICE_INPUT:
+ checked = &dev->data.input->info;
+ break;
+ case VIR_DOMAIN_DEVICE_SOUND:
+ checked = &dev->data.sound->info;
+ break;
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ checked = &dev->data.video->info;
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ checked = &dev->data.hostdev->info;
+ break;
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ checked = &dev->data.watchdog->info;
+ break;
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ checked = &dev->data.controller->info;
+ break;
+ case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */
+ return 0;
+ default:
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Unknown device type"));
+ return -1;
You report error here, but you report error in caller(qemuDomainAttachDevicePersistent())
again.
+ }
+ if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
+ return 0;
+ return virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch, checked);
+}
/* Generate a string representation of a device address
* @param address Device address to stringify
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b791714..53ccf32 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1194,6 +1194,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info);
void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
void virDomainDefClearPCIAddresses(virDomainDefPtr def);
void virDomainDefClearDeviceAliases(virDomainDefPtr def);
+int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
+ virDomainDeviceDefPtr dev);
typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
virDomainDeviceInfoPtr dev,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7459c40..ffdf9cf 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -220,6 +220,7 @@ virDomainCpuSetParse;
virDomainDefAddImplicitControllers;
virDomainDefClearDeviceAliases;
virDomainDefClearPCIAddresses;
+virDomainDefFindDeviceAddressConflict;
virDomainDefFormat;
virDomainDefFree;
virDomainDefParseFile;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4d3b416..e746576 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4144,6 +4144,9 @@ cleanup:
return ret;
}
+/*
+ * Checking device's definition meets qemu's (and qemu drivre's)
limitation.
+ */
Is this comment in correct place?
You do not modify this function. If this comment is for this function, it should be
merged into patch 2.
According to the comment's content, it is not for this function.
static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool
pci)
{
if (!pci && virDomainDefAddImplicitControllers(vmdef))
@@ -4163,6 +4166,12 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr
vmdef,
bool pci;
int ret;
+ if (virDomainDefFindDeviceAddressConflict(vmdef, newdev)) {
+ qemuReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("the device address already in use"));
+ return -1;
+ }
+
switch(newdev->type) {
case VIR_DOMAIN_DEVICE_DISK:
disk = newdev->data.disk;
12:13 a.m.
New subject: [libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
On Tue, 29 Mar 2011 13:09:37 +0800
Wen Congyang <wency(a)cn.fujitsu.com> wrote:
At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
>>From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
> Date: Fri, 25 Mar 2011 17:10:58 +0900
> Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
>
> qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
> and virDomainDefAddImplicitControllers() at the end of its call.
>
> But PCI/Drive address confliction checks are
> PCI - confliction will be found but error report is not verbose.
> Drive - never done.
>
> For example, you can add following (unusual) 2 devices without errors.
>
> <disk type='file' device='disk'>
> <driver name='qemu' type='raw'/>
> <source file='/var/lib/libvirt/images/test3.img'/>
> <target dev="sdx" bus='scsi'/>
> <address type='drive' controller='0' bus='0'
unit='0'/>
> </disk>
>
> <disk type='file' device='disk'>
> <driver name='qemu' type='raw'/>
> <source file='/var/lib/libvirt/images/test3.img'/>
> <target dev="sdy" bus='scsi'/>
> <address type='drive' controller='0' bus='0'
unit='0'/>
> </disk>
>
> It's better to check drive address confliction before addition.
>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
> ---
> src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 2 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 9 +++++++
> 4 files changed, 71 insertions(+), 0 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3e3f342..4a54f62 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def)
> virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
> }
>
> +static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED,
> + virDomainDeviceInfoPtr info,
> + void *opaque)
> +{
> + virDomainDeviceInfoPtr checked = opaque;
> + /* skip to check confliction of alias */
> + if (info->type != checked->type)
> + return 0;
> + if (info->alias && checked->alias &&
strcmp(info->alias, checked->alias))
> + return -1;
> + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
> + return -1;
> + return 0;
> +}
> +
> +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
> + virDomainDeviceDefPtr dev)
> +{
> + virDomainDeviceInfoPtr checked;
> +
> + switch (dev->type) {
> + case VIR_DOMAIN_DEVICE_DISK:
> + checked = &dev->data.disk->info;
> + break;
> + case VIR_DOMAIN_DEVICE_FS:
> + checked = &dev->data.fs->info;
> + break;
> + case VIR_DOMAIN_DEVICE_NET:
> + checked = &dev->data.net->info;
> + break;
> + case VIR_DOMAIN_DEVICE_INPUT:
> + checked = &dev->data.input->info;
> + break;
> + case VIR_DOMAIN_DEVICE_SOUND:
> + checked = &dev->data.sound->info;
> + break;
> + case VIR_DOMAIN_DEVICE_VIDEO:
> + checked = &dev->data.video->info;
> + break;
> + case VIR_DOMAIN_DEVICE_HOSTDEV:
> + checked = &dev->data.hostdev->info;
> + break;
> + case VIR_DOMAIN_DEVICE_WATCHDOG:
> + checked = &dev->data.watchdog->info;
> + break;
> + case VIR_DOMAIN_DEVICE_CONTROLLER:
> + checked = &dev->data.controller->info;
> + break;
> + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */
> + return 0;
> + default:
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("Unknown device type"));
> + return -1;
You report error here, but you report error in
caller(qemuDomainAttachDevicePersistent())
again.
"unknown device type" is an internal/"should never happen" error
rathar than
errors reported in the caller.
I think this error should be reported. Internal error implies bug.
> + }
> + if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> + return 0;
> + return virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch, checked);
> +}
>
> /* Generate a string representation of a device address
> * @param address Device address to stringify
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index b791714..53ccf32 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1194,6 +1194,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info);
> void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
> void virDomainDefClearPCIAddresses(virDomainDefPtr def);
> void virDomainDefClearDeviceAliases(virDomainDefPtr def);
> +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
> + virDomainDeviceDefPtr dev);
>
> typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
> virDomainDeviceInfoPtr dev,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7459c40..ffdf9cf 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -220,6 +220,7 @@ virDomainCpuSetParse;
> virDomainDefAddImplicitControllers;
> virDomainDefClearDeviceAliases;
> virDomainDefClearPCIAddresses;
> +virDomainDefFindDeviceAddressConflict;
> virDomainDefFormat;
> virDomainDefFree;
> virDomainDefParseFile;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4d3b416..e746576 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4144,6 +4144,9 @@ cleanup:
> return ret;
> }
>
> +/*
> + * Checking device's definition meets qemu's (and qemu drivre's)
limitation.
> + */
Is this comment in correct place?
You do not modify this function. If this comment is for this function, it should be
merged into patch 2.
According to the comment's content, it is not for this function.
will check again. I'm not a good git user ;(
Thanks,
-Kame
12:30 a.m.
New subject: [libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
At 03/29/2011 01:13 PM, KAMEZAWA Hiroyuki Write:
On Tue, 29 Mar 2011 13:09:37 +0800
Wen Congyang <wency(a)cn.fujitsu.com> wrote:
> At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
>> >From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001
>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
>> Date: Fri, 25 Mar 2011 17:10:58 +0900
>> Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
>>
>> qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
>> and virDomainDefAddImplicitControllers() at the end of its call.
>>
>> But PCI/Drive address confliction checks are
>> PCI - confliction will be found but error report is not verbose.
>> Drive - never done.
>>
>> For example, you can add following (unusual) 2 devices without errors.
>>
>> <disk type='file' device='disk'>
>> <driver name='qemu' type='raw'/>
>> <source file='/var/lib/libvirt/images/test3.img'/>
>> <target dev="sdx" bus='scsi'/>
>> <address type='drive' controller='0' bus='0'
unit='0'/>
>> </disk>
>>
>> <disk type='file' device='disk'>
>> <driver name='qemu' type='raw'/>
>> <source file='/var/lib/libvirt/images/test3.img'/>
>> <target dev="sdy" bus='scsi'/>
>> <address type='drive' controller='0' bus='0'
unit='0'/>
>> </disk>
>>
>> It's better to check drive address confliction before addition.
>>
>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
>> ---
>> src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
>> src/conf/domain_conf.h | 2 +
>> src/libvirt_private.syms | 1 +
>> src/qemu/qemu_driver.c | 9 +++++++
>> 4 files changed, 71 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 3e3f342..4a54f62 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def)
>> virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
>> }
>>
>> +static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED,
>> + virDomainDeviceInfoPtr info,
>> + void *opaque)
>> +{
>> + virDomainDeviceInfoPtr checked = opaque;
>> + /* skip to check confliction of alias */
>> + if (info->type != checked->type)
>> + return 0;
>> + if (info->alias && checked->alias &&
strcmp(info->alias, checked->alias))
>> + return -1;
>> + if (!memcmp(&info->addr, &checked->addr,
sizeof(info->addr)))
>> + return -1;
>> + return 0;
>> +}
>> +
>> +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
>> + virDomainDeviceDefPtr dev)
>> +{
>> + virDomainDeviceInfoPtr checked;
>> +
>> + switch (dev->type) {
>> + case VIR_DOMAIN_DEVICE_DISK:
>> + checked = &dev->data.disk->info;
>> + break;
>> + case VIR_DOMAIN_DEVICE_FS:
>> + checked = &dev->data.fs->info;
>> + break;
>> + case VIR_DOMAIN_DEVICE_NET:
>> + checked = &dev->data.net->info;
>> + break;
>> + case VIR_DOMAIN_DEVICE_INPUT:
>> + checked = &dev->data.input->info;
>> + break;
>> + case VIR_DOMAIN_DEVICE_SOUND:
>> + checked = &dev->data.sound->info;
>> + break;
>> + case VIR_DOMAIN_DEVICE_VIDEO:
>> + checked = &dev->data.video->info;
>> + break;
>> + case VIR_DOMAIN_DEVICE_HOSTDEV:
>> + checked = &dev->data.hostdev->info;
>> + break;
>> + case VIR_DOMAIN_DEVICE_WATCHDOG:
>> + checked = &dev->data.watchdog->info;
>> + break;
>> + case VIR_DOMAIN_DEVICE_CONTROLLER:
>> + checked = &dev->data.controller->info;
>> + break;
>> + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */
>> + return 0;
>> + default:
>> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>> + "%s", _("Unknown device
type"));
>> + return -1;
>
> You report error here, but you report error in
caller(qemuDomainAttachDevicePersistent())
> again.
"unknown device type" is an internal/"should never happen" error
rathar than
errors reported in the caller.
I think this error should be reported. Internal error implies bug.
Yes, this should not happen. If it happens, the error message will be overwritten by the
caller.
We can report error here if virDomainDeviceInfoIterate() returns -1, and the caller do
not
report error.
>
>> + }
>> + if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>> + return 0;
>> + return virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch,
checked);
>> +}
>>
>> /* Generate a string representation of a device address
>> * @param address Device address to stringify
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index b791714..53ccf32 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1194,6 +1194,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info);
>> void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
>> void virDomainDefClearPCIAddresses(virDomainDefPtr def);
>> void virDomainDefClearDeviceAliases(virDomainDefPtr def);
>> +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
>> + virDomainDeviceDefPtr dev);
>>
>> typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
>> virDomainDeviceInfoPtr dev,
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 7459c40..ffdf9cf 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -220,6 +220,7 @@ virDomainCpuSetParse;
>> virDomainDefAddImplicitControllers;
>> virDomainDefClearDeviceAliases;
>> virDomainDefClearPCIAddresses;
>> +virDomainDefFindDeviceAddressConflict;
>> virDomainDefFormat;
>> virDomainDefFree;
>> virDomainDefParseFile;
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 4d3b416..e746576 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4144,6 +4144,9 @@ cleanup:
>> return ret;
>> }
>>
>> +/*
>> + * Checking device's definition meets qemu's (and qemu drivre's)
limitation.
>> + */
>
> Is this comment in correct place?
> You do not modify this function. If this comment is for this function, it should be
> merged into patch 2.
> According to the comment's content, it is not for this function.
>
will check again. I'm not a good git user ;(
Thanks,
-Kame
12:31 a.m.
New subject: [libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
On Tue, 29 Mar 2011 13:30:03 +0800
Wen Congyang <wency(a)cn.fujitsu.com> wrote:
At 03/29/2011 01:13 PM, KAMEZAWA Hiroyuki Write:
> On Tue, 29 Mar 2011 13:09:37 +0800
> Wen Congyang <wency(a)cn.fujitsu.com> wrote:
>
>> At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
>>> >From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001
>>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
>>> Date: Fri, 25 Mar 2011 17:10:58 +0900
>>> Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before
addition.
>>>
>>> qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
>>> and virDomainDefAddImplicitControllers() at the end of its call.
>>>
>>> But PCI/Drive address confliction checks are
>>> PCI - confliction will be found but error report is not verbose.
>>> Drive - never done.
>>>
>>> For example, you can add following (unusual) 2 devices without errors.
>>>
>>> <disk type='file' device='disk'>
>>> <driver name='qemu' type='raw'/>
>>> <source file='/var/lib/libvirt/images/test3.img'/>
>>> <target dev="sdx" bus='scsi'/>
>>> <address type='drive' controller='0' bus='0'
unit='0'/>
>>> </disk>
>>>
>>> <disk type='file' device='disk'>
>>> <driver name='qemu' type='raw'/>
>>> <source file='/var/lib/libvirt/images/test3.img'/>
>>> <target dev="sdy" bus='scsi'/>
>>> <address type='drive' controller='0' bus='0'
unit='0'/>
>>> </disk>
>>>
>>> It's better to check drive address confliction before addition.
>>>
>>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
>>> ---
>>> src/conf/domain_conf.c | 59
++++++++++++++++++++++++++++++++++++++++++++++
>>> src/conf/domain_conf.h | 2 +
>>> src/libvirt_private.syms | 1 +
>>> src/qemu/qemu_driver.c | 9 +++++++
>>> 4 files changed, 71 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 3e3f342..4a54f62 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr
def)
>>> virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
>>> }
>>>
>>> +static int virDomainDeviceAddressMatch(virDomainDefPtr def
ATTRIBUTE_UNUSED,
>>> + virDomainDeviceInfoPtr info,
>>> + void *opaque)
>>> +{
>>> + virDomainDeviceInfoPtr checked = opaque;
>>> + /* skip to check confliction of alias */
>>> + if (info->type != checked->type)
>>> + return 0;
>>> + if (info->alias && checked->alias &&
strcmp(info->alias, checked->alias))
>>> + return -1;
>>> + if (!memcmp(&info->addr, &checked->addr,
sizeof(info->addr)))
>>> + return -1;
>>> + return 0;
>>> +}
>>> +
>>> +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
>>> + virDomainDeviceDefPtr dev)
>>> +{
>>> + virDomainDeviceInfoPtr checked;
>>> +
>>> + switch (dev->type) {
>>> + case VIR_DOMAIN_DEVICE_DISK:
>>> + checked = &dev->data.disk->info;
>>> + break;
>>> + case VIR_DOMAIN_DEVICE_FS:
>>> + checked = &dev->data.fs->info;
>>> + break;
>>> + case VIR_DOMAIN_DEVICE_NET:
>>> + checked = &dev->data.net->info;
>>> + break;
>>> + case VIR_DOMAIN_DEVICE_INPUT:
>>> + checked = &dev->data.input->info;
>>> + break;
>>> + case VIR_DOMAIN_DEVICE_SOUND:
>>> + checked = &dev->data.sound->info;
>>> + break;
>>> + case VIR_DOMAIN_DEVICE_VIDEO:
>>> + checked = &dev->data.video->info;
>>> + break;
>>> + case VIR_DOMAIN_DEVICE_HOSTDEV:
>>> + checked = &dev->data.hostdev->info;
>>> + break;
>>> + case VIR_DOMAIN_DEVICE_WATCHDOG:
>>> + checked = &dev->data.watchdog->info;
>>> + break;
>>> + case VIR_DOMAIN_DEVICE_CONTROLLER:
>>> + checked = &dev->data.controller->info;
>>> + break;
>>> + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */
>>> + return 0;
>>> + default:
>>> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>>> + "%s", _("Unknown device
type"));
>>> + return -1;
>>
>> You report error here, but you report error in
caller(qemuDomainAttachDevicePersistent())
>> again.
>
> "unknown device type" is an internal/"should never happen" error
rathar than
> errors reported in the caller.
>
> I think this error should be reported. Internal error implies bug.
Yes, this should not happen. If it happens, the error message will be overwritten by the
caller.
We can report error here if virDomainDeviceInfoIterate() returns -1, and the caller do
not
report error.
Is that make usability of this function ? If we report error, this function cannot
be used for simple sanity check of pci addresses.
Hmm, I'll replace this with DEBUG message. ok and never add error report here.
Ok ?
Thanks,
-Kame
12:43 a.m.
New subject: [libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
At 03/29/2011 01:31 PM, KAMEZAWA Hiroyuki Write:
On Tue, 29 Mar 2011 13:30:03 +0800
Wen Congyang <wency(a)cn.fujitsu.com> wrote:
> At 03/29/2011 01:13 PM, KAMEZAWA Hiroyuki Write:
>> On Tue, 29 Mar 2011 13:09:37 +0800
>> Wen Congyang <wency(a)cn.fujitsu.com> wrote:
>>
>>> At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
>>>> >From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00
2001
>>>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
>>>> Date: Fri, 25 Mar 2011 17:10:58 +0900
>>>> Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before
addition.
>>>>
>>>> qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
>>>> and virDomainDefAddImplicitControllers() at the end of its call.
>>>>
>>>> But PCI/Drive address confliction checks are
>>>> PCI - confliction will be found but error report is not verbose.
>>>> Drive - never done.
>>>>
>>>> For example, you can add following (unusual) 2 devices without errors.
>>>>
>>>> <disk type='file' device='disk'>
>>>> <driver name='qemu' type='raw'/>
>>>> <source file='/var/lib/libvirt/images/test3.img'/>
>>>> <target dev="sdx" bus='scsi'/>
>>>> <address type='drive' controller='0'
bus='0' unit='0'/>
>>>> </disk>
>>>>
>>>> <disk type='file' device='disk'>
>>>> <driver name='qemu' type='raw'/>
>>>> <source file='/var/lib/libvirt/images/test3.img'/>
>>>> <target dev="sdy" bus='scsi'/>
>>>> <address type='drive' controller='0'
bus='0' unit='0'/>
>>>> </disk>
>>>>
>>>> It's better to check drive address confliction before addition.
>>>>
>>>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
>>>> ---
>>>> src/conf/domain_conf.c | 59
++++++++++++++++++++++++++++++++++++++++++++++
>>>> src/conf/domain_conf.h | 2 +
>>>> src/libvirt_private.syms | 1 +
>>>> src/qemu/qemu_driver.c | 9 +++++++
>>>> 4 files changed, 71 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>> index 3e3f342..4a54f62 100644
>>>> --- a/src/conf/domain_conf.c
>>>> +++ b/src/conf/domain_conf.c
>>>> @@ -1287,6 +1287,65 @@ void
virDomainDefClearDeviceAliases(virDomainDefPtr def)
>>>> virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias,
NULL);
>>>> }
>>>>
>>>> +static int virDomainDeviceAddressMatch(virDomainDefPtr def
ATTRIBUTE_UNUSED,
>>>> + virDomainDeviceInfoPtr info,
>>>> + void *opaque)
>>>> +{
>>>> + virDomainDeviceInfoPtr checked = opaque;
>>>> + /* skip to check confliction of alias */
>>>> + if (info->type != checked->type)
>>>> + return 0;
>>>> + if (info->alias && checked->alias &&
strcmp(info->alias, checked->alias))
>>>> + return -1;
>>>> + if (!memcmp(&info->addr, &checked->addr,
sizeof(info->addr)))
>>>> + return -1;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
>>>> + virDomainDeviceDefPtr dev)
>>>> +{
>>>> + virDomainDeviceInfoPtr checked;
>>>> +
>>>> + switch (dev->type) {
>>>> + case VIR_DOMAIN_DEVICE_DISK:
>>>> + checked = &dev->data.disk->info;
>>>> + break;
>>>> + case VIR_DOMAIN_DEVICE_FS:
>>>> + checked = &dev->data.fs->info;
>>>> + break;
>>>> + case VIR_DOMAIN_DEVICE_NET:
>>>> + checked = &dev->data.net->info;
>>>> + break;
>>>> + case VIR_DOMAIN_DEVICE_INPUT:
>>>> + checked = &dev->data.input->info;
>>>> + break;
>>>> + case VIR_DOMAIN_DEVICE_SOUND:
>>>> + checked = &dev->data.sound->info;
>>>> + break;
>>>> + case VIR_DOMAIN_DEVICE_VIDEO:
>>>> + checked = &dev->data.video->info;
>>>> + break;
>>>> + case VIR_DOMAIN_DEVICE_HOSTDEV:
>>>> + checked = &dev->data.hostdev->info;
>>>> + break;
>>>> + case VIR_DOMAIN_DEVICE_WATCHDOG:
>>>> + checked = &dev->data.watchdog->info;
>>>> + break;
>>>> + case VIR_DOMAIN_DEVICE_CONTROLLER:
>>>> + checked = &dev->data.controller->info;
>>>> + break;
>>>> + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */
>>>> + return 0;
>>>> + default:
>>>> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + "%s", _("Unknown device
type"));
>>>> + return -1;
>>>
>>> You report error here, but you report error in
caller(qemuDomainAttachDevicePersistent())
>>> again.
>>
>> "unknown device type" is an internal/"should never happen"
error rathar than
>> errors reported in the caller.
>>
>> I think this error should be reported. Internal error implies bug.
>
> Yes, this should not happen. If it happens, the error message will be overwritten by
the caller.
> We can report error here if virDomainDeviceInfoIterate() returns -1, and the caller
do not
> report error.
>
Is that make usability of this function ? If we report error, this function cannot
be used for simple sanity check of pci addresses.
Hmm, I'll replace this with DEBUG message. ok and never add error report here.
Ok ?
Sounds good. But use VIR_ERR instead of VIR_DEBUG, as it implies a bug.
Thanks,
-Kame
3:19 a.m.
New subject: [libvirt] [PATCHv7 4/4] libvirt/qemu - verify new device at attaching persistent.
From 8423316cbd8e4efa4240cc73ac65955d87cb9921 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Date: Fri, 25 Mar 2011 17:13:05 +0900
Subject: [PATCHv7 4/4] libvirt/qemu - verify new device at attaching persistent
When adding a device by attach-device in --persistent mode,
the user can pass invalid devices. For example,
XX
<disk type='file' device='disk'>
<driver name='qemu' type='qcow2'/>
<source file='/var/lib/libvirt/images/test3.img'/>
<target dev='hde' bus='ide'/>
</disk>
XX
When adding this, 2nd IDE controller will be added but
qemu only supports 1 controller. It's better to check this kinds
of qemu/qemu_driver limitation.
(This can be added by virsh edit...)
Check we can make a command line or not at adding device will
be a consistent verification with future qemu driver updates.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
---
src/qemu/qemu_driver.c | 35 +++++++++++++++++++++++++++++++++++
1 files changed, 35 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e746576..482f762 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4156,6 +4156,38 @@ static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool
pci)
}
/*
+ * Check the device definition meet's qemu/qemu_drriver's requirements.
+ */
+static int qemuDeviceVerifyDefinition(virDomainDefPtr vmdef,
+ virDomainDeviceDefPtr dev)
+{
+ virBitmapPtr qemuCaps = NULL;
+ int ret = -1;
+ char *optstr;
+
+ if (qemuCapsExtractVersionInfo(vmdef->emulator, vmdef->os.arch,
+ NULL,
+ &qemuCaps))
+ goto out;
+ switch(dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ /* for verify, check we can create qemu command line or not */
+ optstr = qemuBuildDriveStr(dev->data.disk, 0, qemuCaps);
+ if (!optstr)
+ goto free_out;
+ ret = 0;
+ VIR_FREE(optstr);
+ /* now, don't need to check qemuBuildDriveDevStr() */
+ break;
+ default:
+ break;
+ }
+free_out:
+ qemuCapsFree(qemuCaps);
+out:
+ return ret;
+}
+/*
* Attach a device given by XML, the change will be persistent
* and domain XML definition file is updated.
*/
@@ -4166,6 +4198,9 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
bool pci;
int ret;
+ if (qemuDeviceVerifyDefinition(vmdef, newdev))
+ return -1;
+
if (virDomainDefFindDeviceAddressConflict(vmdef, newdev)) {
qemuReportError(VIR_ERR_INVALID_ARG, "%s",
_("the device address already in use"));
--
1.7.4.1
9:44 p.m.
On Fri, Mar 25, 2011 at 05:10:12PM +0900, KAMEZAWA Hiroyuki wrote:
This is v7. Dropped patches for Nics and added 2 sanity checks and
show correct error messages.
This series looks good to me(except some minor problems in patch 3).
Eric, will you have some time to have a review on this?
--
Thanks,
Hu Tao
10:24 p.m.
At 03/25/2011 04:10 PM, KAMEZAWA Hiroyuki Write:
This is v7. Dropped patches for Nics and added 2 sanity checks and
show correct error messages.
=
>From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
Date: Fri, 25 Mar 2011 16:59:55 +0900
Subject: [PATCHv7 1/4] libvirt/qemu - persistent modification of devices.
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 <kamezawa.hiroyu(a)jp.fujitsu.com>
---
src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 125 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index af897ad..a4fb1cd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4144,16 +4144,125 @@ cleanup:
return ret;
}
-static int qemudDomainAttachDeviceFlags(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"));
+/*
+ * Attach a device given by XML, the change will be persistent
+ * and domain XML definition file is updated.
+ */
+static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
+ virDomainDeviceDefPtr newdev)
+{
+
+ switch(newdev->type) {
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Sorry, the device is not supported for now"));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef,
+ virDomainDeviceDefPtr device)
+{
+ switch(device->type) {
+ default:
+ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Sorry, the device is not supported for now"));
return -1;
}
+ return 0;
+}
+
+static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
+ const char *xml,
+ unsigned int attach,
+ unsigned int flags)
+{
+ struct qemud_driver *driver;
+ virDomainDeviceDefPtr device;
+ virDomainDefPtr vmdef;
+ virDomainObjPtr vm;
+ int ret = -1;
+
+ /*
+ * When both of MODIFY_CONFIG and MODIFY_LIVE is specified at the same,
+ * time return error for now. We should support this later.
s/at the same, time/at the same time,/
+ */
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Now, cannot modify alive domain and its definition
"
You use 'alive domain' here, but you use 'active domain' below.
The message should be consistent.
+ "at the same time."));
+ return -1;
+ }
+
+ driver = dom->conn->privateData;
+ qemuDriverLock(driver);
+ vm = virDomainFindByName(&driver->domains, dom->name);
+ if (!vm) {
+ qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name :
'%s'"),
+ dom->name);
+ goto unlock_out;
+ }
+
+ if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
+ goto unlock_out;
+
+ if (virDomainObjIsActive(vm)) {
+ /*
+ * For now, just allow updating inactive domains. Further development
+ * will allow updating both active domain and its config file at
+ * the same time.
+ */
+ qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("unsupported to update active domain's
definition."));
+ goto endjob;
+ }
+
+ vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
- return qemudDomainAttachDevice(dom, xml);
+ if (!vmdef)
+ goto endjob;
+
+ device = virDomainDeviceDefParse(driver->caps,
+ vmdef, xml, VIR_DOMAIN_XML_INACTIVE);
+ if (!device)
+ goto endjob;
+
+ if (attach)
+ ret = qemuDomainAttachDevicePersistent(vmdef, device);
+ else
+ ret = qemuDomainDetachDevicePersistent(vmdef, device);
+
+ if (!ret) /* save the result */
+ ret = virDomainSaveConfig(driver->configDir, vmdef);
vmdef may be modified even if ret is not 0, but you do not update
the xml file. I do not find any problem about this, but it may be
better to update the xml file when ret is not 0.
+
+ virDomainDeviceDefFree(device);
+
+endjob:
+ if (qemuDomainObjEndJob(vm) == 0)
+ vm = NULL;
+unlock_out:
+ if (vm)
+ virDomainObjUnlock(vm);
+ qemuDriverUnlock(driver);
+ return ret;
+}
+
+static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
+ const char *xml,
+ unsigned int flags)
+{
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
+ return qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
+
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
+ return qemudDomainAttachDevice(dom, xml);
+
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("bad flag: %x not supported"), flags);
If the flags is VIR_DOMAIN_DEVICE_MODIFY_LIVE | unsupported_flags, we do not
report error here.
You can use the macro virCheckFlags to check it.
+
+ return -1;
}
@@ -4368,13 +4477,16 @@ 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;
- }
- return qemudDomainDetachDevice(dom, xml);
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
+ return qemuDomainModifyDevicePersistent(dom, xml, 0, flags);
+
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
+ return qemudDomainDetachDevice(dom, xml);
+
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("bad flag: %x not supported now"), flags);
+ return -1;
the same as above
}
static int qemudDomainGetAutostart(virDomainPtr dom,
11:32 p.m.
On Tue, 29 Mar 2011 11:24:23 +0800
Wen Congyang <wency(a)cn.fujitsu.com> wrote:
At 03/25/2011 04:10 PM, KAMEZAWA Hiroyuki Write:
> This is v7. Dropped patches for Nics and added 2 sanity checks and
> show correct error messages.
>
> =
>>From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
> Date: Fri, 25 Mar 2011 16:59:55 +0900
> Subject: [PATCHv7 1/4] libvirt/qemu - persistent modification of devices.
>
> 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 <kamezawa.hiroyu(a)jp.fujitsu.com>
> ---
> src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++++++-----
> 1 files changed, 125 insertions(+), 13 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index af897ad..a4fb1cd 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4144,16 +4144,125 @@ cleanup:
> return ret;
> }
>
> -static int qemudDomainAttachDeviceFlags(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"));
> +/*
> + * Attach a device given by XML, the change will be persistent
> + * and domain XML definition file is updated.
> + */
> +static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
> + virDomainDeviceDefPtr newdev)
> +{
> +
> + switch(newdev->type) {
> + default:
> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Sorry, the device is not supported for
now"));
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef,
> + virDomainDeviceDefPtr device)
> +{
> + switch(device->type) {
> + default:
> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Sorry, the device is not supported for
now"));
> return -1;
> }
> + return 0;
> +}
> +
> +static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
> + const char *xml,
> + unsigned int attach,
> + unsigned int flags)
> +{
> + struct qemud_driver *driver;
> + virDomainDeviceDefPtr device;
> + virDomainDefPtr vmdef;
> + virDomainObjPtr vm;
> + int ret = -1;
> +
> + /*
> + * When both of MODIFY_CONFIG and MODIFY_LIVE is specified at the same,
> + * time return error for now. We should support this later.
s/at the same, time/at the same time,/
will fix.
> + */
> + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("Now, cannot modify alive domain and its definition
"
You use 'alive domain' here, but you use 'active domain' below.
The message should be consistent.
will fix.
> + "at the same time."));
> + return -1;
> + }
> +
> + driver = dom->conn->privateData;
> + qemuDriverLock(driver);
> + vm = virDomainFindByName(&driver->domains, dom->name);
> + if (!vm) {
> + qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name :
'%s'"),
> + dom->name);
> + goto unlock_out;
> + }
> +
> + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> + goto unlock_out;
> +
> + if (virDomainObjIsActive(vm)) {
> + /*
> + * For now, just allow updating inactive domains. Further development
> + * will allow updating both active domain and its config file at
> + * the same time.
> + */
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("unsupported to update active domain's
definition."));
> + goto endjob;
> + }
> +
> + vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
>
> - return qemudDomainAttachDevice(dom, xml);
> + if (!vmdef)
> + goto endjob;
> +
> + device = virDomainDeviceDefParse(driver->caps,
> + vmdef, xml, VIR_DOMAIN_XML_INACTIVE);
> + if (!device)
> + goto endjob;
> +
> + if (attach)
> + ret = qemuDomainAttachDevicePersistent(vmdef, device);
> + else
> + ret = qemuDomainDetachDevicePersistent(vmdef, device);
> +
> + if (!ret) /* save the result */
> + ret = virDomainSaveConfig(driver->configDir, vmdef);
vmdef may be modified even if ret is not 0, but you do not update
the xml file. I do not find any problem about this, but it may be
better to update the xml file when ret is not 0.
Hmm ? I'll add a spec. on qemuDomainAt(De)tachDevicePersistent() to
never update vmdef when return !0. Is it ok ?
> +
> + virDomainDeviceDefFree(device);
> +
> +endjob:
> + if (qemuDomainObjEndJob(vm) == 0)
> + vm = NULL;
> +unlock_out:
> + if (vm)
> + virDomainObjUnlock(vm);
> + qemuDriverUnlock(driver);
> + return ret;
> +}
> +
> +static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
> + const char *xml,
> + unsigned int flags)
> +{
> + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
> + return qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
> +
> + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
> + return qemudDomainAttachDevice(dom, xml);
> +
> + qemuReportError(VIR_ERR_INVALID_ARG,
> + _("bad flag: %x not supported"), flags);
If the flags is VIR_DOMAIN_DEVICE_MODIFY_LIVE | unsupported_flags, we do not
report error here.
You can use the macro virCheckFlags to check it.
Hm, ok. will see it.
Thanks,
-Kame
12:17 a.m.
At 03/29/2011 12:32 PM, KAMEZAWA Hiroyuki Write:
On Tue, 29 Mar 2011 11:24:23 +0800
Wen Congyang <wency(a)cn.fujitsu.com> wrote:
> At 03/25/2011 04:10 PM, KAMEZAWA Hiroyuki Write:
>> This is v7. Dropped patches for Nics and added 2 sanity checks and
>> show correct error messages.
>>
>> =
>> >From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001
>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
>> Date: Fri, 25 Mar 2011 16:59:55 +0900
>> Subject: [PATCHv7 1/4] libvirt/qemu - persistent modification of devices.
>>
>> 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 <kamezawa.hiroyu(a)jp.fujitsu.com>
>> ---
>> src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++++++-----
>> 1 files changed, 125 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index af897ad..a4fb1cd 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4144,16 +4144,125 @@ cleanup:
>> return ret;
>> }
>>
>> -static int qemudDomainAttachDeviceFlags(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"));
>> +/*
>> + * Attach a device given by XML, the change will be persistent
>> + * and domain XML definition file is updated.
>> + */
>> +static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef,
>> + virDomainDeviceDefPtr newdev)
>> +{
>> +
>> + switch(newdev->type) {
>> + default:
>> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("Sorry, the device is not supported for
now"));
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef,
>> + virDomainDeviceDefPtr device)
>> +{
>> + switch(device->type) {
>> + default:
>> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("Sorry, the device is not supported for
now"));
>> return -1;
>> }
>> + return 0;
>> +}
>> +
>> +static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
>> + const char *xml,
>> + unsigned int attach,
>> + unsigned int flags)
>> +{
>> + struct qemud_driver *driver;
>> + virDomainDeviceDefPtr device;
>> + virDomainDefPtr vmdef;
>> + virDomainObjPtr vm;
>> + int ret = -1;
>> +
>> + /*
>> + * When both of MODIFY_CONFIG and MODIFY_LIVE is specified at the same,
>> + * time return error for now. We should support this later.
>
> s/at the same, time/at the same time,/
>
will fix.
>> + */
>> + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
>> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> + _("Now, cannot modify alive domain and its
definition "
>
> You use 'alive domain' here, but you use 'active domain' below.
> The message should be consistent.
>
will fix.
>> + "at the same time."));
>> + return -1;
>> + }
>> +
>> + driver = dom->conn->privateData;
>> + qemuDriverLock(driver);
>> + vm = virDomainFindByName(&driver->domains, dom->name);
>> + if (!vm) {
>> + qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name :
'%s'"),
>> + dom->name);
>> + goto unlock_out;
>> + }
>> +
>> + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
>> + goto unlock_out;
>> +
>> + if (virDomainObjIsActive(vm)) {
>> + /*
>> + * For now, just allow updating inactive domains. Further development
>> + * will allow updating both active domain and its config file at
>> + * the same time.
>> + */
>> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> + _("unsupported to update active domain's
definition."));
>> + goto endjob;
>> + }
>> +
>> + vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
>>
>> - return qemudDomainAttachDevice(dom, xml);
>> + if (!vmdef)
>> + goto endjob;
>> +
>> + device = virDomainDeviceDefParse(driver->caps,
>> + vmdef, xml, VIR_DOMAIN_XML_INACTIVE);
>> + if (!device)
>> + goto endjob;
>> +
>> + if (attach)
>> + ret = qemuDomainAttachDevicePersistent(vmdef, device);
>> + else
>> + ret = qemuDomainDetachDevicePersistent(vmdef, device);
>> +
>> + if (!ret) /* save the result */
>> + ret = virDomainSaveConfig(driver->configDir, vmdef);
>
> vmdef may be modified even if ret is not 0, but you do not update
> the xml file. I do not find any problem about this, but it may be
> better to update the xml file when ret is not 0.
>
Hmm ? I'll add a spec. on qemuDomainAt(De)tachDevicePersistent() to
never update vmdef when return !0. Is it ok ?
No.
In patch 2, the function qemuDomainDeviceAddressFixup() may modify vmdef
and return -1:
=============
+static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci)
+{
+ if (!pci && virDomainDefAddImplicitControllers(vmdef))
+ return -1;
+ /* added controller requires PCI address */
+ return qemuDomainAssignPCIAddresses(vmdef);
+}
+
=============
The function virDomainDefAddImplicitControllers() may modify vmdef. If
qemuDomainAssignPCIAddresses() failed, there is no way to rollback the
operation that virDomainDefAddImplicitControllers() do.
>> +
>> + virDomainDeviceDefFree(device);
>> +
>> +endjob:
>> + if (qemuDomainObjEndJob(vm) == 0)
>> + vm = NULL;
>> +unlock_out:
>> + if (vm)
>> + virDomainObjUnlock(vm);
>> + qemuDriverUnlock(driver);
>> + return ret;
>> +}
>> +
>> +static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
>> + const char *xml,
>> + unsigned int flags)
>> +{
>> + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
>> + return qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
>> +
>> + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
>> + return qemudDomainAttachDevice(dom, xml);
>> +
>> + qemuReportError(VIR_ERR_INVALID_ARG,
>> + _("bad flag: %x not supported"), flags);
>
> If the flags is VIR_DOMAIN_DEVICE_MODIFY_LIVE | unsupported_flags, we do not
> report error here.
>
> You can use the macro virCheckFlags to check it.
>
Hm, ok. will see it.
Thanks,
-Kame
12:25 a.m.
On Tue, 29 Mar 2011 13:17:55 +0800
Wen Congyang <wency(a)cn.fujitsu.com> wrote:
At 03/29/2011 12:32 PM, KAMEZAWA Hiroyuki Write:
> On Tue, 29 Mar 2011 11:24:23 +0800
> Wen Congyang <wency(a)cn.fujitsu.com> wrote:
> Hmm ? I'll add a spec. on
qemuDomainAt(De)tachDevicePersistent() to
> never update vmdef when return !0. Is it ok ?
No.
In patch 2, the function qemuDomainDeviceAddressFixup() may modify vmdef
and return -1:
=============
+static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci)
+{
+ if (!pci && virDomainDefAddImplicitControllers(vmdef))
+ return -1;
+ /* added controller requires PCI address */
+ return qemuDomainAssignPCIAddresses(vmdef);
+}
+
=============
The function virDomainDefAddImplicitControllers() may modify vmdef. If
qemuDomainAssignPCIAddresses() failed, there is no way to rollback the
operation that virDomainDefAddImplicitControllers() do.
Of course, I'll add a rollback. That's a bug in patch'2', not here.
Thanks,
-Kame
5240
days inactive
5244
days old
16 comments
3 participants
participants (3)
-
Hu Tao -
KAMEZAWA Hiroyuki -
Wen Congyang