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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list