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

2014-05-14 Thread Michael Mueller
On Tue, 13 May 2014 09:29:37 -0600
Eric Blake ebl...@redhat.com 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


signature.asc
Description: PGP signature


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

2014-05-13 Thread Eric Blake
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.

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
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 }

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature