[Libvir] [PATCH][RFC] libvirt ldoms support
Eunice Moon
Eunice.Moon at Sun.COM
Wed Apr 9 19:38:34 UTC 2008
Hi Richard,
Thanks for your feedback. Please see my responses inline below.
Eunice
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
>
> 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.
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
> call?
>
> 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];
>> +#endif
>> +
>>
>> 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;
>> +#endif
>> +
>> 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")},
>> +#else
>> {"help", gettext_noop("start a (previously defined) inactive domain")},
>> +#endif
>> {"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")},
>> +#else
>> {"name", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("name of the inactive domain")},
>> +#endif
>> {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");
>> +#endif
>
> Instead of changes like this, just return an empty affinity map (if
> LDoms doesn't support this??).
>
OK.
>> @@ -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
>> +#endif
>> } 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 */
>> +#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.
>
OK.
>> 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.
>
OK.
> 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.
>
> Rich.
>
Thanks!
More information about the libvir-list
mailing list