[PATCH 0/7] qemu: support updating device's bootindex
Support updating device's(support cdrom, disk and network) bootindex online in virDomainUpdateDeviceFlags. The new bootindex will take effect after guest rebooting. Enable bootindex can be set to 0, it means cancel the device's bootindex. To use this feature, we need to get the device's xml first and modify the boot order in the xml, then use 'virsh update-device <domain> <xml> --flag' to update the bootindex. Note that the flag should be --config or --persistent if the vm is running. diff to v1: * add a bool bootIndexSpecified in device info to indicates that the bootorder of the device can be set. * use boot order = '0' to cancel the previous bootorder instead of '-1' Jiang Jiacheng (7): qemu: Introduce qemuDomainChangeBootIndex API qemu: Add bootIndexSpecified and support set bootIndex to '0' qemu: Introduce qemuCheckBootIndex and qemuChangeDiskBootIndex API qemu: Support update disk's bootindex qemu: Support update net's bootindex qemu: Support add bootindex = 0 to boothash when its bootIndexSpecified is true qemu: Reserve bootIndexSpecified when update device src/conf/device_conf.h | 3 + src/conf/domain_conf.c | 9 ++- src/conf/domain_postparse.c | 8 +- src/qemu/qemu_conf.c | 140 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 16 ++++ src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_driver.c | 35 +++++++++ src/qemu/qemu_hotplug.c | 17 +++-- src/qemu/qemu_monitor.c | 20 +++++ src/qemu/qemu_monitor.h | 6 ++ src/qemu/qemu_monitor_json.c | 33 +++++++++ src/qemu/qemu_monitor_json.h | 6 ++ 12 files changed, 286 insertions(+), 10 deletions(-) -- 2.33.0
Introduce qemuDomainChangeBootIndex api to support update device's bootindex. These function will be used in following patches to support change device's (support cdrom, disk and net) bootindex with virsh command like 'virsh update-device <domain> <file> [--flag]'. Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_conf.c | 40 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/qemu_monitor.c | 20 ++++++++++++++++++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 33 +++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++++ 6 files changed, 110 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ae5bbcd138..0071a95cb6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1641,3 +1641,43 @@ qemuHugepageMakeBasedir(virQEMUDriver *driver, return 0; } + +/** + * qemuDomainChangeBootIndex: + * @vm: domain object + * @devInfo: origin device info + * @newBootIndex: new bootIndex + * + * check the alias and bootindex before send the command + * + * Returns: 0 on success, + * -1 otherwise. + */ +int +qemuDomainChangeBootIndex(virDomainObj *vm, + virDomainDeviceInfo *devInfo, + int newBootIndex) +{ + int ret = -1; + int dummyIndex = -1; + qemuDomainObjPrivate *priv = vm->privateData; + + if (!devInfo->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("cannot change boot index: device alias not found")); + return -1; + } + + VIR_DEBUG("Change dev: %s boot index from %d to %d", devInfo->alias, + devInfo->bootIndex, newBootIndex); + + qemuDomainObjEnterMonitor(vm); + /* newBootIndex = 0 means cancel the specified bootIndex, send -1 to qemu instead */ + if (!newBootIndex) + ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, dummyIndex); + else + ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, newBootIndex); + qemuDomainObjExitMonitor(vm); + + return ret; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 8cf2dd2ec5..e7447191df 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -375,3 +375,8 @@ int qemuGetMemoryBackingPath(virQEMUDriver *driver, int qemuHugepageMakeBasedir(virQEMUDriver *driver, virHugeTLBFS *hugepage); + +int qemuDomainChangeBootIndex(virDomainObj *vm, + virDomainDeviceInfo *devInfo, + int newBootIndex) + ATTRIBUTE_NONNULL(2); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 80f262cec7..2b89d9bdae 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4491,3 +4491,23 @@ qemuMonitorGetStatsByQOMPath(virJSONValue *arr, return NULL; } + +/** + * qemuMonitorSetBootIndex: + * @mon: monitor object + * @alias: device's alias + * @bootIndex: new boot index + * + * Returns data from a call to 'qom-set' + */ +int +qemuMonitorSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) +{ + VIR_DEBUG("name=%s, bootIndex=%d", alias, bootIndex); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONSetBootIndex(mon, alias, bootIndex); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c690fc3655..0b9943e1f7 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1568,3 +1568,9 @@ qemuMonitorExtractQueryStats(virJSONValue *info); virJSONValue * qemuMonitorGetStatsByQOMPath(virJSONValue *arr, char *qom_path); + +int +qemuMonitorSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index f4e942a32f..75de73e61b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8890,3 +8890,36 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon, return virJSONValueObjectStealArray(reply, "return"); } + +/** + * qemuMonitorSetBootIndex: + * @mon: monitor object + * @alias: device's alias + * @bootIndex: new boot index + * + * Set the bootindex of device to new bootIndex + * + * Returns: 0 on success, + * -1 otherwise. + */ +int +qemuMonitorJSONSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-set", "s:path", alias, + "s:property", "bootindex", + "i:value", bootIndex, NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 93789480c5..30cd92f232 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -824,3 +824,9 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon, qemuMonitorQueryStatsTargetType target, char **vcpus, GPtrArray *providers); + +int +qemuMonitorJSONSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- 2.33.0
On Thu, Nov 17, 2022 at 10:05:27 +0800, Jiang Jiacheng wrote:
Introduce qemuDomainChangeBootIndex api to support update device's bootindex. These function will be used in following patches to support change device's (support cdrom, disk and net) bootindex with virsh command like 'virsh update-device <domain> <file> [--flag]'.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_conf.c | 40 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/qemu_monitor.c | 20 ++++++++++++++++++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 33 +++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++++ 6 files changed, 110 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ae5bbcd138..0071a95cb6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1641,3 +1641,43 @@ qemuHugepageMakeBasedir(virQEMUDriver *driver,
return 0; } + +/** + * qemuDomainChangeBootIndex: + * @vm: domain object + * @devInfo: origin device info + * @newBootIndex: new bootIndex + * + * check the alias and bootindex before send the command
This description doesn't make sense. Please describe what effect the function is supposed to have on the VM, not that it does checks. Readers of the code would have to dig deep to figure out what is going on. Example: Live-update the bootindex of device @devInfo. @newBootIndex of 0 disables booting of the given device.
+ * + * Returns: 0 on success, + * -1 otherwise.
Generally this behaviour is assumed, so there's no specific need for this part of the comment.
+ */ +int +qemuDomainChangeBootIndex(virDomainObj *vm, + virDomainDeviceInfo *devInfo, + int newBootIndex) +{ + int ret = -1; + int dummyIndex = -1;
This variable is not needed. You can either pass the constant directly or overwrite teh 'newBootIndex' argument.
+ qemuDomainObjPrivate *priv = vm->privateData; + + if (!devInfo->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("cannot change boot index: device alias not found")); + return -1;
Previously I mentioned that using the shortcut of alias as qom path is not something we do normally. I didn't get to check why libvirt is looking up the full qom path in most cases. Perhaps you can explain why using an alias is sufficient. (E.g. by checking that qemu-4.2 and newer supported it).
+ } + + VIR_DEBUG("Change dev: %s boot index from %d to %d", devInfo->alias, + devInfo->bootIndex, newBootIndex);
Replace the below by: /* To clear the boot index in qemu we must set it to -1 */ if (newBootIndex == 0) newBootIndex = -1; qemuDomainObjEnterMonitor(vm); ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, newBootIndex); qemuDomainObjExitMonitor(vm);
+ + qemuDomainObjEnterMonitor(vm); + /* newBootIndex = 0 means cancel the specified bootIndex, send -1 to qemu instead */ + if (!newBootIndex) + ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, dummyIndex); + else + ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, newBootIndex); + qemuDomainObjExitMonitor(vm); + + return ret; +}
[...]
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 80f262cec7..2b89d9bdae 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4491,3 +4491,23 @@ qemuMonitorGetStatsByQOMPath(virJSONValue *arr,
return NULL; } + +/** + * qemuMonitorSetBootIndex: + * @mon: monitor object + * @alias: device's alias + * @bootIndex: new boot index + * + * Returns data from a call to 'qom-set'
Once again, this description doesn't explain anything here. You either omit it or explain what the actual effect is.
+ */ +int +qemuMonitorSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) +{ + VIR_DEBUG("name=%s, bootIndex=%d", alias, bootIndex); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONSetBootIndex(mon, alias, bootIndex); +}
[...]
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index f4e942a32f..75de73e61b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8890,3 +8890,36 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon,
return virJSONValueObjectStealArray(reply, "return"); } + +/** + * qemuMonitorSetBootIndex: + * @mon: monitor object + * @alias: device's alias + * @bootIndex: new boot index + * + * Set the bootindex of device to new bootIndex + * + * Returns: 0 on success, + * -1 otherwise.
See above.
+ */ +int +qemuMonitorJSONSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-set", "s:path", alias, + "s:property", "bootindex", + "i:value", bootIndex, NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +}
On 2022/11/22 22:36, Peter Krempa Wrote:
On Thu, Nov 17, 2022 at 10:05:27 +0800, Jiang Jiacheng wrote:
Introduce qemuDomainChangeBootIndex api to support update device's bootindex. These function will be used in following patches to support change device's (support cdrom, disk and net) bootindex with virsh command like 'virsh update-device <domain> <file> [--flag]'.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_conf.c | 40 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/qemu_monitor.c | 20 ++++++++++++++++++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 33 +++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++++ 6 files changed, 110 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ae5bbcd138..0071a95cb6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1641,3 +1641,43 @@ qemuHugepageMakeBasedir(virQEMUDriver *driver,
return 0; } + +/** + * qemuDomainChangeBootIndex: + * @vm: domain object + * @devInfo: origin device info + * @newBootIndex: new bootIndex + * + * check the alias and bootindex before send the command
This description doesn't make sense. Please describe what effect the function is supposed to have on the VM, not that it does checks. Readers of the code would have to dig deep to figure out what is going on.
Example:
Live-update the bootindex of device @devInfo. @newBootIndex of 0 disables booting of the given device.
I see, I'll better describe my functions in the next patch.
+ * + * Returns: 0 on success, + * -1 otherwise.
Generally this behaviour is assumed, so there's no specific need for this part of the comment.
+ */ +int +qemuDomainChangeBootIndex(virDomainObj *vm, + virDomainDeviceInfo *devInfo, + int newBootIndex) +{ + int ret = -1; + int dummyIndex = -1;
This variable is not needed. You can either pass the constant directly or overwrite teh 'newBootIndex' argument.
+ qemuDomainObjPrivate *priv = vm->privateData; + + if (!devInfo->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("cannot change boot index: device alias not found")); + return -1;
Previously I mentioned that using the shortcut of alias as qom path is not something we do normally. I didn't get to check why libvirt is looking up the full qom path in most cases. Perhaps you can explain why using an alias is sufficient. (E.g. by checking that qemu-4.2 and newer supported it).
I have tried it on qemu-4.1 and qemu-6.2 and they both work will. And the alias of device is unique to the domain, so I think it's okay to use alias here.
+ } + + VIR_DEBUG("Change dev: %s boot index from %d to %d", devInfo->alias, + devInfo->bootIndex, newBootIndex);
Replace the below by:
/* To clear the boot index in qemu we must set it to -1 */ if (newBootIndex == 0) newBootIndex = -1;
qemuDomainObjEnterMonitor(vm); ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, newBootIndex); qemuDomainObjExitMonitor(vm);
Thanks for your suggestion, I will modify it in next version.
+ + qemuDomainObjEnterMonitor(vm); + /* newBootIndex = 0 means cancel the specified bootIndex, send -1 to qemu instead */ + if (!newBootIndex) + ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, dummyIndex); + else + ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, newBootIndex); + qemuDomainObjExitMonitor(vm); + + return ret; +}
[...]
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 80f262cec7..2b89d9bdae 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4491,3 +4491,23 @@ qemuMonitorGetStatsByQOMPath(virJSONValue *arr,
return NULL; } + +/** + * qemuMonitorSetBootIndex: + * @mon: monitor object + * @alias: device's alias + * @bootIndex: new boot index + * + * Returns data from a call to 'qom-set'
Once again, this description doesn't explain anything here. You either omit it or explain what the actual effect is.
+ */ +int +qemuMonitorSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) +{ + VIR_DEBUG("name=%s, bootIndex=%d", alias, bootIndex); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONSetBootIndex(mon, alias, bootIndex); +}
[...]
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index f4e942a32f..75de73e61b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8890,3 +8890,36 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon,
return virJSONValueObjectStealArray(reply, "return"); } + +/** + * qemuMonitorSetBootIndex: + * @mon: monitor object + * @alias: device's alias + * @bootIndex: new boot index + * + * Set the bootindex of device to new bootIndex + * + * Returns: 0 on success, + * -1 otherwise.
See above.
+ */ +int +qemuMonitorJSONSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-set", "s:path", alias, + "s:property", "bootindex", + "i:value", bootIndex, NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +}
Add a bool bootIndexSpecified into _virDomainDeviceInfo, which means whether the bootindex could be update or not. BootIndexSpecified will be set to true if bootindex is set in XML. Support set bootindex to 0, which means cancel the previous bootindex setting, and format boot order = '0' into XML when its bootIndexSpecified is true to keep its bootindex changeble. Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/device_conf.h | 3 +++ src/conf/domain_conf.c | 9 ++++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index f2907dc596..5259e25c10 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -144,6 +144,9 @@ struct _virDomainDeviceInfo { * not formatted back. This allows HV drivers to update it if <os><boot .. * is present. */ unsigned int effectiveBootIndex; + /* bootIndexSpecified is set to true when device's bootIndex is provided in + * the XML. This allows changing bootIndex online of some devices. */ + bool bootIndexSpecified; /* Valid for any PCI device. Can be used for NIC to get * stable numbering in Linux */ unsigned int acpiIndex; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3790121cf7..dcd9696a93 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5142,7 +5142,10 @@ virDomainDeviceInfoFormat(virBuffer *buf, g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); - if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) { + if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && (info->bootIndex || info->bootIndexSpecified)) { + /* format the boot order = 0 in XML when its bootIndexSpecified is true, + * which means the boot order could be changed by virsh update-device. + */ virBufferAsprintf(buf, "<boot order='%u'", info->bootIndex); if (info->loadparm) @@ -5304,12 +5307,12 @@ virDomainDeviceBootParseXML(xmlNodePtr node, { g_autofree char *loadparm = NULL; - if (virXMLPropUInt(node, "order", 10, - VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, + if (virXMLPropUInt(node, "order", 10, VIR_XML_PROP_REQUIRED, &info->bootIndex) < 0) return -1; info->effectiveBootIndex = info->bootIndex; + info->bootIndexSpecified = true; loadparm = virXMLPropString(node, "loadparm"); if (loadparm) { -- 2.33.0
On Thu, Nov 17, 2022 at 10:05:28 +0800, Jiang Jiacheng wrote:
Add a bool bootIndexSpecified into _virDomainDeviceInfo, which means whether the bootindex could be update or not. BootIndexSpecified will be set to true if bootindex is set in XML. Support set bootindex to 0, which means cancel the previous bootindex setting, and format boot order = '0' into XML when its bootIndexSpecified is true to keep its bootindex changeble.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/device_conf.h | 3 +++ src/conf/domain_conf.c | 9 ++++++--- 2 files changed, 9 insertions(+), 3 deletions(-)
You'll need to note in the documentation the special 0 value and how it behaves when updating a device. In case where you disagree with my comment below that the bootindex should not be in the output XML you'll also need to add XML2XML test cases.
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index f2907dc596..5259e25c10 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -144,6 +144,9 @@ struct _virDomainDeviceInfo { * not formatted back. This allows HV drivers to update it if <os><boot .. * is present. */ unsigned int effectiveBootIndex; + /* bootIndexSpecified is set to true when device's bootIndex is provided in + * the XML. This allows changing bootIndex online of some devices. */ + bool bootIndexSpecified; /* Valid for any PCI device. Can be used for NIC to get * stable numbering in Linux */ unsigned int acpiIndex; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3790121cf7..dcd9696a93 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5142,7 +5142,10 @@ virDomainDeviceInfoFormat(virBuffer *buf, g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
- if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) { + if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && (info->bootIndex || info->bootIndexSpecified)) { + /* format the boot order = 0 in XML when its bootIndexSpecified is true, + * which means the boot order could be changed by virsh update-device. + */
I'm not persuaded that the logic here needs changing. If the boot index is 0 and the device is not bootable we should avoid the element. This includes cases where the device was updated in order co clear the boot index.
virBufferAsprintf(buf, "<boot order='%u'", info->bootIndex);
if (info->loadparm) @@ -5304,12 +5307,12 @@ virDomainDeviceBootParseXML(xmlNodePtr node, { g_autofree char *loadparm = NULL;
- if (virXMLPropUInt(node, "order", 10, - VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, + if (virXMLPropUInt(node, "order", 10, VIR_XML_PROP_REQUIRED, &info->bootIndex) < 0) return -1;
info->effectiveBootIndex = info->bootIndex; + info->bootIndexSpecified = true;
loadparm = virXMLPropString(node, "loadparm"); if (loadparm) { -- 2.33.0
On 2022/11/22 23:02, Peter Krempa wrote:
On Thu, Nov 17, 2022 at 10:05:28 +0800, Jiang Jiacheng wrote:
Add a bool bootIndexSpecified into _virDomainDeviceInfo, which means whether the bootindex could be update or not. BootIndexSpecified will be set to true if bootindex is set in XML. Support set bootindex to 0, which means cancel the previous bootindex setting, and format boot order = '0' into XML when its bootIndexSpecified is true to keep its bootindex changeble.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/device_conf.h | 3 +++ src/conf/domain_conf.c | 9 ++++++--- 2 files changed, 9 insertions(+), 3 deletions(-)
You'll need to note in the documentation the special 0 value and how it behaves when updating a device.
In case where you disagree with my comment below that the bootindex should not be in the output XML you'll also need to add XML2XML test cases.
Well, I think I should organize and describe my patches better. My patches only support update and cancel the bootindex which has been set in the domain'xml, and don't support add bootindex to a device whose bootindex hasn't been set. The new parameter 'bootIndexSpecified' in _virDomainDeviceInfo is used to distinguish whether the bootindex is specified or not. It is set to true only when device's bootindex is specified in domain'xml, which means the device's bootindex could be changed after. The special 0 is used to cancel the bootindex setting. We get bootindex = 0 if the bootindex isn't set or specified to 0 in the device's xml. Update with bootindex = 0 should not effect 'bootIndexSpecified', since the device's bootindex can still be changed after canceling. However, we only get the 'bootIndexSpecified' when parsing the domian's xml, to keep it true with devices whose bootindex is canceld, I format 'boot order = 0' into xml. I do need a XML2XML case here. If all devices' bootindex is set to 0, qemu will boot in an default sequence 'disk->cdrom->net' in those devices.
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index f2907dc596..5259e25c10 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -144,6 +144,9 @@ struct _virDomainDeviceInfo { * not formatted back. This allows HV drivers to update it if <os><boot .. * is present. */ unsigned int effectiveBootIndex; + /* bootIndexSpecified is set to true when device's bootIndex is provided in + * the XML. This allows changing bootIndex online of some devices. */ + bool bootIndexSpecified; /* Valid for any PCI device. Can be used for NIC to get * stable numbering in Linux */ unsigned int acpiIndex; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3790121cf7..dcd9696a93 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5142,7 +5142,10 @@ virDomainDeviceInfoFormat(virBuffer *buf, g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
- if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) { + if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && (info->bootIndex || info->bootIndexSpecified)) { + /* format the boot order = 0 in XML when its bootIndexSpecified is true, + * which means the boot order could be changed by virsh update-device. + */
I'm not persuaded that the logic here needs changing. If the boot index is 0 and the device is not bootable we should avoid the element. This includes cases where the device was updated in order co clear the boot index.
As I mentioned before, the 'boot order = 0' will be format into xml only when bootIndexSpecified is true. BootIndexSpecified is set to true only if the device is bootable and it's bootindex is specified when starting the domain, and it could not be changed after the domain started.
virBufferAsprintf(buf, "<boot order='%u'", info->bootIndex);
if (info->loadparm) @@ -5304,12 +5307,12 @@ virDomainDeviceBootParseXML(xmlNodePtr node, { g_autofree char *loadparm = NULL;
- if (virXMLPropUInt(node, "order", 10, - VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, + if (virXMLPropUInt(node, "order", 10, VIR_XML_PROP_REQUIRED, &info->bootIndex) < 0) return -1;
info->effectiveBootIndex = info->bootIndex; + info->bootIndexSpecified = true;
loadparm = virXMLPropString(node, "loadparm"); if (loadparm) { -- 2.33.0
Introduce qemuCheckBootIndex to check the new bootindex and is it nessary to update the bootindex. Introduce qemuChangeDiskBootIndex to support update disk's bootindex according to different disks' type. Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 7 +++++ 2 files changed, 74 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 0071a95cb6..9a7992db01 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1681,3 +1681,70 @@ qemuDomainChangeBootIndex(virDomainObj *vm, return ret; } + +/** + * qemuCheckBootIndex: + * @devInfo: origin device info + * @new_bootindex: new bootIndex + * + * check whether the device's bootIndex could be changed or neet to + * be changed + * + * Returns: 1 on need to change + * 0 on don't need to change + * -1 on could not change with an error + */ +int +qemuCheckBootIndex(virDomainDeviceInfo *devInfo, + const int new_bootindex) +{ + if (!devInfo->bootIndexSpecified) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("this device does not set boot index, cannot change it.")); + return -1; + } + + /* if the new bootindex is different from the old bootindex, + * we will update the bootindex. */ + if (devInfo->bootIndex != new_bootindex) { + return 1; + } + + return 0; +} + +/** + * qemuChangeDiskBootIndex: + * @vm: domain object + * @orig_disk: the origin disk + * @disk: the new disk to be updated to + * + * check device's type and if its type support update bootIndex, update it. + * + * Returns: 0 on success + * -1 on fail + */ +int +qemuChangeDiskBootIndex(virDomainObj *vm, + virDomainDiskDef *orig_disk, + virDomainDiskDef *disk) +{ + switch (disk->device) { + case VIR_DOMAIN_DISK_DEVICE_CDROM: + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + if (qemuDomainChangeBootIndex(vm, &orig_disk->info, + disk->info.bootIndex) < 0) + return -1; + + orig_disk->info.bootIndex = disk->info.bootIndex; + break; + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + case VIR_DOMAIN_DISK_DEVICE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The boot index of disk bus '%s' cannot be changed."), + virDomainDiskBusTypeToString(disk->bus)); + return -1; + } + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e7447191df..5b05a7392b 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -380,3 +380,10 @@ int qemuDomainChangeBootIndex(virDomainObj *vm, virDomainDeviceInfo *devInfo, int newBootIndex) ATTRIBUTE_NONNULL(2); + +int qemuCheckBootIndex(virDomainDeviceInfo *devInfo, + const int new_bootindex); + +int qemuChangeDiskBootIndex(virDomainObj *vm, + virDomainDiskDef *orig_disk, + virDomainDiskDef *disk); -- 2.33.0
On Thu, Nov 17, 2022 at 10:05:29 +0800, Jiang Jiacheng wrote:
Introduce qemuCheckBootIndex to check the new bootindex and is it nessary to update the bootindex. Introduce qemuChangeDiskBootIndex to support update disk's bootindex according to different disks' type.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 7 +++++
qemu_conf.* seems to be inappropriate file for these helpers. qemu_conf is a module which deals mostly with the configuration of the qemu driver itself and host-side specifics. qemu_domain is usually a better fit.
2 files changed, 74 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 0071a95cb6..9a7992db01 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1681,3 +1681,70 @@ qemuDomainChangeBootIndex(virDomainObj *vm,
return ret; } + +/** + * qemuCheckBootIndex: + * @devInfo: origin device info + * @new_bootindex: new bootIndex + * + * check whether the device's bootIndex could be changed or neet to + * be changed + * + * Returns: 1 on need to change + * 0 on don't need to change + * -1 on could not change with an error + */ +int +qemuCheckBootIndex(virDomainDeviceInfo *devInfo, + const int new_bootindex) +{ + if (!devInfo->bootIndexSpecified) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("this device does not set boot index, cannot change it.")); + return -1;
This check doesn't seem to make much sense. What if you want to add boot index?
+ } + + /* if the new bootindex is different from the old bootindex, + * we will update the bootindex. */ + if (devInfo->bootIndex != new_bootindex) { + return 1; + } + + return 0; +}
On 2022/11/22 23:06, Peter Krempa wrote:
On Thu, Nov 17, 2022 at 10:05:29 +0800, Jiang Jiacheng wrote:
Introduce qemuCheckBootIndex to check the new bootindex and is it nessary to update the bootindex. Introduce qemuChangeDiskBootIndex to support update disk's bootindex according to different disks' type.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 7 +++++
qemu_conf.* seems to be inappropriate file for these helpers. qemu_conf is a module which deals mostly with the configuration of the qemu driver itself and host-side specifics.
qemu_domain is usually a better fit.
Thank for your suggestion, I will move it to qemu_domain in my next patch.
2 files changed, 74 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 0071a95cb6..9a7992db01 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1681,3 +1681,70 @@ qemuDomainChangeBootIndex(virDomainObj *vm,
return ret; } + +/** + * qemuCheckBootIndex: + * @devInfo: origin device info + * @new_bootindex: new bootIndex + * + * check whether the device's bootIndex could be changed or neet to + * be changed + * + * Returns: 1 on need to change + * 0 on don't need to change + * -1 on could not change with an error + */ +int +qemuCheckBootIndex(virDomainDeviceInfo *devInfo, + const int new_bootindex) +{ + if (!devInfo->bootIndexSpecified) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("this device does not set boot index, cannot change it.")); + return -1;
This check doesn't seem to make much sense. What if you want to add boot index?> The bootindex changable devices is fixed when start the domain, so it is not supported to add a boot index, this check is used to prevent them. However, it do have problems as you mentioned in next patch, the logical here should be optimized.
+ } + + /* if the new bootindex is different from the old bootindex, + * we will update the bootindex. */ + if (devInfo->bootIndex != new_bootindex) { + return 1; + } + + return 0; +}
Support to update the disk's bootindex using 'virsh update-device'. Using flag --config or --persistent to change the boot index and the change will be affected after reboot. With --persistent, we can get the result of change immediently, but it still takes effect after reboot. Currently, support update bootindex of disk type as cdrom or disk. Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ef1a9c8c74..8cda2d814d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8234,7 +8234,8 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, /* device alias is checked already in virDomainDefCompatibleDevice */ - CHECK_EQ(info.bootIndex, "boot order", true); + if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) + CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); CHECK_EQ(sgio, "sgio", true); CHECK_EQ(discard, "discard", true); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ff5a743716..65abc04998 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6767,6 +6767,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm, virDomainDiskDef *orig_disk = NULL; virDomainStartupPolicy origStartupPolicy; virDomainDeviceDef oldDev = { .type = dev->type }; + int needChangeIndex = 0; if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6784,6 +6785,10 @@ qemuDomainChangeDiskLive(virDomainObj *vm, if (!qemuDomainDiskChangeSupported(disk, orig_disk)) return -1; + if ((needChangeIndex = qemuCheckBootIndex(&orig_disk->info, + disk->info.bootIndex)) < 0) + return -1; + if (!virStorageSourceIsSameLocation(disk->src, orig_disk->src)) { /* Disk source can be changed only for removable devices */ if (disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM && @@ -6807,6 +6812,11 @@ qemuDomainChangeDiskLive(virDomainObj *vm, dev->data.disk->src = NULL; } + if (needChangeIndex) { + if (qemuChangeDiskBootIndex(vm, orig_disk, disk) < 0) + return -1; + } + /* in case when we aren't updating disk source we update startup policy here */ orig_disk->startupPolicy = dev->data.disk->startupPolicy; orig_disk->snapshot = dev->data.disk->snapshot; @@ -7503,6 +7513,24 @@ qemuDomainUpdateDeviceConfig(virDomainDef *vmdef, false) < 0) return -1; + switch (vmdef->disks[pos]->device) { + case VIR_DOMAIN_DISK_DEVICE_CDROM: + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + if (qemuCheckBootIndex(&vmdef->disks[pos]->info, + newDisk->info.bootIndex) < 0) + return -1; + break; + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + case VIR_DOMAIN_DISK_DEVICE_LAST: + if ((newDisk->info.bootIndex) && + (vmdef->disks[pos]->info.bootIndex != newDisk->info.bootIndex)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("this disk doesn't support update")); + return -1; + } + } + virDomainDiskDefFree(vmdef->disks[pos]); vmdef->disks[pos] = newDisk; dev->data.disk = NULL; -- 2.33.0
On Thu, Nov 17, 2022 at 10:05:30 +0800, Jiang Jiacheng wrote:
Support to update the disk's bootindex using 'virsh update-device'. Using flag --config or --persistent to change the boot index and the change will be affected after reboot. With --persistent, we can get the result of change immediently, but it still takes effect after reboot. Currently, support update bootindex of disk type as cdrom or disk.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-)
[...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ff5a743716..65abc04998 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6767,6 +6767,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm, virDomainDiskDef *orig_disk = NULL; virDomainStartupPolicy origStartupPolicy; virDomainDeviceDef oldDev = { .type = dev->type }; + int needChangeIndex = 0;
if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6784,6 +6785,10 @@ qemuDomainChangeDiskLive(virDomainObj *vm, if (!qemuDomainDiskChangeSupported(disk, orig_disk)) return -1;
+ if ((needChangeIndex = qemuCheckBootIndex(&orig_disk->info, + disk->info.bootIndex)) < 0) + return -1;
Due to the weird check I complained about in previous patch, this actively breaks media change (and other parameter change) for disks which don't have bootindex.
+ if (!virStorageSourceIsSameLocation(disk->src, orig_disk->src)) { /* Disk source can be changed only for removable devices */ if (disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM && @@ -6807,6 +6812,11 @@ qemuDomainChangeDiskLive(virDomainObj *vm, dev->data.disk->src = NULL; }
+ if (needChangeIndex) { + if (qemuChangeDiskBootIndex(vm, orig_disk, disk) < 0) + return -1; + } + /* in case when we aren't updating disk source we update startup policy here */ orig_disk->startupPolicy = dev->data.disk->startupPolicy; orig_disk->snapshot = dev->data.disk->snapshot; @@ -7503,6 +7513,24 @@ qemuDomainUpdateDeviceConfig(virDomainDef *vmdef, false) < 0) return -1;
+ switch (vmdef->disks[pos]->device) { + case VIR_DOMAIN_DISK_DEVICE_CDROM: + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + if (qemuCheckBootIndex(&vmdef->disks[pos]->info, + newDisk->info.bootIndex) < 0) + return -1; + break; + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + case VIR_DOMAIN_DISK_DEVICE_LAST: + if ((newDisk->info.bootIndex) && + (vmdef->disks[pos]->info.bootIndex != newDisk->info.bootIndex)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("this disk doesn't support update")); + return -1; + } + }
This also doesn't make sense. You don't have the same issue updating the bootindex of a floppy. Similarly, the qemuCheckBootIndex check is also not applicable as the next boot config willr esult in a new boot where you can change anything.
On 2022/11/22 23:17, Peter Krempa wrote:
On Thu, Nov 17, 2022 at 10:05:30 +0800, Jiang Jiacheng wrote:
Support to update the disk's bootindex using 'virsh update-device'. Using flag --config or --persistent to change the boot index and the change will be affected after reboot. With --persistent, we can get the result of change immediently, but it still takes effect after reboot. Currently, support update bootindex of disk type as cdrom or disk.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-)
[...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ff5a743716..65abc04998 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6767,6 +6767,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm, virDomainDiskDef *orig_disk = NULL; virDomainStartupPolicy origStartupPolicy; virDomainDeviceDef oldDev = { .type = dev->type }; + int needChangeIndex = 0;
if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6784,6 +6785,10 @@ qemuDomainChangeDiskLive(virDomainObj *vm, if (!qemuDomainDiskChangeSupported(disk, orig_disk)) return -1;
+ if ((needChangeIndex = qemuCheckBootIndex(&orig_disk->info, + disk->info.bootIndex)) < 0) + return -1;
Due to the weird check I complained about in previous patch, this actively breaks media change (and other parameter change) for disks which don't have bootindex.
This check do have this problem , I should fix it. I only want to return -1 when add a bootindex for a device whose bootindex is not set before.
+ if (!virStorageSourceIsSameLocation(disk->src, orig_disk->src)) { /* Disk source can be changed only for removable devices */ if (disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM && @@ -6807,6 +6812,11 @@ qemuDomainChangeDiskLive(virDomainObj *vm, dev->data.disk->src = NULL; }
+ if (needChangeIndex) { + if (qemuChangeDiskBootIndex(vm, orig_disk, disk) < 0) + return -1; + } + /* in case when we aren't updating disk source we update startup policy here */ orig_disk->startupPolicy = dev->data.disk->startupPolicy; orig_disk->snapshot = dev->data.disk->snapshot; @@ -7503,6 +7513,24 @@ qemuDomainUpdateDeviceConfig(virDomainDef *vmdef, false) < 0) return -1;
+ switch (vmdef->disks[pos]->device) { + case VIR_DOMAIN_DISK_DEVICE_CDROM: + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + if (qemuCheckBootIndex(&vmdef->disks[pos]->info, + newDisk->info.bootIndex) < 0) + return -1; + break; + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + case VIR_DOMAIN_DISK_DEVICE_LAST: + if ((newDisk->info.bootIndex) && + (vmdef->disks[pos]->info.bootIndex != newDisk->info.bootIndex)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("this disk doesn't support update")); + return -1; + } + }
This also doesn't make sense. You don't have the same issue updating the bootindex of a floppy. Similarly, the qemuCheckBootIndex check is also not applicable as the next boot config willr esult in a new boot where you can change anything.
My patches don't support change floppy's bootindex, so it will return an error if the bootindex is changed in its xml.
Support to update the net's bootindex using 'virsh update-device'. Using flag --config or --persistent to change the boot index and the change will be affected after reboot. With --persistent, we can get the result of change immediently, but it still takes effect after reboot. Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_driver.c | 4 ++++ src/qemu/qemu_hotplug.c | 17 ++++++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65abc04998..863b779514 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7568,6 +7568,10 @@ qemuDomainUpdateDeviceConfig(virDomainDef *vmdef, false) < 0) return -1; + if (qemuCheckBootIndex(&vmdef->nets[pos]->info, + net->info.bootIndex) < 0) + return -1; + if (virDomainNetUpdate(vmdef, pos, net)) return -1; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index da92ced2f4..5d1913d0c7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3468,6 +3468,7 @@ qemuDomainChangeNet(virQEMUDriver *driver, bool needCoalesceChange = false; bool needVlanUpdate = false; bool needIsolatedPortChange = false; + int needChangeIndex = 0; int ret = -1; int changeidx = -1; g_autoptr(virConnect) conn = NULL; @@ -3633,11 +3634,8 @@ qemuDomainChangeNet(virQEMUDriver *driver, goto cleanup; } - if (newdev->info.bootIndex == 0) - newdev->info.bootIndex = olddev->info.bootIndex; - if (olddev->info.bootIndex != newdev->info.bootIndex) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot modify network device boot index setting")); + if ((needChangeIndex = qemuCheckBootIndex(&olddev->info, + newdev->info.bootIndex)) < 0) { goto cleanup; } @@ -3918,6 +3916,15 @@ qemuDomainChangeNet(virQEMUDriver *driver, needReplaceDevDef = true; } + if (needChangeIndex) { + if (qemuDomainChangeBootIndex(vm, &olddev->info, newdev->info.bootIndex) < 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. -- 2.33.0
On Thu, Nov 17, 2022 at 10:05:31 +0800, Jiang Jiacheng wrote:
Support to update the net's bootindex using 'virsh update-device'. Using flag --config or --persistent to change the boot index and the change will be affected after reboot. With --persistent, we can get the result of change immediently, but it still takes effect after reboot.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_driver.c | 4 ++++ src/qemu/qemu_hotplug.c | 17 ++++++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65abc04998..863b779514 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7568,6 +7568,10 @@ qemuDomainUpdateDeviceConfig(virDomainDef *vmdef, false) < 0) return -1;
+ if (qemuCheckBootIndex(&vmdef->nets[pos]->info, + net->info.bootIndex) < 0) + return -1;
Same as with disks. This check makes no sense for the *Config variant.
+ if (virDomainNetUpdate(vmdef, pos, net)) return -1;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index da92ced2f4..5d1913d0c7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3468,6 +3468,7 @@ qemuDomainChangeNet(virQEMUDriver *driver, bool needCoalesceChange = false; bool needVlanUpdate = false; bool needIsolatedPortChange = false; + int needChangeIndex = 0; int ret = -1; int changeidx = -1; g_autoptr(virConnect) conn = NULL; @@ -3633,11 +3634,8 @@ qemuDomainChangeNet(virQEMUDriver *driver, goto cleanup; }
- if (newdev->info.bootIndex == 0) - newdev->info.bootIndex = olddev->info.bootIndex;
You are deleting too much of the logic here, this code specifically ensures that if the new device's boot index is not specified it adopts the old one, thus this needs to be changed suich that it's still adopted in cases when an explicit new boot index was not specified.
- if (olddev->info.bootIndex != newdev->info.bootIndex) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot modify network device boot index setting")); + if ((needChangeIndex = qemuCheckBootIndex(&olddev->info, + newdev->info.bootIndex)) < 0) {
This once again (due to the weird check) breaks update of network devices which don't have bootindex enabled.
goto cleanup; }
On 2022/11/22 23:40, Peter Krempa wrote:
On Thu, Nov 17, 2022 at 10:05:31 +0800, Jiang Jiacheng wrote:
Support to update the net's bootindex using 'virsh update-device'. Using flag --config or --persistent to change the boot index and the change will be affected after reboot. With --persistent, we can get the result of change immediently, but it still takes effect after reboot.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_driver.c | 4 ++++ src/qemu/qemu_hotplug.c | 17 ++++++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65abc04998..863b779514 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7568,6 +7568,10 @@ qemuDomainUpdateDeviceConfig(virDomainDef *vmdef, false) < 0) return -1;
+ if (qemuCheckBootIndex(&vmdef->nets[pos]->info, + net->info.bootIndex) < 0) + return -1;
Same as with disks. This check makes no sense for the *Config variant.
+ if (virDomainNetUpdate(vmdef, pos, net)) return -1;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index da92ced2f4..5d1913d0c7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3468,6 +3468,7 @@ qemuDomainChangeNet(virQEMUDriver *driver, bool needCoalesceChange = false; bool needVlanUpdate = false; bool needIsolatedPortChange = false; + int needChangeIndex = 0; int ret = -1; int changeidx = -1; g_autoptr(virConnect) conn = NULL; @@ -3633,11 +3634,8 @@ qemuDomainChangeNet(virQEMUDriver *driver, goto cleanup; }
- if (newdev->info.bootIndex == 0) - newdev->info.bootIndex = olddev->info.bootIndex;
You are deleting too much of the logic here, this code specifically ensures that if the new device's boot index is not specified it adopts the old one, thus this needs to be changed suich that it's still adopted in cases when an explicit new boot index was not specified.
I use 'boot order = 0' or new bootindex was not specified to cancel the bootindex setting. It seems not specified bootindex means inherit the old bootindex? But I can't find similiar code with disk, is this only used for net?
- if (olddev->info.bootIndex != newdev->info.bootIndex) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot modify network device boot index setting")); + if ((needChangeIndex = qemuCheckBootIndex(&olddev->info, + newdev->info.bootIndex)) < 0) {
This once again (due to the weird check) breaks update of network devices which don't have bootindex enabled.
See above.
goto cleanup; }
Support add bootindex = 0 to boothash and return 0 if duplicated bootindex = 0 is set. It is nessary to add bootindex = 0 into boothash, otherwise libvirt will auto set boot dev, which will cause bootindex unchangable. Meanwhile, bootindex = 0 means disable bootindex, so it should be allowed to set duplicated bootindex = 0. Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/domain_postparse.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_postparse.c b/src/conf/domain_postparse.c index 9a3e8f494c..2ba3186561 100644 --- a/src/conf/domain_postparse.c +++ b/src/conf/domain_postparse.c @@ -1031,7 +1031,7 @@ virDomainDefCollectBootOrder(virDomainDef *def G_GNUC_UNUSED, GHashTable *bootHash = data; g_autofree char *order = NULL; - if (info->bootIndex == 0) + if (info->bootIndex == 0 && !info->bootIndexSpecified) return 0; if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV && @@ -1045,6 +1045,12 @@ virDomainDefCollectBootOrder(virDomainDef *def G_GNUC_UNUSED, order = g_strdup_printf("%u", info->bootIndex); if (virHashLookup(bootHash, order)) { + /* 0 in the bootHash means cancel the bootIndex specified in + * XML, so it allowed to make more than one device use 0 with + * its bootIndexSpecified = true. + */ + if (info->bootIndex == 0) + return 0; virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("boot order '%s' used for more than one device"), order); -- 2.33.0
On Thu, Nov 17, 2022 at 10:05:32 +0800, Jiang Jiacheng wrote:
Support add bootindex = 0 to boothash and return 0 if duplicated bootindex = 0 is set. It is nessary to add bootindex = 0 into boothash, otherwise libvirt will auto set boot dev, which will cause bootindex unchangable. Meanwhile, bootindex = 0 means disable bootindex, so it should be allowed to set duplicated bootindex = 0.
Libvirt auto-sets boot order when there is none specified. With this patch you could un-select all bootable devices and end up with an un-bootable VM. Do you really want to allow that? Also doing this will require that we hohour the explicit bootindex setting of 0 in the XML separate from when it's unpopulated. Thus this requires that the XML indeed records that an explicit value of 0 was provided by the user. Thus you must improve patch 2 to carry the appropriate tests and documentation if you want to go this route.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/domain_postparse.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
On 2022/11/22 23:52, Peter Krempa wrote:
On Thu, Nov 17, 2022 at 10:05:32 +0800, Jiang Jiacheng wrote:
Support add bootindex = 0 to boothash and return 0 if duplicated bootindex = 0 is set. It is nessary to add bootindex = 0 into boothash, otherwise libvirt will auto set boot dev, which will cause bootindex unchangable. Meanwhile, bootindex = 0 means disable bootindex, so it should be allowed to set duplicated bootindex = 0.
Libvirt auto-sets boot order when there is none specified. With this patch you could un-select all bootable devices and end up with an un-bootable VM. Do you really want to allow that?
Also doing this will require that we hohour the explicit bootindex setting of 0 in the XML separate from when it's unpopulated. Thus this requires that the XML indeed records that an explicit value of 0 was provided by the user.
Thus you must improve patch 2 to carry the appropriate tests and documentation if you want to go this route.
Yeah, this what I mean in patch2. Qemu will boot in an default sequence 'disk->cdrom->net' in those devices whose bootindex is set to explicit 0. I will improve all those patches in next version.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/domain_postparse.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
If the 'boot order' is not specified in the xml, we cancel its bootindex setting and set it to '0'. However, the bootIndexSpecified will be set to false because we cannot parse boot order from XML, so copy bootIndexSpecified from origin device when updating it. Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_conf.c | 33 +++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_driver.c | 3 +++ 3 files changed, 40 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 9a7992db01..517fc813f2 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1748,3 +1748,36 @@ qemuChangeDiskBootIndex(virDomainObj *vm, } return 0; } + +/** + * qemuDomainDefCopyBootState: + * @dev: pointer to device parsed from xml + * @vmdef: pointer to vm definition structure + * + * copy the bootIndexSpecified from vmDef's device info + * + * Returns: 0 on success or no need to copy + * -1 on fail to find the device + */ +int +qemuDomainDefCopyBootState(virDomainDeviceDef *dev, virDomainDef *vmDef) +{ + int idx = -1; + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + if ((idx = virDomainDiskIndexByName(vmDef, dev->data.disk->dst, false)) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("target %s doesn't exist."), dev->data.disk->info.alias); + return -1; + } + dev->data.disk->info.bootIndexSpecified = vmDef->disks[idx]->info.bootIndexSpecified; + } + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + if ((idx = virDomainNetFindIdx(vmDef, dev->data.net)) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("target %s doesn't exist."), dev->data.net->info.alias); + return -1; + } + dev->data.net->info.bootIndexSpecified = vmDef->nets[idx]->info.bootIndexSpecified; + } + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 5b05a7392b..c1e9a2120f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -387,3 +387,7 @@ int qemuCheckBootIndex(virDomainDeviceInfo *devInfo, int qemuChangeDiskBootIndex(virDomainObj *vm, virDomainDiskDef *orig_disk, virDomainDiskDef *disk); + + +int qemuDomainDefCopyBootState(virDomainDeviceDef *dev, + virDomainDef *vmDef); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 863b779514..3dbf95041e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7841,6 +7841,9 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (dev == NULL) goto endjob; + if (qemuDomainDefCopyBootState(dev, vm->def) < 0) + goto endjob; + if (flags & VIR_DOMAIN_AFFECT_CONFIG && flags & VIR_DOMAIN_AFFECT_LIVE) { /* If we are affecting both CONFIG and LIVE -- 2.33.0
On Thu, Nov 17, 2022 at 10:05:33 +0800, Jiang Jiacheng wrote:
If the 'boot order' is not specified in the xml, we cancel its bootindex setting and set it to '0'. However, the bootIndexSpecified will be set to false because we cannot parse boot order from XML, so copy bootIndexSpecified from origin device when updating it.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_conf.c | 33 +++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_driver.c | 3 +++ 3 files changed, 40 insertions(+)
Rather than doing this qemuDomainUpdateDeviceFlags can parse the XML twice as it's acutally less work than invoking the copy function (Which re-formats the XML and parses it again). I'll post patches to adress that in qemuDomainUpdateDeviceFlags. You can drop this patch.
On 2022/11/22 23:53, Peter Krempa wrote:
On Thu, Nov 17, 2022 at 10:05:33 +0800, Jiang Jiacheng wrote:
If the 'boot order' is not specified in the xml, we cancel its bootindex setting and set it to '0'. However, the bootIndexSpecified will be set to false because we cannot parse boot order from XML, so copy bootIndexSpecified from origin device when updating it.
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_conf.c | 33 +++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_driver.c | 3 +++ 3 files changed, 40 insertions(+)
Rather than doing this qemuDomainUpdateDeviceFlags can parse the XML twice as it's acutally less work than invoking the copy function (Which re-formats the XML and parses it again). I'll post patches to adress that in qemuDomainUpdateDeviceFlags. You can drop this patch.
Thanks for your review and suggetions, I will improve my patches in next version.
participants (2)
-
Jiang Jiacheng -
Peter Krempa