On 01/05/2015 02:29 AM, Wang Rui wrote:

The new qemuMonitorSetBootIndex() method can set device' boot order online using 'qom-set' JSON monitor command. HMP is not supported. And it is used for QEMU >= 2.2.0 . The QMP command is like "qom-set net1 bootindex 2".

Signed-off-by: Wang Rui <moon.wangrui@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> --- src/qemu/qemu_monitor.c | 25 +++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 4 ++++ src/qemu/qemu_monitor_json.c | 19 +++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 +++++ 4 files changed, 53 insertions(+)

Looks like this was "missed/lost/forgotten" from the holidays... You'll need to update with top of tree... What happens if qemu < 2.2? Is there a way to test? Callers will end up failing perhaps when you don't want them to... After reading later patches - is there a similar qom-get for this field? That would simplify a few nasty error paths later..

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 100bbd0..907834f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4292,3 +4292,28 @@ void qemuMonitorIOThreadsInfoFree(qemuMonitorIOThreadsInfoPtr iothread) VIR_FREE(iothread->name); VIR_FREE(iothread); } + +int +qemuMonitorSetBootIndex(qemuMonitorPtr mon, + const char *name, + int bootIndex) +{ + int ret; + VIR_DEBUG("mon=%p, name=%p:%s, bootIndex=%d", mon, name, name, bootIndex); + + if (!mon || !name) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor || name must not be NULL")); + return -1; + }

See [1]

+ + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -1; + } + + ret = qemuMonitorJSONSetBootIndex(mon, name, bootIndex); + + return ret; +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index edab66f..8e5d86e 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -888,6 +888,10 @@ int qemuMonitorGetIOThreads(qemuMonitorPtr mon,

void qemuMonitorIOThreadsInfoFree(qemuMonitorIOThreadsInfoPtr iothread);

+int qemuMonitorSetBootIndex(qemuMonitorPtr mon, + const char *name, + int bootIndex);

[1] If you change this slightly to be int bootIndex) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); Then you avoid the !mon || !name checks above.... also the %p:%s becomes unnecessary - that is either %p or %s.

+ /** * When running two dd process and using <> redirection, we need a * shell that will not truncate files. These two strings serve that diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e567aa7..df52101 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6500,3 +6500,22 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon, virJSONValueFree(reply); return ret; } + +int +qemuMonitorJSONSetBootIndex(qemuMonitorPtr mon, + const char *name, + int bootIndex) +{ + qemuMonitorJSONObjectProperty prop; + + memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; + prop.val.iv = bootIndex; + + if (qemuMonitorJSONSetObjectProperty(mon, name, + "bootindex", + &prop) < 0) { + return -1; + } + return 0; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 222f11e..6355ddf 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -472,4 +472,9 @@ int qemuMonitorJSONRTCResetReinjection(qemuMonitorPtr mon); int qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon, qemuMonitorIOThreadsInfoPtr **iothreads) ATTRIBUTE_NONNULL(2); + +int qemuMonitorJSONSetBootIndex(qemuMonitorPtr mon, + const char *name, + int bootIndex); + #endif /* QEMU_MONITOR_JSON_H */

On 01/05/2015 02:29 AM, Wang Rui wrote:

When we attach an interface to a running VM with boot index, we get a successful result. But in fact the boot index won't take effect. QEMU supported to set device's boot index online recently(since QEMU 2.2.0).

After this patch, the boot index will take effect after virDomainAttachDevice(Flags) API returning success.

Signed-off-by: Wang Rui <moon.wangrui@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> --- src/qemu/qemu_hotplug.c | 33 +++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 4 ++++ 2 files changed, 37 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7f93b9b..919a061 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1087,6 +1087,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, /* link set to down */ }

+ if (net->info.bootIndex > 0) { + if (qemuDomainChangeBootIndex(driver, vm, &net->info, + net->info.bootIndex) < 0) { + virDomainAuditNet(vm, NULL, net, "attach", false);

[1] See note below...

+ goto try_remove; + } + } + virDomainAuditNet(vm, NULL, net, "attach", true);

ret = 0; @@ -1853,6 +1861,31 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver, }

int +qemuDomainChangeBootIndex(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceInfoPtr devInfo, + int newBootIndex) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!devInfo->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("can't change boot index: device alias not found"));

s/can't/cannot (I know you copied it from ChangeNetLinkState)

+ return -1; + } + + VIR_DEBUG("Change dev: %s boot index from %d to %d", devInfo->alias, + devInfo->bootIndex, newBootIndex);

Interesting that from patch 1 on you checked "!name" in the subsequent SetBootIndex call and failed. Since you make that check above all the more reason to go with the ATTRIBUTE_NONNULL as I suggested.

+ + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, newBootIndex); + qemuDomainObjExitMonitor(driver, vm);

[1] Due to commit id '5c703ca39' a check of the return is required. However, it seems that this can follow other code from the calling function and do an 'ignore_value();'. If ExitMonitor returns < 0, then the vm is dead - that could cause issues in the calling path which makes a virDomainNetAudit() call. Other error path code from the caller will ignore status and audit, so I suppose this could follow that, but it may not be as safe as expected.

+ + return ret; +} + +int qemuDomainChangeNet(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainPtr dom, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index d13c532..3af0875 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -57,6 +57,10 @@ int qemuDomainAttachHostDevice(virConnectPtr conn, virDomainHostdevDefPtr hostdev); int qemuDomainFindGraphicsIndex(virDomainDefPtr def, virDomainGraphicsDefPtr dev); +int qemuDomainChangeBootIndex(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceInfoPtr devInfo, + int newBootIndex);

s/);/) ATTRIBUTE_NONNULL(3); especially since you access it without checking for it... John

int qemuDomainChangeGraphics(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainGraphicsDefPtr dev);

On 01/05/2015 02:29 AM, Wang Rui wrote:

We can update device and attach device(cdrom) with the boot order unchanged. But the boot order check method will report an error: "boot order %d is already used by another device"

In fact the boot order is used by itself, we just want to keep it as before. This patch improves boot order check to distinguish the owner.

Signed-off-by: Wang Rui <moon.wangrui@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> --- src/conf/device_conf.c | 13 +++++++++++++ src/conf/device_conf.h | 12 ++++++++++++ src/conf/domain_conf.c | 31 +++++++++++++++++++++++++++---- src/conf/domain_conf.h | 9 --------- 4 files changed, 52 insertions(+), 13 deletions(-)

Probably could have split this in two in order to do code motion fi... 3a. -> Move virDomainDeviceDriveAddress from domain_conf.h to device_conf.h which just seems to be separate from the... op 3b. -> Rest of this patch and you will also need to modify src/libvirt_private.syms in order to add virDeviceDriveAddressEqual; "properly" since it's global. I also think that there's some synergies with patch 6/8 especially with the matching logic Perhaps an update to the "boot order" text in the <hostdev> section of formatdomain.html.in would be beneficial to describe these additional checks.

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 5ffe159..907a7b8 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -144,6 +144,19 @@ virDevicePCIAddressEqual(virDevicePCIAddress *addr1, return false; }

+bool +virDeviceDriveAddressEqual(virDomainDeviceDriveAddressPtr addr1, + virDomainDeviceDriveAddressPtr addr2) +{ + if (addr1->controller == addr2->controller && + addr1->bus == addr2->bus && + addr1->target == addr2->target && + addr1->unit == addr2->unit) { + return true; + } + return false; +} + int virInterfaceLinkParseXML(xmlNodePtr node, virInterfaceLinkPtr lnk) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index f067a35..afcf0ee 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -55,6 +55,15 @@ struct _virDevicePCIAddress { int multi; /* enum virDomainDeviceAddressPCIMulti */ };

+typedef struct _virDomainDeviceDriveAddress virDomainDeviceDriveAddress; +typedef virDomainDeviceDriveAddress *virDomainDeviceDriveAddressPtr; +struct _virDomainDeviceDriveAddress { + unsigned int controller; + unsigned int bus; + unsigned int target; + unsigned int unit; +}; + typedef struct _virInterfaceLink virInterfaceLink; typedef virInterfaceLink *virInterfaceLinkPtr; struct _virInterfaceLink { @@ -74,6 +83,9 @@ int virDevicePCIAddressFormat(virBufferPtr buf, bool virDevicePCIAddressEqual(virDevicePCIAddress *addr1, virDevicePCIAddress *addr2);

+bool virDeviceDriveAddressEqual(virDomainDeviceDriveAddressPtr addr1, + virDomainDeviceDriveAddressPtr addr2); + int virInterfaceLinkParseXML(xmlNodePtr node, virInterfaceLinkPtr lnk);

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aafc05e..d2c4a0a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19953,12 +19953,35 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainDeviceInfoPtr newinfo = opaque;

if (info->bootIndex == newinfo->bootIndex) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("boot order %d is already used by another device"), - newinfo->bootIndex); - return -1; + /* check if the same boot order is used by another device or itself */ + if (info->type != newinfo->type) { + goto error; + } else { + switch ((virDomainDeviceAddressType) newinfo->type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: + if (virDevicePCIAddressEqual(&info->addr.pci, + &newinfo->addr.pci)) + return 0; + else + goto error; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: + if (virDeviceDriveAddressEqual(&info->addr.drive, + &newinfo->addr.drive)) + return 0; + else + goto error; + default: + goto error;

This is difficult to read with all the if-then-else and goto's. How about the following: if (info->bootIndex == newinfo->bootIndex) { /* check if the same boot order is used by another device or itself */ if (info->type != newinfo->type) goto error; if ((newinfo->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && virDevicePCIAddressEqual(&info->addr.pci, &newinfo->addr.pci)) || (newinfo->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE && virDeviceDriveAddressEqual(&info->addr.drive, &newinfo->addr.drive))) return 0; goto error; } return 0; error: ... Even with this change - I'm concerned about the cases where 'type' doesn't match and is not PCI or DRIVE (e.g., the two new error paths). Trying to wrap my mind about the timing of the check and when the 'type' and address value are filled in w/r/t if they're not present in the XML when the guest starts, but I think this is only called in an attach, update, or detach path so it should be ok... Although I see virDomainDefCompatibleDevice is called in an LXC path, so while I assume you've made sure the qemu path works right, what about the lxc path as it relates to the device info type/address checks? Since patch 6 changes the Iterate from just 'attach' to 'attach' or 'update', it seems you're trying to "cover" the update case, but may have caused unforeseen issues with the attach case. I think my 3b and 6 should be combined to make it easier to consider the options. In particular, it seems new restrictions are in place that bootIndex can only work for PCI and DRIVE virDomainDeviceInfo address types. I think the USB type is throwing me off a bit. Just trying to make sure some already existing option isn't being disallowed with the new changes. Adding in specific device address checks based on types just raises that concern. John

+ } + } } return 0; + + error: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("boot order %d is already used by another device"), + newinfo->bootIndex); + return -1; }

int diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 57297cd..3bddc80 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -236,15 +236,6 @@ typedef enum { VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST } virDomainDeviceAddressType;

-typedef struct _virDomainDeviceDriveAddress virDomainDeviceDriveAddress; -typedef virDomainDeviceDriveAddress *virDomainDeviceDriveAddressPtr; -struct _virDomainDeviceDriveAddress { - unsigned int controller; - unsigned int bus; - unsigned int target; - unsigned int unit; -}; - typedef struct _virDomainDeviceVirtioSerialAddress virDomainDeviceVirtioSerialAddress; typedef virDomainDeviceVirtioSerialAddress *virDomainDeviceVirtioSerialAddressPtr; struct _virDomainDeviceVirtioSerialAddress {

On 01/05/2015 02:29 AM, Wang Rui wrote:

When we attach a disk to a running VM with boot index, we can get a successful result. But in fact the boot index won't take effect. QEMU supported to set device's boot index online recently(since QEMU 2.2.0).

After this patch, the boot index will take effect after virDomainAttachDevice(Flags) API returning success. If new disk is attached successfully but boot index is set failed, we'll remove the new disk to restore.

Signed-off-by: Wang Rui <moon.wangrui@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> --- src/qemu/qemu_hotplug.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)

If I read the tea leaves correctly - running this on < qemu 2.2 will cause a failure since the ChangeBootIndex isn't supported there. I don't get a sense why just failing the boot index setting should cause the disk to not be attached. How do you differentiate the failure is not supported vs. not present? Seems to me there should be a way to qom-get and do some "pre-checks" before adding, then trying to rip it out which is just ripe for all sorts of errors. Taking that route perhaps means the code movement patch is unnecessary and the device removal is unnecessary. I didn't dive deep into the following code as I really think the qom-get is something that'll obsolete certain parts of it. John

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2f84949..5eacfce 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2999,7 +2999,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, { virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; + virDomainDeviceDefPtr new_dev_copy = NULL; + virDomainDeviceDefPtr old_dev_copy = NULL; + virDomainDiskDefPtr tmp = NULL; + virCapsPtr caps = NULL; int ret = -1; + int removed = 0; const char *driverName = virDomainDiskGetDriver(disk); const char *src = virDomainDiskGetSource(disk);

@@ -3022,6 +3027,13 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) goto end;

+ if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto end; + + if (!(new_dev_copy = virDomainDeviceDefCopy(dev, vm->def, + caps, driver->xmlopt))) + goto end; + switch (disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: @@ -3036,12 +3048,33 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; }

+ tmp = dev->data.disk; + dev->data.disk = orig_disk; + /* save a copy of old device to restore */ + if (!(old_dev_copy = virDomainDeviceDefCopy(dev, vm->def, + caps, driver->xmlopt))) { + dev->data.disk = tmp; + goto end; + } + dev->data.disk = tmp; + if (qemuDomainChangeEjectableMedia(driver, conn, vm, orig_disk, disk->src, false) < 0) goto end;

disk->src = NULL; ret = 0; + + tmp = new_dev_copy->data.disk; + if (orig_disk->info.bootIndex != tmp->info.bootIndex) { + /* If boot index is to be changed to 0, we can pass "value":-1 to + qmp command("qom-set") to cancel boot index. */ + if (qemuDomainChangeBootIndex(driver, vm, &orig_disk->info, + tmp->info.bootIndex ? + tmp->info.bootIndex : -1) < 0) + goto try_remove; + orig_disk->info.bootIndex = tmp->info.bootIndex; + } break;

case VIR_DOMAIN_DISK_DEVICE_DISK: @@ -3063,6 +3096,15 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, _("disk bus '%s' cannot be hotplugged."), virDomainDiskBusTypeToString(disk->bus)); } + + tmp = new_dev_copy->data.disk; + if (!ret && tmp->info.bootIndex > 0) { + if (qemuDomainChangeBootIndex(driver, vm, &disk->info, + tmp->info.bootIndex) < 0) + goto try_remove; + disk->info.bootIndex = tmp->info.bootIndex; + } + break; default: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -3074,7 +3116,53 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, end: if (ret != 0) ignore_value(qemuRemoveSharedDevice(driver, dev, vm->def->name)); + virObjectUnref(caps); + virDomainDeviceDefFree(new_dev_copy); + virDomainDeviceDefFree(old_dev_copy); + if (removed) { + dev->data.disk = NULL; + ret = -1; + } return ret; + + try_remove: + if (!virDomainObjIsActive(vm)) { + removed = 1; + goto end; + } + + switch (new_dev_copy->data.disk->device) { + case VIR_DOMAIN_DISK_DEVICE_CDROM: + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + if (qemuAddSharedDevice(driver, old_dev_copy, vm->def->name) < 0) + break; + + tmp = old_dev_copy->data.disk; + if (qemuDomainChangeEjectableMedia(driver, conn, vm, orig_disk, tmp->src, false) < 0) { + VIR_WARN("Unable to recover original media with target '%s' and path '%s'", + tmp->dst, tmp->src->path); + ignore_value(qemuRemoveSharedDevice(driver, old_dev_copy, vm->def->name)); + } else { + tmp->src = NULL; + } + break; + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + if (qemuDomainDetachVirtioDiskDevice(driver, vm, disk) != 0) + VIR_WARN("Unable to detach new disk with bus '%s' and target '%s'", + virDomainDiskBusTypeToString(disk->bus), disk->dst); + } + else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || + disk->bus == VIR_DOMAIN_DISK_BUS_USB) + if (qemuDomainDetachDiskDevice(driver, vm, disk) != 0) + VIR_WARN("Unable to detach new disk with bus '%s' and target '%s'", + virDomainDiskBusTypeToString(disk->bus), disk->dst); + ret = -1; + break; + } + removed = 1; + goto end; }

static int

On 01/05/2015 02:29 AM, Wang Rui wrote:

Signed-off-by: Wang Rui <moon.wangrui@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-)

Since there's no commit message - even more reason to combine with patch 3 (or my 3b)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d2c4a0a..5beb830 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19991,29 +19991,32 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, { virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);

- if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH) - return 0; - - if (!virDomainDefHasUSB(def) && - STRNEQ(def->os.type, "exe") && - virDomainDeviceIsUSB(dev)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Device configuration is not compatible: " - "Domain has no USB bus support")); - return -1; - } - - if (info && info->bootIndex > 0) { - if (def->os.nBootDevs > 0) { + switch (action) { + case VIR_DOMAIN_DEVICE_ACTION_ATTACH: + if (!virDomainDefHasUSB(def) && + STRNEQ(def->os.type, "exe") && + virDomainDeviceIsUSB(dev)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("per-device boot elements cannot be used" - " together with os/boot elements")); + _("Device configuration is not compatible: " + "Domain has no USB bus support")); return -1; } - if (virDomainDeviceInfoIterate(def, - virDomainDeviceInfoCheckBootIndex, - info) < 0) - return -1; + case VIR_DOMAIN_DEVICE_ACTION_UPDATE: + if (info && info->bootIndex > 0) { + if (def->os.nBootDevs > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("per-device boot elements cannot be used" + " together with os/boot elements")); + return -1; + } + if (virDomainDeviceInfoIterate(def, + virDomainDeviceInfoCheckBootIndex, + info) < 0) + return -1; + } + break; + default: + return 0;

The 'norm' of using default is to list all the cases - makes it easier to determine when one is missing. I think the switch in this case is overkill. I think this is better: if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH || action != VIR_DOMAIN_DEVICE_ACTION_UPDATE) return 0; if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH && !virDomainDefHasUSB(def) && STRNEQ(def->os.type, "exe") && virDomainDeviceIsUSB(dev)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Device configuration is not compatible: " "Domain has no USB bus support")); return -1; } if (info && info->bootIndex > 0) { if (def->os.nBootDevs > 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("per-device boot elements cannot be used" " together with os/boot elements")); return -1; } if (virDomainDeviceInfoIterate(def, virDomainDeviceInfoCheckBootIndex, info) < 0) return -1; } [sorry about the wrapping on the CheckBootIndex call - the point is all you're doing is adding _UPDATE to the initial check... then adding the "if (action == *_ATTACH)" for just the if USB check...

}

return 0;

On 01/05/2015 02:29 AM, Wang Rui wrote:

QEMU supported to set device's boot index online recently(since QEMU 2.2.0). This patch implements the interface's boot index update lively.

If PCI address is not specified in the new xml, we can also update boot index. If boot order is not specified in the new xml, we take it as canceling boot index. So pass "value":-1 to qmp command("qom-set") to cancel boot index.

Signed-off-by: Wang Rui <moon.wangrui@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> --- src/qemu/qemu_hotplug.c | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-)

So now we're adding the same functionality to a network interface? Maybe a better description above would help me understand what you're trying to accomplish. Perhaps let's get the disk devices to be all set before taking this path

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5eacfce..4c86370 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1814,6 +1814,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, bool needLinkStateChange = false; bool needReplaceDevDef = false; bool needBandwidthSet = false; + bool needBootIndexChange = false; int ret = -1;

if (!devslot || !(olddev = *devslot)) { @@ -1909,6 +1910,19 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }

+ /* If(and olny if) PCI adderss is not specified in the new xml, newdev->info.type

s/If(and olny if)/If (and only if) a s/adderss/address Going back a few patches for attach & update and my concern over whether the XML has an address already or not - this shows why I was concerned. If you don't yet have an address you could possibly match something that you weren't expecting to or not match something... I didn't look too much deeper here. BTW: If you're adding network, you'll need to document that. John

+ * here is NONE. We can copy the old device's PCI address to the new one. We can't + * copy olddev->info to newdev->info, because other members in newdev->info(such + * as bootIndex) should not be overridden. + */ + if (newdev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + newdev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + newdev->info.addr.pci.domain = olddev->info.addr.pci.domain; + newdev->info.addr.pci.bus = olddev->info.addr.pci.bus; + newdev->info.addr.pci.slot = olddev->info.addr.pci.slot; + newdev->info.addr.pci.function = olddev->info.addr.pci.function; + } + /* info: if newdev->info is empty, fill it in from olddev, * otherwise verify that it matches - nothing is allowed to * change. (There is no helper function to do this, so @@ -1945,11 +1959,6 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, _("cannot modify network rom file")); goto cleanup; } - if (olddev->info.bootIndex != newdev->info.bootIndex) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot modify network device boot index setting")); - goto cleanup; - } /* (end of device info checks) */

if (STRNEQ_NULLABLE(olddev->filter, newdev->filter) || @@ -2101,6 +2110,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, virDomainNetGetActualBandwidth(newdev))) needBandwidthSet = true;

+ if (olddev->info.bootIndex != newdev->info.bootIndex) + needBootIndexChange = true; + /* FINALLY - actually perform the required actions */

if (needReconnect) { @@ -2141,6 +2153,19 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }

+ if (needBootIndexChange) { + /* If boot index is to be changed to 0, we can pass "value":-1 to + qmp command("qom-set") to cancel boot index. */ + if (qemuDomainChangeBootIndex(driver, vm, &olddev->info, + newdev->info.bootIndex ? + newdev->info.bootIndex : -1) < 0) + goto cleanup; + /* we successfully switched to the new boot index, and we've + * determined that the rest of newdev is equivalent to olddev, + * so move newdev into place */ + needReplaceDevDef = true; + } + if (needReplaceDevDef) { /* the changes above warrant replacing olddev with newdev in * the domain's nets list.

On 01/05/2015 02:29 AM, Wang Rui wrote:

QEMU supported to set device's boot index online recently(since QEMU 2.2.0). This patch implements the disk's boot index update lively.

Description needs some more beef... The usage of "update lively" doesn't translate well. I think you're talking about being able to make an 'online update' of a disk's bootIndex. Essentially this allows us to change an existing from "2" to "3" perhaps and then add a new "2" - is that correct? What happens if you have 1, 2, 3 ... try to change 3 to 2 - will be an error? The clearing of index in order to remove it needs to be made clearer on the formatdomain.html.in and/or virsh.pod. All this needs to be done via XML editing - prone to errors... Perhaps consider adding a virsh option... Makes it a whole lot easier to adjust things rather than attempting to generate XML to modify things. Trying to figure out the magic incantation required to "remove" the index "live" by only providing some XML to a virsh command can be a challenge. Sure you understand it and I'm beginning to, but how would one "use" this feature is not described anywhere. John

If boot order is not specified in the new xml, we take it as canceling boot index. So pass "value":-1 to qmp command("qom-set") to cancel boot index.

Signed-off-by: Wang Rui <moon.wangrui@huawei.com> Signed-off-by: Zhou Yimin <zhouyimin@huawei.com> --- src/qemu/qemu_driver.c | 64 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 673d8a6..9615fe4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7048,6 +7048,7 @@ qemuDomainChangeDiskMediaLive(virConnectPtr conn, { virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; + int oldBootIndex = -1; int ret = -1;

if (virStorageTranslateDiskSourcePool(conn, disk) < 0) @@ -7056,16 +7057,25 @@ qemuDomainChangeDiskMediaLive(virConnectPtr conn, if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) goto end;

+ if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def, + disk->bus, disk->dst))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No device with bus '%s' and target '%s'"), + virDomainDiskBusTypeToString(disk->bus), + disk->dst); + goto end; + } + switch (disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: - if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def, - disk->bus, disk->dst))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("No device with bus '%s' and target '%s'"), - virDomainDiskBusTypeToString(disk->bus), - disk->dst); - goto end; + if (orig_disk->info.bootIndex != disk->info.bootIndex) { + oldBootIndex = orig_disk->info.bootIndex; + if (qemuDomainChangeBootIndex(driver, vm, &orig_disk->info, + disk->info.bootIndex ? + disk->info.bootIndex : -1) < 0) + goto end; + orig_disk->info.bootIndex = disk->info.bootIndex; }

/* Add the new disk src into shared disk hash table */ @@ -7075,6 +7085,16 @@ qemuDomainChangeDiskMediaLive(virConnectPtr conn, if (qemuDomainChangeEjectableMedia(driver, conn, vm, orig_disk, dev->data.disk->src, force) < 0) { ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, vm->def->name)); + + /* Change media failed. Boot index should be changed back to original. */ + if (oldBootIndex >= 0) { + if (qemuDomainChangeBootIndex(driver, vm, &orig_disk->info, + oldBootIndex ? oldBootIndex : -1) < 0) { + VIR_WARN("Change Boot index back to original failed"); + goto end; + } + orig_disk->info.bootIndex = oldBootIndex; + } goto end; }

@@ -7082,6 +7102,22 @@ qemuDomainChangeDiskMediaLive(virConnectPtr conn, ret = 0; break;

+ case VIR_DOMAIN_DISK_DEVICE_DISK: + if (orig_disk->info.bootIndex != disk->info.bootIndex) { + if (qemuDomainChangeBootIndex(driver, vm, &orig_disk->info, + disk->info.bootIndex ? + disk->info.bootIndex : -1) < 0) + goto end; + orig_disk->info.bootIndex = disk->info.bootIndex; + ret = 0; + break; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk bus '%s' cannot be updated except boot index."), + virDomainDiskBusTypeToString(disk->bus)); + goto end; + } + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk bus '%s' cannot be updated."), @@ -7413,8 +7449,17 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, return -1; } orig = vmdef->disks[pos]; - if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && - !(orig->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) { + if (orig->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (orig->info.bootIndex != disk->info.bootIndex) { + orig->info.bootIndex = disk->info.bootIndex; + break; + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("this disk doesn't support update except boot index")); + return -1; + } + } else if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && + !(orig->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("this disk doesn't support update")); return -1; @@ -7436,6 +7481,7 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, orig->src->format = disk->src->format; disk->src->path = NULL; orig->startupPolicy = disk->startupPolicy; + orig->info.bootIndex = disk->info.bootIndex; break;

case VIR_DOMAIN_DEVICE_GRAPHICS: