[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

Hi Richard,

Thanks for your feedback.  Please see my responses inline below.


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


However I have some concerns about some of the modified "core code"
files in libvirt, ie:


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);
+#ifdef WITH_LDOMS
+    memcpy(uuid, &domain->uuid[0], VIR_UUID_BUFLEN);
     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.

Yes, that #ifdef was added to have the valid UUID (not UUID 0000-0000-
0000-0000) for the primary domain. I will remove the #ifdef here.

@@ -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
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.

For example it might look like:

    <graphics type='vnc' port='5904'/>

(see full example at http://libvirt.org/format.html).  So can you add
a similar device to the LDom domain XML to avoid the need for this new

From reading the virsh code it looks like virLDomConsole is meant to
return the telnet port number for the serial console(?) so perhaps
something like:

  <console port='1234'/>

For LDoms, we retrieve the console info from the XML file which has
different format (looking for the <port> element within <console>).
The #ifdef WITH_LDOMS block was added in cmdConsole() in virsh.c
to deal with the LDoms specific console code. But, if it is not
allowed to add any hypervisor specific code in the libvirt API
or virsh.c, I will try to figure out a way to avoid the virLDomConsole.

diff --git a/src/virsh.c b/src/virsh.c
--- a/src/virsh.c
+++ b/src/virsh.c
@@ -494,6 +494,11 @@ cmdConsole(vshControl * ctl, vshCmd * cm
     virDomainPtr dom;
     int ret = FALSE;
     char *doc;
+#ifdef WITH_LDOMS
+    int port;
+    char command[80];
if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
         return FALSE;
@@ -501,6 +506,19 @@ cmdConsole(vshControl * ctl, vshCmd * cm
     if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", NULL)))
         return FALSE;
+#ifdef WITH_LDOMS
+    port = virLDomConsole(dom);
+    if (port > 0) {
+        sprintf(command, "%s %d &",
+            "/usr/X/bin/xterm -sb -sl 1000 -e telnet localhost ", port);
+        system(command);
+        return TRUE;
+    }
+    vshError(ctl, FALSE, _("Failed to start console"));
+    return FALSE;
     doc = virDomainGetXMLDesc(dom, 0);
     if (!doc)
         goto cleanup;

See discussion above.

@@ -1003,13 +1021,21 @@ cmdUndefine(vshControl * ctl, vshCmd * c
 static vshCmdInfo info_start[] = {
     {"syntax", "start <domain>"},
+#ifdef WITH_LDOMS
+        {"help", gettext_noop("start an inactive or bound domain")},
     {"help", gettext_noop("start a (previously defined) inactive domain")},
     {"desc", gettext_noop("Start a domain.")},
     {NULL, NULL}
static vshCmdOptDef opts_start[] = {
+#ifdef WITH_LDOMS
+        {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("name of the inactive or bound domain")},
     {"name", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("name of the inactive domain")},
     {NULL, 0, 0, NULL}

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?

OK. I can remove most of the #ifdef WITH_LDOMS added in virsh.c
such as commenting out unsupported commands or printing different start/
help messages.  But, I still need to figure out how to handle the
LDoms specific console without adding #ifdef WITH_LDOMS in virsh.c.

+#ifndef WITH_LDOMS
             vshPrint(ctl, "%-15s ", _("CPU Affinity:"));
             for (m = 0 ; m < VIR_NODEINFO_MAXCPUS(nodeinfo) ; m++) {
                 vshPrint(ctl, "%c", VIR_CPU_USABLE(cpumap, cpumaplen, n, m) ? 'y' : '-');
             vshPrint(ctl, "\n");

Instead of changes like this, just return an empty affinity map (if
LDoms doesn't support this??).


@@ -5087,19 +5124,23 @@ cmdQuit(vshControl * ctl, vshCmd * cmd A
 static vshCmdDef commands[] = {
     {"help", cmdHelp, opts_help, info_help},
+#ifndef WITH_LDOMS
     {"attach-device", cmdAttachDevice, opts_attach_device, info_attach_device},
     {"attach-disk", cmdAttachDisk, opts_attach_disk, info_attach_disk},
     {"attach-interface", cmdAttachInterface, opts_attach_interface, info_attach_interface},
     {"autostart", cmdAutostart, opts_autostart, info_autostart},
     {"capabilities", cmdCapabilities, NULL, info_capabilities},
     {"connect", cmdConnect, opts_connect, info_connect},
+#endif /* WITH_LDOMS */
     {"console", cmdConsole, opts_console, info_console},
     {"create", cmdCreate, opts_create, info_create},
     {"start", cmdStart, opts_start, info_start},
     {"destroy", cmdDestroy, opts_destroy, info_destroy},
+#ifndef WITH_LDOMS
     {"detach-device", cmdDetachDevice, opts_detach_device, info_detach_device},
     {"detach-disk", cmdDetachDisk, opts_detach_disk, info_detach_disk},
     {"detach-interface", cmdDetachInterface, opts_detach_interface, info_detach_interface},
+#endif /* WITH_LDOMS */
     {"define", cmdDefine, opts_define, info_define},
     {"domid", cmdDomid, opts_domid, info_domid},
     {"domuuid", cmdDomuuid, opts_domuuid, info_domuuid},
@@ -5112,7 +5153,9 @@ static vshCmdDef commands[] = {
     {"freecell", cmdFreecell, opts_freecell, info_freecell},
     {"hostname", cmdHostname, NULL, info_hostname},
     {"list", cmdList, opts_list, info_list},
+#ifndef WITH_LDOMS
     {"migrate", cmdMigrate, opts_migrate, info_migrate},
+#endif /* WITH_LDOMS */

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.

OK.  I will remove all these #ifdef WITH_LDOMS here.

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/src/virterror.c b/src/virterror.c
--- a/src/virterror.c
+++ b/src/virterror.c

The changes to virterror are all fine.

diff --git a/src/driver.h b/src/driver.h
--- a/src/driver.h
+++ b/src/driver.h
@@ -24,7 +24,10 @@ typedef enum {
     VIR_DRV_QEMU = 3,
     VIR_DRV_REMOTE = 4,
     VIR_DRV_OPENVZ = 5,
-    VIR_DRV_LXC = 6
+    VIR_DRV_LXC = 6,
+#ifdef WITH_LDOMS
+    VIR_DRV_LDOMS = 7
 } virDrvNo;

This change doesn't need the #ifdef around it.  We can add new driver
types whenever even if not all platforms will support them.

OK.  I will remove the #ifdef WITH_LDOMS here.

Rest of the changes to driver.h are fine however.

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 */
 } 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.


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 */
 } 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 */
 } virErrorNumber;

Again, no need for #ifdefs here, otherwise this is fine.


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.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]