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@xxxxxxxxxx>
>> ---
>> 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;
>> +}
>
