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.
> @@ -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'/>
> 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?
> +#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??).
> @@ -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.
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.
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.
> 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.
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.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list