Re: [Qemu-devel] [PATCH v1 RFC 09/10] QEMU: s390: cpu model QMP query-cpu-model

[Michael Mueller]

On Tue, 13 May 2014 09:29:37 -0600
Eric Blake <eblake@xxxxxxxxxx> wrote:

Hi Eric,

> On 05/13/2014 09:00 AM, Michael Mueller wrote:
> > This patch implements a new QMP request named "query-cpu-model". It returns
> > the cpu model of cpu 0. eg:
> > 
> > request: '{"execute" : "query-cpu-model" }
> > answer:  '{"return" : {"name": "2827-ga2"}}
> > 
> > Alias names are resolved to their respective machine type and GA names
> > already during cpu instantiation. Thus, also a cpu name like "host",
> > which is implemented as alias, will return its normalized cpu model name.
> > 
> 
> > +    }
> > +    cpu_model = g_try_malloc0(sizeof(*cpu_model));
> 
> It's simpler to just use g_malloc0 and rely on glibc's exit-on-OOM
> behavior than to try and deal with NULL - this isn't user input (so
> unlikely to be so huge as to cause OOM), and would be more in line with
> what most other QMP code does.  But that said...
> 
> > +    if (!cpu_model) {
> > +        goto out_no_mem;
> > +    }
> > +    cpu_model->name = g_try_malloc0(CPU_MODEL_NAME_LEN);
> > +    if (!cpu_model->name) {
> > +        goto out_no_mem;
> > +    }
> > +    snprintf(cpu_model->name, CPU_MODEL_NAME_LEN - 1, "%04x-ga%u",
> > +             cc->proc->type, cc->mach->ga);
> 
> ...why not just use g_strdup_printf() instead of trying to size a buffer
> yourself?  In other words, skip the g_try_malloc0 to begin with.

I will use that function and I think I can use it with "query-cpu-definitions"
as well.

> 
> The fact that you are packing two pieces of information into one string
> is a bit worrisome - that means that the client of the QMP command has
> to parse the string back into two pieces of information if they ever
> need either item in isolation.  If the user never has a need to split
> the name down into parts, you are okay; I don't know S390 well enough to

I see your point, but I consider it as a name only, which needs to be unique
to identify different configurations but has no meaning on the management
interface level.

> know whether anyone will care about type separate from ga.  But if
> someone DOES care, then the QMP command should return the parts already
> split, as in { "type": 2827, "ga": 2 }, or even as convenience provide
> both split and combined forms: { "name": "2827-ga2", "type": 2827, "ga": 2 }
> 

Offering both options seems to be desirable but I have to talk with libvirt if that
could be done in a transparent form for them.

Thanks a lot for your comments
Michael

Attachment: signature.asc
Description: PGP signature