[libvirt] [PATCH] lib: implement new API to retrieve the list of CPU models

Eric Blake eblake at redhat.com
Wed Aug 21 19:19:24 UTC 2013


On 08/21/2013 01:02 PM, Giuseppe Scrivano wrote:
> The new function virConnectGetCPUModelNames allows to retrieve the list
> of CPU models known by the hypervisor for a specific architecture.
> 
> Signed-off-by: Giuseppe Scrivano <gscrivan at redhat.com>
> ---
> I have collected your comments on my RFC patch into this new version.  I've
> replaced "virConnectGetCPUMapDesc" with "virConnectGetCPUModelNames".

Good that the above paragraph is below the ---...

> 
> The new function signature is:
> 
> int
> virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char **models,
>                            unsigned int flags);
> 
> It returns (in MODELS) the list of CPU models formatted as an XML document,
> like:
> 
> <models>
>   <arch name='x86'>
>     <model name='486'/>
>     <model name='pentium'/>
>     <model name='pentium2'/>
>     <model name='pentium3'/>
>     <model name='pentiumpro'/>
>     <model name='coreduo'/>
>     ...
>   </arch>
> </models>

...but this is useful 6 months down the road, and should be in the
commit message proper, above the ---.

I'm not sure whether returning XML or a straight-up list makes more
sense.  If you used char ***models, then the user would get an array of
directly-usable strings, "486", "pentium", ..., instead of a document
that has to be parsed.  On the other hand, your idea of returning XML
lets us return information for multiple arches simultaneously. But do we
need that flexibility, since arch is also an input parameter?  Is the
idea that you pass arch=NULL to get the full list, or arch="x86" to get
the x86 subset of the xml?  Why not just make arch mandatory and return
char ***; but then you have the question of which arches are supported.

So, let's get agreement on the best design before worrying about
implementation (I'm still 50/50 on whether xml vs. char*** makes more
sense, without more discussion to sway me one way or the other).

> 
>  daemon/remote.c                 | 36 +++++++++++++++++++++++++++
>  include/libvirt/libvirt.h.in    | 18 ++++++++++++++
>  python/generator.py             |  1 +
>  python/libvirt-override-api.xml |  7 ++++++
>  python/libvirt-override.c       | 28 +++++++++++++++++++++
>  python/libvirt-override.py      | 11 +++++++++
>  src/cpu/cpu.c                   | 55 +++++++++++++++++++++++++++++++++++++++++
>  src/cpu/cpu.h                   |  3 +++
>  src/driver.h                    |  7 ++++++
>  src/libvirt.c                   | 42 +++++++++++++++++++++++++++++++
>  src/libvirt_private.syms        |  1 +
>  src/libvirt_public.syms         |  5 ++++
>  src/qemu/qemu_driver.c          | 14 +++++++++++
>  src/remote/remote_driver.c      | 40 ++++++++++++++++++++++++++++++
>  src/remote/remote_protocol.x    | 22 ++++++++++++++++-
>  src/remote_protocol-structs     |  8 ++++++
>  src/test/test_driver.c          | 17 +++++++++++++
>  tools/virsh-host.c              | 45 +++++++++++++++++++++++++++++++++
>  tools/virsh.pod                 |  5 ++++

This is a rather big patch.  When adding new API, it's best to do it in
pieces:

1. use of the API itself (include, tools, src/driver, src/libvirt*)
2. RPC support (daemon, src/remote)
3. qemu support (src/qemu)
4. test support (src/test)
5. python bindings (python)

as long as each part builds and passes 'make check'.

I did not review the patch itself, on the grounds that getting the
design right, and the patch split, will make review easier.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130821/f0654064/attachment-0001.sig>


More information about the libvir-list mailing list