[Libvir] [PATCH][RFC] libvirt ldoms support

Wed Apr 9 12:15:58 UTC 2008

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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/