On 9/4/25 14:10, Stefan Kober wrote:
> Refactor BuildDiskJson to return a virJSONValue instead of adding the
> disk json to an json array. This makes the function reusable for
> hotplugging disks.
>
> On-behalf-of: SAP stefan.kober@xxxxxxx
> Signed-off-by: Stefan Kober <stefan.kober@xxxxxxxxxxxxxxxxxxxxx>
> ---
> src/ch/ch_monitor.c | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index d369236183..f65cca648b 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -234,43 +234,41 @@ virCHMonitorBuildMemoryJson(virJSONValue *content, virDomainDef *vmdef)
> return 0;
> }
>
> -static int
> -virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef)
> +static virJSONValue*
> +virCHMonitorBuildDiskJson(virDomainDiskDef *diskdef)
> {
> g_autoptr(virJSONValue) disk = virJSONValueNewObject();
>
> if (!diskdef->src)
> - return -1;
> + return NULL;
>
> switch (diskdef->src->type) {
> case VIR_STORAGE_TYPE_FILE:
> if (!diskdef->src->path) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("Missing disk file path in domain"));
> - return -1;
> + return NULL;
> }
> if (!diskdef->info.alias) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Missing disk alias"));
> - return -1;
> + return NULL;
> }
> if (diskdef->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
> virReportError(VIR_ERR_INVALID_ARG,
> _("Only virtio bus types are supported for '%1$s'"),
> diskdef->src->path);
> - return -1;
> + return NULL;
> }
> if (virJSONValueObjectAppendString(disk, "path", diskdef->src->path) < 0)
> - return -1;
> + return NULL;
> if (diskdef->src->readonly) {
> if (virJSONValueObjectAppendBoolean(disk, "readonly", true) < 0)
> - return -1;
> + return NULL;
> }
> if (virJSONValueObjectAppendString(disk, "id", diskdef->info.alias) < 0) {
> - return -1;
> + return NULL;
> }
> - if (virJSONValueArrayAppend(disks, &disk) < 0)
> - return -1;
>
> break;
> case VIR_STORAGE_TYPE_NONE:
> @@ -284,23 +282,26 @@ virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef)
> case VIR_STORAGE_TYPE_LAST:
> default:
> virReportEnumRangeError(virStorageType, diskdef->src->type);
> - return -1;
> + return NULL;
> }
>
> - return 0;
> + return g_steal_pointer(&disk);
> }
>
> static int
> virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef)
> {
> g_autoptr(virJSONValue) disks = NULL;
> + g_autoptr(virJSONValue) disk = NULL;
This variable ...
> size_t i;
>
> if (vmdef->ndisks > 0) {
> disks = virJSONValueNewArray();
>
> for (i = 0; i < vmdef->ndisks; i++) {
... is used exclusively in this loop. Declaring it here not only
improves readability (I don't have to hold yet another variable inside
my head when reading this function) but also avoids unexpected reuse of
the variable.
In fact, this whole function could be written a bit better. One level of
nesting could be dropped, but that's outside of the scope of this
particular patch.
> - if (virCHMonitorBuildDiskJson(disks, vmdef->disks[i]) < 0)
> + if ((disk = virCHMonitorBuildDiskJson(vmdef->disks[i])) == NULL)
> + return -1;
> + if (virJSONValueArrayAppend(disks, &disk) < 0)
> return -1;
> }
> if (virJSONValueObjectAppend(content, "disks", &disks) < 0)
Michal
