[Qemu-devel] [PATCH] hmp.c: Revert hmp_info_cpus output format

[Qemu-devel] [PATCH] hmp.c: Revert hmp_info_cpus output format

[sathnaga]

From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>

This commit 137b5cb6ab565cb3781d5337591e155932b4230e
refactors info cpus output and changes output format from
'thread_id' to 'thread-id', this would break parsing
of output in above layers like libvirt, test framework etc.

This patch just reverts back output format to 'thread_id'.

CC: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
---
 hmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hmp.c b/hmp.c
index 679467d85a..a25c7bd9a8 100644
--- a/hmp.c
+++ b/hmp.c
@@ -381,7 +381,7 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
 
         monitor_printf(mon, "%c CPU #%" PRId64 ":", active,
                        cpu->value->cpu_index);
-        monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
+        monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id);
     }
 
     qapi_free_CpuInfoFastList(cpu_list);
-- 
2.14.3

Re: [Qemu-devel] [PATCH] hmp.c: Revert hmp_info_cpus output format

[Daniel P. Berrangé]

On Tue, Mar 27, 2018 at 06:08:00PM +0530, sathnaga@linux.vnet.ibm.com wrote:
> From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> 
> This commit 137b5cb6ab565cb3781d5337591e155932b4230e
> refactors info cpus output and changes output format from
> 'thread_id' to 'thread-id', this would break parsing
> of output in above layers like libvirt, test framework etc.

Libvirt doesn't use HMP, only QMP, so this shouldn't have broken libvirt
afaik.  HMP is not intended to be a stable API so these kind of changes
should be expected.

> 
> This patch just reverts back output format to 'thread_id'.
> 
> CC: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> ---
>  hmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 679467d85a..a25c7bd9a8 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -381,7 +381,7 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>  
>          monitor_printf(mon, "%c CPU #%" PRId64 ":", active,
>                         cpu->value->cpu_index);
> -        monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
> +        monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id);
>      }
>  
>      qapi_free_CpuInfoFastList(cpu_list);
> -- 
> 2.14.3
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] hmp.c: Revert hmp_info_cpus output format

[Daniel P. Berrangé]

Re-adding qemu-devel address - please don't drop the mailing list from
CC's when replying to messages on the list.

On Tue, Mar 27, 2018 at 06:46:19PM +0530, satheesh rajendran wrote:
> On Tue, Mar 27, 2018 at 01:40:00PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Mar 27, 2018 at 06:08:00PM +0530, sathnaga@linux.vnet.ibm.com wrote:
> > > From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > > 
> > > This commit 137b5cb6ab565cb3781d5337591e155932b4230e
> > > refactors info cpus output and changes output format from
> > > 'thread_id' to 'thread-id', this would break parsing
> > > of output in above layers like libvirt, test framework etc.
> > 
> > Libvirt doesn't use HMP, only QMP, so this shouldn't have broken libvirt
> > afaik.  HMP is not intended to be a stable API so these kind of changes
> > should be expected.
> 
> Am sure avocado test framework uses this output for parsing to extract threadid

If avocado is relying on the HMP then it should be updated to use QMP,
as HMP is not a stable API for automation.

> and in libvirt atleast I checked below code uses it, I hope keeping the
> format same as previous won't affect anything?
> 
> in src/qemu/qemu_monitor_text.c
> int
> qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
>                          struct qemuMonitorQueryCpusEntry **entries,
>                          size_t *nentries)
> ...

This libvirt code is only executed when managing QEMU versions < 0.15,
so not affected.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] hmp.c: Revert hmp_info_cpus output format

[satheesh rajendran]

On Tue, Mar 27, 2018 at 01:40:00PM +0100, Daniel P. Berrangé wrote:
> On Tue, Mar 27, 2018 at 06:08:00PM +0530, sathnaga@linux.vnet.ibm.com wrote:
> > From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > 
> > This commit 137b5cb6ab565cb3781d5337591e155932b4230e
> > refactors info cpus output and changes output format from
> > 'thread_id' to 'thread-id', this would break parsing
> > of output in above layers like libvirt, test framework etc.
> 
> Libvirt doesn't use HMP, only QMP, so this shouldn't have broken libvirt
> afaik.  HMP is not intended to be a stable API so these kind of changes
> should be expected.

Am sure avocado test framework uses this output for parsing to extract threadid
and in libvirt atleast I checked below code uses it, I hope keeping the
format same as previous won't affect anything?

in src/qemu/qemu_monitor_text.c
int
qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
                         struct qemuMonitorQueryCpusEntry **entries,
                         size_t *nentries)
...
    if (qemuMonitorHMPCommand(mon, "info cpus", &qemucpus) < 0)
        return -1;

    /*
     * This is the gross format we're about to parse :-{
     *
     * (qemu) info cpus
     * * CPU #0: pc=0x00000000000f0c4a thread_id=30019
     *   CPU #1: pc=0x00000000fffffff0 thread_id=30020
     *   CPU #2: pc=0x00000000fffffff0 (halted) thread_id=30021
     *
     */
....
        /* Extract host Thread ID */
        if ((offset = strstr(line, "thread_id=")) == NULL)

Regards,
-Satheesh
> 
> > 
> > This patch just reverts back output format to 'thread_id'.
> > 
> > CC: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> > Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > ---
> >  hmp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hmp.c b/hmp.c
> > index 679467d85a..a25c7bd9a8 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -381,7 +381,7 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
> >  
> >          monitor_printf(mon, "%c CPU #%" PRId64 ":", active,
> >                         cpu->value->cpu_index);
> > -        monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
> > +        monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id);
> >      }
> >  
> >      qapi_free_CpuInfoFastList(cpu_list);
> > -- 
> > 2.14.3
> > 
> > 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [Qemu-devel] [PATCH] hmp.c: Revert hmp_info_cpus output format

[satheesh rajendran]

On Tue, Mar 27, 2018 at 02:20:11PM +0100, Daniel P. Berrangé wrote:
> Re-adding qemu-devel address - please don't drop the mailing list from
> CC's when replying to messages on the list.
Sorry, Missed intially, had resent it but was too late :-(.

> 
> On Tue, Mar 27, 2018 at 06:46:19PM +0530, satheesh rajendran wrote:
> > On Tue, Mar 27, 2018 at 01:40:00PM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Mar 27, 2018 at 06:08:00PM +0530, sathnaga@linux.vnet.ibm.com wrote:
> > > > From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > > > 
> > > > This commit 137b5cb6ab565cb3781d5337591e155932b4230e
> > > > refactors info cpus output and changes output format from
> > > > 'thread_id' to 'thread-id', this would break parsing
> > > > of output in above layers like libvirt, test framework etc.
> > > 
> > > Libvirt doesn't use HMP, only QMP, so this shouldn't have broken libvirt
> > > afaik.  HMP is not intended to be a stable API so these kind of changes
> > > should be expected.
> > 
> > Am sure avocado test framework uses this output for parsing to extract threadid
> 
> If avocado is relying on the HMP then it should be updated to use QMP,
> as HMP is not a stable API for automation.
> 
Sure, Thanks, will change it.

> > and in libvirt atleast I checked below code uses it, I hope keeping the
> > format same as previous won't affect anything?
> > 
> > in src/qemu/qemu_monitor_text.c
> > int
> > qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
> >                          struct qemuMonitorQueryCpusEntry **entries,
> >                          size_t *nentries)
> > ...
> 
> This libvirt code is only executed when managing QEMU versions < 0.15,
> so not affected.
>
Okies :-)

Regards,
-Satheesh
 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [Qemu-devel] [PATCH for-2.12] hmp.c: Revert hmp_info_cpus output format

[Eric Blake]

On 03/27/2018 07:38 AM, sathnaga@linux.vnet.ibm.com wrote:
> From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> 
> This commit 137b5cb6ab565cb3781d5337591e155932b4230e
> refactors info cpus output and changes output format from
> 'thread_id' to 'thread-id', this would break parsing
> of output in above layers like libvirt, test framework etc.
> 
> This patch just reverts back output format to 'thread_id'.
> 
> CC: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> ---
>   hmp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

I'm not opposed to this patch (because it is a trivial way to keep older 
stuff working), but agree with Daniel that HMP output can change at any 
time, so other stuff relying on HMP should be fixed.  Libvirt is not 
affected, so if we DO take this into 2.12, we should update the commit 
message to drop mention of libvirt being impacted, maybe along the lines of:

Commit 137b5cb6 refactored 'info cpus' output, changing 'thread_id' to 
'thread-id'.  While HMP is not a stable interface, it is trivial to keep 
the spelling consistent for test frameworks that have not yet updated to 
using QMP.

With the improved commit message,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] hmp.c: Revert hmp_info_cpus output format

[Eric Blake]

[adding the maintainer in cc]

On 03/27/2018 07:38 AM, sathnaga@linux.vnet.ibm.com wrote:
> From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> 
> This commit 137b5cb6ab565cb3781d5337591e155932b4230e
> refactors info cpus output and changes output format from
> 'thread_id' to 'thread-id', this would break parsing
> of output in above layers like libvirt, test framework etc.
> 
> This patch just reverts back output format to 'thread_id'.
> 
> CC: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> ---
>   hmp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

I can take this through my qapi tree if David is happy with it

> 
> diff --git a/hmp.c b/hmp.c
> index 679467d85a..a25c7bd9a8 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -381,7 +381,7 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>   
>           monitor_printf(mon, "%c CPU #%" PRId64 ":", active,
>                          cpu->value->cpu_index);
> -        monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
> +        monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id);
>       }
>   
>       qapi_free_CpuInfoFastList(cpu_list);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] hmp.c: Revert hmp_info_cpus output format

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> [adding the maintainer in cc]
> 
> On 03/27/2018 07:38 AM, sathnaga@linux.vnet.ibm.com wrote:
> > From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > 
> > This commit 137b5cb6ab565cb3781d5337591e155932b4230e
> > refactors info cpus output and changes output format from
> > 'thread_id' to 'thread-id', this would break parsing
> > of output in above layers like libvirt, test framework etc.
> > 
> > This patch just reverts back output format to 'thread_id'.
> > 
> > CC: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> > Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > ---
> >   hmp.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I can take this through my qapi tree if David is happy with it

Yes, with the commit message change you suggested.


Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> > 
> > diff --git a/hmp.c b/hmp.c
> > index 679467d85a..a25c7bd9a8 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -381,7 +381,7 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
> >           monitor_printf(mon, "%c CPU #%" PRId64 ":", active,
> >                          cpu->value->cpu_index);
> > -        monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
> > +        monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id);
> >       }
> >       qapi_free_CpuInfoFastList(cpu_list);
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-2.12] hmp.c: Revert hmp_info_cpus output format

[Cornelia Huck]

On Tue, 27 Mar 2018 08:59:04 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 03/27/2018 07:38 AM, sathnaga@linux.vnet.ibm.com wrote:
> > From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > 
> > This commit 137b5cb6ab565cb3781d5337591e155932b4230e
> > refactors info cpus output and changes output format from
> > 'thread_id' to 'thread-id', this would break parsing
> > of output in above layers like libvirt, test framework etc.
> > 
> > This patch just reverts back output format to 'thread_id'.
> > 
> > CC: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> > Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > ---
> >   hmp.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)  
> 
> I'm not opposed to this patch (because it is a trivial way to keep older 
> stuff working), but agree with Daniel that HMP output can change at any 
> time, so other stuff relying on HMP should be fixed.  Libvirt is not 
> affected, so if we DO take this into 2.12, we should update the commit 
> message to drop mention of libvirt being impacted, maybe along the lines of:
> 
> Commit 137b5cb6 refactored 'info cpus' output, changing 'thread_id' to 
> 'thread-id'.  While HMP is not a stable interface, it is trivial to keep 
> the spelling consistent for test frameworks that have not yet updated to 
> using QMP.
> 
> With the improved commit message,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Yes, that certainly makes sense.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH for-2.12] hmp.c: Revert hmp_info_cpus output format

[Viktor Mihajlovski]

On 27.03.2018 16:25, Cornelia Huck wrote:
> On Tue, 27 Mar 2018 08:59:04 -0500
> Eric Blake <eblake@redhat.com> wrote:
> 
>> On 03/27/2018 07:38 AM, sathnaga@linux.vnet.ibm.com wrote:
>>> From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
>>>
>>> This commit 137b5cb6ab565cb3781d5337591e155932b4230e
>>> refactors info cpus output and changes output format from
>>> 'thread_id' to 'thread-id', this would break parsing
>>> of output in above layers like libvirt, test framework etc.
>>>
>>> This patch just reverts back output format to 'thread_id'.
>>>
>>> CC: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
>>> Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
>>> ---
>>>   hmp.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)  
>>
>> I'm not opposed to this patch (because it is a trivial way to keep older 
>> stuff working), but agree with Daniel that HMP output can change at any 
>> time, so other stuff relying on HMP should be fixed.  Libvirt is not 
>> affected, so if we DO take this into 2.12, we should update the commit 
>> message to drop mention of libvirt being impacted, maybe along the lines of:
>>
>> Commit 137b5cb6 refactored 'info cpus' output, changing 'thread_id' to 
>> 'thread-id'.  While HMP is not a stable interface, it is trivial to keep 
>> the spelling consistent for test frameworks that have not yet updated to 
>> using QMP.
>>
>> With the improved commit message,
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
> 
> Yes, that certainly makes sense.
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
Right. Seems I got carried away during the thread_id to thread-id
conversion.

-- 
Regards,
 Viktor Mihajlovski