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

[Michal Prívozník]

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 at redhat.com>,
>>> 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 at redhat.com>
>>> ---
>>>  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