[PATCH 0/4] qemu: Fix handling of 'copy-on-read' on hotplug with blockdev
See patch 4. Peter Krempa (4): qemuMonitorJSONBlockdevAdd: Refactor cleanup qemuMonitorJSONBlockdevDel: Refactor cleanup qemuMonitorBlockdevAdd: Take double pointer argument qemu: hotplug: Fix handling of the 'copy-on-read' layer with blockdev src/qemu/qemu_block.c | 14 ++------------ src/qemu/qemu_hotplug.c | 25 +++++++++++++++--------- src/qemu/qemu_monitor.c | 16 ++++++---------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 37 +++++++++++++----------------------- src/qemu/qemu_monitor_json.h | 2 +- 6 files changed, 39 insertions(+), 57 deletions(-) -- 2.24.1
Use automatic variable freeing and get rid of the cleanup section. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index dc1fc310ca..0d66f32887 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8811,25 +8811,19 @@ int qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon, virJSONValuePtr props) { - virJSONValuePtr cmd; - virJSONValuePtr reply = NULL; - int ret = -1; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-add", props))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; } -- 2.24.1
Use automatic variable freeing and get rid of the cleanup section. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 0d66f32887..95b5ab6968 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8831,9 +8831,8 @@ int qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon, const char *nodename) { - virJSONValuePtr cmd; - virJSONValuePtr reply = NULL; - int ret = -1; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; if (!(cmd = qemuMonitorJSONMakeCommand("blockdev-del", "s:node-name", nodename, @@ -8841,17 +8840,12 @@ qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon, return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; } -- 2.24.1
Modify qemuMonitorBlockdevAdd so that it takes a double pointer for the @props argument so that it's cleared inside the call. This allows writing cleaner callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 14 ++------------ src/qemu/qemu_monitor.c | 16 ++++++---------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 5 +++-- src/qemu/qemu_monitor_json.h | 2 +- 5 files changed, 13 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 63116ef5f2..fa6c6f98e3 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1530,13 +1530,8 @@ static int qemuBlockStorageSourceAttachApplyStorage(qemuMonitorPtr mon, qemuBlockStorageSourceAttachDataPtr data) { - int rv; - if (data->storageProps) { - rv = qemuMonitorBlockdevAdd(mon, data->storageProps); - data->storageProps = NULL; - - if (rv < 0) + if (qemuMonitorBlockdevAdd(mon, &data->storageProps) < 0) return -1; data->storageAttached = true; @@ -1563,13 +1558,8 @@ static int qemuBlockStorageSourceAttachApplyFormat(qemuMonitorPtr mon, qemuBlockStorageSourceAttachDataPtr data) { - int rv; - if (data->formatProps) { - rv = qemuMonitorBlockdevAdd(mon, data->formatProps); - data->formatProps = NULL; - - if (rv < 0) + if (qemuMonitorBlockdevAdd(mon, &data->formatProps) < 0) return -1; data->formatAttached = true; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ceedcd527a..463e24657a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4390,23 +4390,19 @@ qemuMonitorBlockdevCreate(qemuMonitorPtr mon, * @mon: monitor object * @props: JSON object describing the blockdev to add * - * Adds a new block device (BDS) to qemu. Note that @props is always consumed - * by this function and should not be accessed after calling this function. + * Adds a new block device (BDS) to qemu. Note that *@props is consumed + * and set to NULL on success. */ int qemuMonitorBlockdevAdd(qemuMonitorPtr mon, - virJSONValuePtr props) + virJSONValuePtr *props) { - VIR_DEBUG("props=%p (node-name=%s)", props, - NULLSTR(virJSONValueObjectGetString(props, "node-name"))); + VIR_DEBUG("props=%p (node-name=%s)", *props, + NULLSTR(virJSONValueObjectGetString(*props, "node-name"))); - QEMU_CHECK_MONITOR_GOTO(mon, error); + QEMU_CHECK_MONITOR(mon); return qemuMonitorJSONBlockdevAdd(mon, props); - - error: - virJSONValueFree(props); - return -1; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cca2cdcb27..6a6b8efaee 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1323,7 +1323,7 @@ int qemuMonitorBlockdevCreate(qemuMonitorPtr mon, virJSONValuePtr props); int qemuMonitorBlockdevAdd(qemuMonitorPtr mon, - virJSONValuePtr props); + virJSONValuePtr *props); int qemuMonitorBlockdevDel(qemuMonitorPtr mon, const char *nodename); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 95b5ab6968..815d17520a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8809,12 +8809,13 @@ qemuMonitorJSONBlockdevCreate(qemuMonitorPtr mon, int qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon, - virJSONValuePtr props) + virJSONValuePtr *props) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; + virJSONValuePtr pr = g_steal_pointer(props); - if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-add", props))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-add", pr))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 61f5b0061d..fd2e09025e 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -597,7 +597,7 @@ int qemuMonitorJSONBlockdevCreate(qemuMonitorPtr mon, ATTRIBUTE_NONNULL(1); int qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon, - virJSONValuePtr props) + virJSONValuePtr *props) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon, -- 2.24.1
My original implementation was completely broken because it attempted to use object-add/del instead of blockdev-add/del. https://bugzilla.redhat.com/show_bug.cgi?id=1798366 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 39bcb70f83..c840889968 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -680,6 +680,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virJSONValue) corProps = NULL; g_autofree char *corAlias = NULL; + bool corAdded = false; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) @@ -692,9 +693,12 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, goto cleanup; if (blockdev) { - if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON && - !(corProps = qemuBlockStorageGetCopyOnReadProps(disk))) - goto cleanup; + if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON) { + if (!(corProps = qemuBlockStorageGetCopyOnReadProps(disk))) + goto cleanup; + + corAlias = g_strdup(QEMU_DOMAIN_DISK_PRIVATE(disk)->nodeCopyOnRead); + } if (!(data = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->src, priv->qemuCaps))) @@ -719,9 +723,12 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuBlockStorageSourceChainAttach(priv->mon, data) < 0) goto exit_monitor; - if (corProps && - qemuMonitorAddObject(priv->mon, &corProps, &corAlias) < 0) - goto exit_monitor; + if (corProps) { + if (qemuMonitorBlockdevAdd(priv->mon, &corProps) < 0) + goto exit_monitor; + + corAdded = true; + } if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) goto exit_monitor; @@ -763,8 +770,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, return ret; exit_monitor: - if (corAlias) - ignore_value(qemuMonitorDelObject(priv->mon, corAlias)); + if (corAdded) + ignore_value(qemuMonitorBlockdevDel(priv->mon, corAlias)); qemuBlockStorageSourceChainDetach(priv->mon, data); if (qemuDomainObjExitMonitor(driver, vm) < 0) @@ -4250,7 +4257,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); if (corAlias) - ignore_value(qemuMonitorDelObject(priv->mon, corAlias)); + ignore_value(qemuMonitorBlockdevDel(priv->mon, corAlias)); if (diskBackend) qemuBlockStorageSourceChainDetach(priv->mon, diskBackend); -- 2.24.1
On 2/6/20 9:54 AM, Peter Krempa wrote:
See patch 4.
Peter Krempa (4): qemuMonitorJSONBlockdevAdd: Refactor cleanup qemuMonitorJSONBlockdevDel: Refactor cleanup qemuMonitorBlockdevAdd: Take double pointer argument qemu: hotplug: Fix handling of the 'copy-on-read' layer with blockdev
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
src/qemu/qemu_block.c | 14 ++------------ src/qemu/qemu_hotplug.c | 25 +++++++++++++++--------- src/qemu/qemu_monitor.c | 16 ++++++---------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 37 +++++++++++++----------------------- src/qemu/qemu_monitor_json.h | 2 +- 6 files changed, 39 insertions(+), 57 deletions(-)
On Thu, Feb 06, 2020 at 01:54:15PM +0100, Peter Krempa wrote:
See patch 4.
Peter Krempa (4): qemuMonitorJSONBlockdevAdd: Refactor cleanup qemuMonitorJSONBlockdevDel: Refactor cleanup qemuMonitorBlockdevAdd: Take double pointer argument qemu: hotplug: Fix handling of the 'copy-on-read' layer with blockdev
src/qemu/qemu_block.c | 14 ++------------ src/qemu/qemu_hotplug.c | 25 +++++++++++++++--------- src/qemu/qemu_monitor.c | 16 ++++++---------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 37 +++++++++++++----------------------- src/qemu/qemu_monitor_json.h | 2 +- 6 files changed, 39 insertions(+), 57 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Daniel Henrique Barboza -
Ján Tomko -
Peter Krempa