Re: [libvirt] [PATCH 06/11] qemu: Add monitor functions to set IOThread params

On 10/19/2018 03:09 PM, John Ferlan wrote: > > > On 10/19/18 7:06 AM, Michal Privoznik wrote: >> On 10/07/2018 03:00 PM, John Ferlan wrote: >>> Add functions to set the IOThreadInfo param data for the live guest. >>> >>> Based on code originally posted by Pavel Hrdina <phrdina(a)redhat.com&gt;, >>> but extracted into a separate patch. Note that qapi expects to receive >>> integer parameters rather than unsigned long long or unsigned int's. >>> QEMU does save the value in larger signed 64 bit values eventually. >>> >>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>> --- >>> src/qemu/qemu_monitor.c | 19 +++++++++++++++++++ >>> src/qemu/qemu_monitor.h | 2 ++ >>> src/qemu/qemu_monitor_json.c | 33 +++++++++++++++++++++++++++++++++ >>> src/qemu/qemu_monitor_json.h | 4 ++++ >>> 4 files changed, 58 insertions(+) >>> >>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c >>> index 7f7013e115..a65d638ab8 100644 >>> --- a/src/qemu/qemu_monitor.c >>> +++ b/src/qemu/qemu_monitor.c >>> @@ -4135,6 +4135,25 @@ qemuMonitorGetIOThreads(qemuMonitorPtr mon, >>> } >>> >>> >>> +/** >>> + * qemuMonitorSetIOThread: >>> + * @mon: Pointer to the monitor >>> + * @iothreadInfo: filled IOThread info with data >>> + * >>> + * Alter the specified IOThread's IOThreadInfo values. >>> + */ >>> +int >>> +qemuMonitorSetIOThread(qemuMonitorPtr mon, >>> + qemuMonitorIOThreadInfoPtr iothreadInfo) >>> +{ >>> + VIR_DEBUG("iothread=%p", iothreadInfo); >>> + >>> + QEMU_CHECK_MONITOR(mon); >>> + >>> + return qemuMonitorJSONSetIOThread(mon, iothreadInfo); >>> +} >>> + >>> + >>> /** >>> * qemuMonitorGetMemoryDeviceInfo: >>> * @mon: pointer to the monitor >>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h >>> index c2991e2b16..ef71fc6448 100644 >>> --- a/src/qemu/qemu_monitor.h >>> +++ b/src/qemu/qemu_monitor.h >>> @@ -1123,6 +1123,8 @@ struct _qemuMonitorIOThreadInfo { >>> }; >>> int qemuMonitorGetIOThreads(qemuMonitorPtr mon, >>> qemuMonitorIOThreadInfoPtr **iothreads); >>> +int qemuMonitorSetIOThread(qemuMonitorPtr mon, >>> + qemuMonitorIOThreadInfoPtr iothreadInfo); >>> >>> typedef struct _qemuMonitorMemoryDeviceInfo qemuMonitorMemoryDeviceInfo; >>> typedef qemuMonitorMemoryDeviceInfo *qemuMonitorMemoryDeviceInfoPtr; >>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c >>> index 2e92984b44..bb1d62b844 100644 >>> --- a/src/qemu/qemu_monitor_json.c >>> +++ b/src/qemu/qemu_monitor_json.c >>> @@ -7474,6 +7474,39 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon, >>> } >>> >>> >>> +int >>> +qemuMonitorJSONSetIOThread(qemuMonitorPtr mon, >>> + qemuMonitorIOThreadInfoPtr iothreadInfo) >>> +{ >>> + int ret = -1; >>> + char *path = NULL; >>> + qemuMonitorJSONObjectProperty prop; >>> + >>> + if (virAsprintf(&path, "/objects/iothread%u", >>> + iothreadInfo->iothread_id) < 0) >>> + goto cleanup; >>> + >>> +#define VIR_IOTHREAD_SET_PROP(propName, propVal) \ >>> + memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); \ >>> + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; \ >>> + prop.val.iv = propVal; \ >>> + if (qemuMonitorJSONSetObjectProperty(mon, path, propName, &prop) < 0) \ >>> + goto cleanup; >>> + >>> + VIR_IOTHREAD_SET_PROP("poll-max-ns", iothreadInfo->poll_max_ns) >>> + VIR_IOTHREAD_SET_PROP("poll-grow", iothreadInfo->poll_grow) >>> + VIR_IOTHREAD_SET_PROP("poll-shrink", iothreadInfo->poll_shrink) >> >> So this is all or nothing approach. All values are set - even though >> user might request through public API to change just one. I don't think >> it is a good design. Either we need a monitor API that changes just one >> value and call it for every typed parameter that user sends to us, or >> public API implementation must copy the old values into this struct >> (even though it would be ugly). >> >> Michal >> > > Fair complaint - I tried to reuse as much as possible from the initial > series so that I didn't "waste" time implementing something that in the > long run wasn't desired. Originally there were lots of checks about what > was or wasn't set - I just took the path of least resistance. > > It should be simple to add flag for each to determine which was set > before setting them in the object path. > > IOW: > > #define VIR_IOTHREAD_SET_PROP(propName, propVal) \ > if (iothreadInfo->set_##propVal) { \ This will fly iff zero value can't be set on the monitor. I have no idea whether it can or can not. If it can be set, then we have to go with something more clever. Michal