This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999

Signed-off-by: Chris Venteicher <cventeic@redhat.com>

diff to v1:
- Replaced c++ style comments with c style
- qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} use To/From form
- qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} return pointers not integers
- VIR_STEAL_PTR form used
- switch statement uses virReportEnumRangeError
- qemuMonitorJSONHasError(reply, "GenericError") treated as no info, not fatal error
- virJSONValueFree(cpu_props) during error case

diff to v2:
- qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON}
works on {"name": "IvyBridge", "props": {}} 
rather than  {"model": {"name": "IvyBridge", "props": {}}}
- additional cleanups and fixes

Chris Venteicher (3):
  qemu_monitor_json: Populate CPUModelInfo struct from json
  qemu_monitor_json: Build Json CPU Model Info
  qemu_monitor: query-cpu-model-baseline QMP command

 src/qemu/qemu_monitor.c      |  13 ++++
 src/qemu/qemu_monitor.h      |   5 ++
 src/qemu/qemu_monitor_json.c | 179 +++++++++++++++++++++++++++++++++++++------
 src/qemu/qemu_monitor_json.h |   7 ++
 4 files changed, 182 insertions(+), 22 deletions(-)

-- 
2.14.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 04/30/2018 10:55 PM, Chris Venteicher wrote:
> This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
> 
> Signed-off-by: Chris Venteicher <cventeic@redhat.com>
> 
> diff to v1:
> - Replaced c++ style comments with c style
> - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} use To/From form
> - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} return pointers not integers
> - VIR_STEAL_PTR form used
> - switch statement uses virReportEnumRangeError
> - qemuMonitorJSONHasError(reply, "GenericError") treated as no info, not fatal error
> - virJSONValueFree(cpu_props) during error case
> 
> diff to v2:
> - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON}
> works on {"name": "IvyBridge", "props": {}} 
> rather than  {"model": {"name": "IvyBridge", "props": {}}}
> - additional cleanups and fixes
> 
> Chris Venteicher (3):
>   qemu_monitor_json: Populate CPUModelInfo struct from json
>   qemu_monitor_json: Build Json CPU Model Info
>   qemu_monitor: query-cpu-model-baseline QMP command
> 
>  src/qemu/qemu_monitor.c      |  13 ++++
>  src/qemu/qemu_monitor.h      |   5 ++
>  src/qemu/qemu_monitor_json.c | 179 +++++++++++++++++++++++++++++++++++++------
>  src/qemu/qemu_monitor_json.h |   7 ++
>  4 files changed, 182 insertions(+), 22 deletions(-)
> 

Changes look good to me,

Reviewed-by: John Ferlan <jferlan@redhat.com>

I have to wait for 4.4.0 to open before pushing, but it would also be
good to know what the plans are for code that will use the new
functions. Do you plan on posting something for 4.4.0?

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Quoting John Ferlan (2018-05-01 08:57:08)
> 
> 
> On 04/30/2018 10:55 PM, Chris Venteicher wrote:
> > This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
> > 
> > Signed-off-by: Chris Venteicher <cventeic@redhat.com>
> > 
> > diff to v1:
> > - Replaced c++ style comments with c style
> > - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} use To/From form
> > - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} return pointers not integers
> > - VIR_STEAL_PTR form used
> > - switch statement uses virReportEnumRangeError
> > - qemuMonitorJSONHasError(reply, "GenericError") treated as no info, not fatal error
> > - virJSONValueFree(cpu_props) during error case
> > 
> > diff to v2:
> > - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON}
> > works on {"name": "IvyBridge", "props": {}} 
> > rather than  {"model": {"name": "IvyBridge", "props": {}}}
> > - additional cleanups and fixes
> > 
> > Chris Venteicher (3):
> >   qemu_monitor_json: Populate CPUModelInfo struct from json
> >   qemu_monitor_json: Build Json CPU Model Info
> >   qemu_monitor: query-cpu-model-baseline QMP command
> > 
> >  src/qemu/qemu_monitor.c      |  13 ++++
> >  src/qemu/qemu_monitor.h      |   5 ++
> >  src/qemu/qemu_monitor_json.c | 179 +++++++++++++++++++++++++++++++++++++------
> >  src/qemu/qemu_monitor_json.h |   7 ++
> >  4 files changed, 182 insertions(+), 22 deletions(-)
> > 
> 
> Changes look good to me,
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> I have to wait for 4.4.0 to open before pushing, but it would also be
> good to know what the plans are for code that will use the new
> functions. Do you plan on posting something for 4.4.0?

I plan on posting something yet this week that uses this qemu_monitor interface.

It will likely take some rounds of feedback before the new patch set is ready to 
be accepted.  Not sure how this maps to 4.4.0 window.

However, I don't think there is much that could happen with the new patch set 
that would change the need for the qemu_monitor interface exposed by this patch 
set.

Thanks, Chris

> 
> John
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, May 01, 2018 at 09:57:08 -0400, John Ferlan wrote:
> 
> 
> On 04/30/2018 10:55 PM, Chris Venteicher wrote:
> > This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
> > 
> > Signed-off-by: Chris Venteicher <cventeic@redhat.com>
> > 
> > diff to v1:
> > - Replaced c++ style comments with c style
> > - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} use To/From form
> > - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} return pointers not integers
> > - VIR_STEAL_PTR form used
> > - switch statement uses virReportEnumRangeError
> > - qemuMonitorJSONHasError(reply, "GenericError") treated as no info, not fatal error
> > - virJSONValueFree(cpu_props) during error case
> > 
> > diff to v2:
> > - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON}
> > works on {"name": "IvyBridge", "props": {}} 
> > rather than  {"model": {"name": "IvyBridge", "props": {}}}
> > - additional cleanups and fixes
> > 
> > Chris Venteicher (3):
> >   qemu_monitor_json: Populate CPUModelInfo struct from json
> >   qemu_monitor_json: Build Json CPU Model Info
> >   qemu_monitor: query-cpu-model-baseline QMP command
> > 
> >  src/qemu/qemu_monitor.c      |  13 ++++
> >  src/qemu/qemu_monitor.h      |   5 ++
> >  src/qemu/qemu_monitor_json.c | 179 +++++++++++++++++++++++++++++++++++++------
> >  src/qemu/qemu_monitor_json.h |   7 ++
> >  4 files changed, 182 insertions(+), 22 deletions(-)
> > 
> 
> Changes look good to me,
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> I have to wait for 4.4.0 to open before pushing, but it would also be
> good to know what the plans are for code that will use the new
> functions. Do you plan on posting something for 4.4.0?

I don't think pushing this makes a lot of sense without any users.
Without seeing the complete picture from a public APIs down to the
monitor code it's a bit hard to see if the code fits together nicely. We
have two approaches now... Chris makes the monitor APIs work on
CPUModelInfo while Collin's APIs use virCPUDef. We need to see how this
all fits into the public APIs first.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 05/02/2018 03:43 AM, Jiri Denemark wrote:
> On Tue, May 01, 2018 at 09:57:08 -0400, John Ferlan wrote:
>>
>>
>> On 04/30/2018 10:55 PM, Chris Venteicher wrote:
>>> This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
>>>
>>> Signed-off-by: Chris Venteicher <cventeic@redhat.com>
>>>
>>> diff to v1:
>>> - Replaced c++ style comments with c style
>>> - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} use To/From form
>>> - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} return pointers not integers
>>> - VIR_STEAL_PTR form used
>>> - switch statement uses virReportEnumRangeError
>>> - qemuMonitorJSONHasError(reply, "GenericError") treated as no info, not fatal error
>>> - virJSONValueFree(cpu_props) during error case
>>>
>>> diff to v2:
>>> - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON}
>>> works on {"name": "IvyBridge", "props": {}} 
>>> rather than  {"model": {"name": "IvyBridge", "props": {}}}
>>> - additional cleanups and fixes
>>>
>>> Chris Venteicher (3):
>>>   qemu_monitor_json: Populate CPUModelInfo struct from json
>>>   qemu_monitor_json: Build Json CPU Model Info
>>>   qemu_monitor: query-cpu-model-baseline QMP command
>>>
>>>  src/qemu/qemu_monitor.c      |  13 ++++
>>>  src/qemu/qemu_monitor.h      |   5 ++
>>>  src/qemu/qemu_monitor_json.c | 179 +++++++++++++++++++++++++++++++++++++------
>>>  src/qemu/qemu_monitor_json.h |   7 ++
>>>  4 files changed, 182 insertions(+), 22 deletions(-)
>>>
>>
>> Changes look good to me,
>>
>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>>
>> I have to wait for 4.4.0 to open before pushing, but it would also be
>> good to know what the plans are for code that will use the new
>> functions. Do you plan on posting something for 4.4.0?
> 
> I don't think pushing this makes a lot of sense without any users.
> Without seeing the complete picture from a public APIs down to the
> monitor code it's a bit hard to see if the code fits together nicely. We
> have two approaches now... Chris makes the monitor APIs work on
> CPUModelInfo while Collin's APIs use virCPUDef. We need to see how this
> all fits into the public APIs first.
> 
> Jirka
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

I agree with Jiri. In their current state, I think your patches look fine but
I'll hold off on giving my r-b's until I see how they get used.

Great progress! Looking forward to seeing how it all hooks up.


-- 
Respectfully,
- Collin Walling

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 05/02/2018 03:34 PM, Collin Walling wrote:
> On 05/02/2018 03:43 AM, Jiri Denemark wrote:
>> On Tue, May 01, 2018 at 09:57:08 -0400, John Ferlan wrote:
>>>
>>>
>>> On 04/30/2018 10:55 PM, Chris Venteicher wrote:
>>>> This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
>>>>
>>>> Signed-off-by: Chris Venteicher <cventeic@redhat.com>
>>>>
>>>> diff to v1:
>>>> - Replaced c++ style comments with c style
>>>> - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} use To/From form
>>>> - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON} return pointers not integers
>>>> - VIR_STEAL_PTR form used
>>>> - switch statement uses virReportEnumRangeError
>>>> - qemuMonitorJSONHasError(reply, "GenericError") treated as no info, not fatal error
>>>> - virJSONValueFree(cpu_props) during error case
>>>>
>>>> diff to v2:
>>>> - qemuMonitorJSONGetCPUModelInfo{ToJSON,FromJSON}
>>>> works on {"name": "IvyBridge", "props": {}} 
>>>> rather than  {"model": {"name": "IvyBridge", "props": {}}}
>>>> - additional cleanups and fixes
>>>>
>>>> Chris Venteicher (3):
>>>>   qemu_monitor_json: Populate CPUModelInfo struct from json
>>>>   qemu_monitor_json: Build Json CPU Model Info
>>>>   qemu_monitor: query-cpu-model-baseline QMP command
>>>>
>>>>  src/qemu/qemu_monitor.c      |  13 ++++
>>>>  src/qemu/qemu_monitor.h      |   5 ++
>>>>  src/qemu/qemu_monitor_json.c | 179 +++++++++++++++++++++++++++++++++++++------
>>>>  src/qemu/qemu_monitor_json.h |   7 ++
>>>>  4 files changed, 182 insertions(+), 22 deletions(-)
>>>>
>>>
>>> Changes look good to me,
>>>
>>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>>>
>>> I have to wait for 4.4.0 to open before pushing, but it would also be
>>> good to know what the plans are for code that will use the new
>>> functions. Do you plan on posting something for 4.4.0?
>>
>> I don't think pushing this makes a lot of sense without any users.
>> Without seeing the complete picture from a public APIs down to the
>> monitor code it's a bit hard to see if the code fits together nicely. We
>> have two approaches now... Chris makes the monitor APIs work on
>> CPUModelInfo while Collin's APIs use virCPUDef. We need to see how this
>> all fits into the public APIs first.
>>
>> Jirka
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
> 
> I agree with Jiri. In their current state, I think your patches look fine but
> I'll hold off on giving my r-b's until I see how they get used.
> 
> Great progress! Looking forward to seeing how it all hooks up.
> 
> 

Fine by me to wait - yes, it's a bit different order to adjust the
plumbing first...  At least if we had to make changes there it wouldn't
affect user/external API's ;-)

Perhaps it's "easier" to process patches in small groups rather than 15,
20, 35, 75, etc. patch bomb series.  Just going to have to deal with the
coordination of two people. Hopefully it gets done soon while things are
relatively fresh in mind!

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list