[PATCH 1/7] qemu: Introduce qemuDomainChangeBootIndex API

[Jiang Jiacheng]

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