Re: [PATCH qemu v2 1/3] hw/s390x: add CPI identifiers to QOM

["Daniel P. Berrangé" <berrange@redhat.com>]

BTW, your email client is possibly mis-configured - your mail
came through with "From: shalini <shalini@imap.linux.ibm.com>"
and attempting to reply to that gets an error saying that
'imap.linux.ibm.com' does not exist.

On Thu, Mar 06, 2025 at 02:55:27PM +0100, shalini wrote:
> On 2025-03-05 17:06, Daniel P. Berrangé wrote:
> > On Mon, Feb 24, 2025 at 01:04:47PM +0100, Shalini Chellathurai Saroja
> > wrote:
> > > Add Control-Program Identification (CPI) to the QEMU Object
> > > Model (QOM). The CPI identifiers provide information about
> > > the guest operating system. The CPI identifiers are:
> > > system type, system name, system level and sysplex name.
> > > 
> > > The system type provides the OS type of the guest (e.g. LINUX).
> > > The system name provides the name of the guest (e.g. TESTVM).
> > > The system level provides the distribution and kernel version
> > > of the guest OS (e.g. 0x50e00).
> > > The sysplex name provides the sysplex name of the guest
> > > (e.g. SYSPLEX).
> > > 
> > > Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
> > > ---
> > >  hw/s390x/s390-virtio-ccw.c         | 29 +++++++++++++++++++++++++++++
> > >  include/hw/s390x/s390-virtio-ccw.h |  8 ++++++++
> > >  qapi/machine.json                  | 24 ++++++++++++++++++++++++
> > >  3 files changed, 61 insertions(+)
> > > 
> > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > > index 51ae0c133d..13ea8db1b0 100644
> > > --- a/hw/s390x/s390-virtio-ccw.c
> > > +++ b/hw/s390x/s390-virtio-ccw.c
> > > @@ -50,6 +50,7 @@
> > >  #include "hw/s390x/virtio-ccw-md.h"
> > >  #include "system/replay.h"
> > >  #include CONFIG_DEVICES
> > > +#include "qapi/qapi-visit-machine.h"
> > > 
> > >  static Error *pv_mig_blocker;
> > > 
> > > @@ -803,6 +804,26 @@ static void machine_set_loadparm(Object *obj,
> > > Visitor *v,
> > >      s390_ipl_fmt_loadparm(ms->loadparm, val, errp);
> > >  }
> > > 
> > > +static void machine_get_control_program_id(Object *obj, Visitor *v,
> > > +                                           const char *name, void
> > > *opaque,
> > > +                                           Error **errp)
> > > +{
> > > +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> > > +    S390ControlProgramId *cpi;
> > > +    cpi = &(S390ControlProgramId){
> > > +        .system_type = g_strndup((char *) ms->cpi.system_type,
> > > +                       sizeof(ms->cpi.system_type)),
> > > +        .system_name = g_strndup((char *) ms->cpi.system_name,
> > > +                       sizeof(ms->cpi.system_name)),
> > > +        .system_level = g_strdup_printf("0x%lx",
> > > ms->cpi.system_level),
> > 
> > If the data is an integer, we must return it in QMP as an integer,
> > not formatted into a hex string.
> > 
> 
> Hello Daniel,
> 
> Thank you very much for the review.
> 
> The system-level, when interpreted as an int provides the output below
> 
> 'system-level': 74872343805430528

Yes, that is correct from a QMP design POV. Data should formatted
in the most appropriate way for machines to consume, using native
JSON data types.

> But the desired output below is obtained only when interpreted as a str.
> 
> 'system-level': '0x10a000000060b00'

If a human wants to read the data in hex format, that should be
formatted by whatever tool is consuming the data from QMP and
presenting it in the user.

> > > +# @system-name: system name of Linux instance
> > 
> > What is a system name ?  Is that a hostname, or is that something
> > else ?
> > 
> 
> Yes, it is the hostname of the guest virtual machine.

Lets rename it to 'system-hostname' to be unambiguous.


With 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 :|