[PATCH 1/7] qemu: Introduce qemuDomainChangeBootIndex API

[Peter Krempa]

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 at 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;
> +}