7:15 a.m.
On Wed, Apr 09, 2008 at 12:52:33PM +0100, Richard W.M. Jones wrote:
On Tue, Apr 08, 2008 at 10:49:01AM -0700, Ryan Scott wrote:
[...]
Hi Ryan,
Thanks for adding LDom support - obviously an important contribution
for libvirt and when I get my Solaris box working again I'll be able
to try it out.
For the new files forming the LDom driver, it all looks well-contained
and I just need to check that it doesn't prevent compilation on other
platforms.
> src/ldoms_common.h
> src/ldoms_internal.h
> src/ldoms_internal.c
> src/ldoms_intfc.h
> src/ldoms_intfc.c
> src/ldoms_xml_parse.h
> src/ldoms_xml_parse.c
However I have some concerns about some of the modified "core code"
files in libvirt, ie:
> src/libvirt.c
> src/virsh.c
> src/virterror.c
> src/driver.h
> include/libvirt/libvirt.h.in
I'm in complete agreement with Richard so far,
I will post my own review separately.
which I'll discuss inline below.
> @@ -1794,11 +1802,17 @@ virDomainGetUUID(virDomainPtr domain, un
> return (-1);
> }
>
> +#ifndef WITH_LDOMS
> if (domain->id == 0) {
> memset(uuid, 0, VIR_UUID_BUFLEN);
> } else {
> memcpy(uuid, &domain->uuid[0], VIR_UUID_BUFLEN);
> }
> +#endif
> +
> +#ifdef WITH_LDOMS
> + memcpy(uuid, &domain->uuid[0], VIR_UUID_BUFLEN);
> +#endif
> return (0);
> }
>
I guess this is working around the Xen assumption that dom0 has UUID
0000-0000-0000-0000, so it exposes a Xen-ism in the code. This should
move down to the Xen driver, so I'll propose a patch to fix that,
which should remove the need for the specific #ifdef here.
agreed
> @@ -5025,6 +5039,42 @@ virStorageVolGetPath(virStorageVolPtr vo
> return NULL;
> }
>
> +#ifdef WITH_LDOMS
> +/**
> + * virLDomConsole:
> + * @domain: the domain if available
[...]
I think having a generic "get console" call in libvirt might be a good
idea, but having public LDom-specific calls isn't. Dan Berrange will
We cannot add APIs specific to one hypervisor, virLDomConsole can't be added
to libvirt API.
correct me if I'm wrong here, but currently the method used is to
dump
the domain XML of a domain and look for the <console/> or <graphics/>
element within <devices/>, corresponding to the serial console or the
graphical (VNC) console respectively.
yes that's what virsh 'console' code uses, see cmdConsole() in virsh.c
The problem with changes lie the above is that they fractionalize
virsh into LDom and non-LDom variants. Can we come up with a middle
ground help text and avoid changing the option name?
In general libvirt code should never rely on WITH_LDOMS conditional
compilation except for:
- the registration of the ldom driver
virInitialize() in src/libvirt.c
- in the ldom specific files
- potentially in some of the storage or xml back-end for a bit of
specific processing
but really it should never affect virsh.c, or the API files.
[...]
You shouldn't need to comment out unsupported commands. They will
return an error if they aren't supported. In fact, QEMU, KVM and
OpenVZ only support a subset of the available operations.
and that's contrilled at the driver API level by having NULL entry points.
However if you want to propose a more general patch which allows
virsh
to determine which operations are supported on the current connection,
then I'm all for it. Some of the infrastructure is in place to do
this already.
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -549,6 +549,9 @@ typedef enum {
> VIR_VCPU_OFFLINE = 0, /* the virtual CPU is offline */
> VIR_VCPU_RUNNING = 1, /* the virtual CPU is running */
> VIR_VCPU_BLOCKED = 2, /* the virtual CPU is blocked on resource */
> +#ifdef WITH_LDOMS
> + VIR_VCPU_UNKNOWN = 3, /* the virtual CPU state is unknown */
> +#endif
> } virVcpuState;
I think this is fine (and the corresponding change to virsh.c to
support it). No need for the #ifdef since in theory other drivers
could have CPUs in this unknown state too.
Adding the new state is fine, the ifdef cannot be included in the header
> diff --git a/include/libvirt/virterror.h
b/include/libvirt/virterror.h
> --- a/include/libvirt/virterror.h
> +++ b/include/libvirt/virterror.h
> @@ -56,6 +56,9 @@ typedef enum {
> VIR_FROM_STATS_LINUX, /* Error in the Linux Stats code */
> VIR_FROM_LXC, /* Error from Linux Container driver */
> VIR_FROM_STORAGE, /* Error from storage driver */
> +#ifdef WITH_LDOMS
> + VIR_FROM_LDOMS, /* Error from LDoms driver */
> +#endif
> } virErrorDomain;
>
>
> @@ -139,6 +142,9 @@ typedef enum {
> VIR_WAR_NO_STORAGE, /* failed to start storage */
> VIR_ERR_NO_STORAGE_POOL, /* storage pool not found */
> VIR_ERR_NO_STORAGE_VOL, /* storage pool not found */
> +#ifdef WITH_LDOMS
> + VIR_ERR_INVALID_OPTION, /* invalid command line option */
> +#endif
> } virErrorNumber;
>
> /**
Again, no need for #ifdefs here, otherwise this is fine.
Agreed,
Changes to Makefile.am, configure.in, look fine.
And the rest of the patch contains the new LDom driver code, which is
fine because it's isolated to Solaris. These could go in straight
away as with the arrangement we have for OpenVZ code.
Generally agreed,
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/