On Tue, Apr 08, 2008 at 10:49:01AM -0700, Ryan Scott wrote:
> diff --git a/src/libvirt.c b/src/libvirt.c
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -48,6 +48,9 @@
> #ifdef WITH_LXC
> #include "lxc_driver.h"
> #endif
> +#ifdef WITH_LDOMS
> +extern int ldomsRegister(void);
> +#endif
>
> /*
> * TODO:
> @@ -267,6 +270,11 @@ virInitialize(void)
> * Note that the order is important: the first ones have a higher
> * priority when calling virConnectOpen.
> */
> +#ifdef WITH_LDOMS
> + if (ldomsRegister() == -1) return -1;
> + /* Don't want to run any other HV with LDoms */
> + return (0);
> +#endif
This seems rather bogus. We already have the ability to disable drivers
at compile time, and choose between them based on the URI passed to
the open call. Further restricting at registration time doesn't serve
any obvious purpose & breaks the test driver for example, which is useful
if testing apps using libvirt. And breaks the storage / networking drivers.
IMHO the 'return (0)' should be removed.
> +#ifdef WITH_LDOMS
> +/**
> + * virLDomConsole:
> + * @domain: the domain if available
> + *
> + * Opens a terminal window to the console for a domain
> + *
> + * Returns -1 in case of error, LDom console port number in case of success
> + */
> +int
> +virLDomConsole(virDomainPtr domain)
This has to die. Adding driver specific functions in the public API is
not acceptable. This information needs to go in the XML description.
> 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
As mentioned elsewhere on this thread, we need to define a formal description
for this in the XML, and then remove the WITH_LDOMS stuff and make this a
generic impl. Furthermore, poping up an xterm from virsh is non-sensical
as virsh is probably already running in an xterm, and the user can create a
new window themselves if they so desire.
> @@ -1019,17 +1045,26 @@ cmdStart(vshControl * ctl, vshCmd * cmd)
> virDomainPtr dom;
> int ret = TRUE;
>
> +#ifdef WITH_LDOMS
> + /* Need to send in the 'domain' option name instead of 'name' */
> + if (!(dom = vshCommandOptDomainBy(ctl, cmd, "domain", NULL, VSH_BYNAME)))
> + return FALSE;
> +#else
> if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
> return FALSE;
> +#endif
THis is not acceptable. Basically if there is any WITH_LDOMS code in
virsh, then it has failed. virsh is a client of libvirt and libvirt
is intended to provide a generic API.
>
> if (!(dom = vshCommandOptDomainBy(ctl, cmd, "name", NULL, VSH_BYNAME)))
> return FALSE;
>
> + /* Allow LDoms domain state to be inactive or bound */
> +#ifndef WITH_LDOMS
> if (virDomainGetID(dom) != (unsigned int)-1) {
> vshError(ctl, FALSE, "%s", _("Domain is already active"));
> virDomainFree(dom);
> return FALSE;
> }
> +#endif
>
> if (virDomainCreate(dom) == 0) {
> vshPrint(ctl, _("Domain %s started\n"),
> @@ -1660,11 +1695,13 @@ cmdVcpuinfo(vshControl * ctl, vshCmd * c
>
> vshPrint(ctl, "%-15s %.1lfs\n", _("CPU time:"), cpuUsed);
> }
> +#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
If LDoms don't have CPU affinity, then the mask should be filled such at it
is all 0, or all 1 as appropriate. Then this WITH_LDOMS can go.
> @@ -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 */
>
> {"net-autostart", cmdNetworkAutostart, opts_network_autostart, info_network_autostart},
> {"net-create", cmdNetworkCreate, opts_network_create, info_network_create},
> @@ -5144,20 +5187,28 @@ static vshCmdDef commands[] = {
> {"pool-uuid", cmdPoolUuid, opts_pool_uuid, info_pool_uuid},
>
> {"quit", cmdQuit, NULL, info_quit},
> +#ifndef WITH_LDOMS
> {"reboot", cmdReboot, opts_reboot, info_reboot},
> {"restore", cmdRestore, opts_restore, info_restore},
> {"resume", cmdResume, opts_resume, info_resume},
> {"save", cmdSave, opts_save, info_save},
> {"schedinfo", cmdSchedinfo, opts_schedinfo, info_schedinfo},
> {"dump", cmdDump, opts_dump, info_dump},
> +#endif /* WITH_LDOMS */
> {"shutdown", cmdShutdown, opts_shutdown, info_shutdown},
> {"setmem", cmdSetmem, opts_setmem, info_setmem},
> +#ifndef WITH_LDOMS
> {"setmaxmem", cmdSetmaxmem, opts_setmaxmem, info_setmaxmem},
> +#endif /* WITH_LDOMS */
> {"setvcpus", cmdSetvcpus, opts_setvcpus, info_setvcpus},
> +#ifndef WITH_LDOMS
> {"suspend", cmdSuspend, opts_suspend, info_suspend},
> {"ttyconsole", cmdTTYConsole, opts_ttyconsole, info_ttyconsole},
> +#endif /* WITH_LDOMS */
> {"undefine", cmdUndefine, opts_undefine, info_undefine},
> +#ifndef WITH_LDOMS
> {"uri", cmdURI, NULL, info_uri},
> +#endif /* WITH_LDOMS */
Again, none of this is acceptable - LDoms should simply not implement the APIs
in the driver, and thus virsh will print out an appropriate message such as
"operation is not supported on this hypervisor"
> @@ -5905,6 +5960,10 @@ vshDomainVcpuStateToString(int state)
> return gettext_noop("blocked");
> case VIR_VCPU_RUNNING:
> return gettext_noop("running");
> +#ifdef WITH_LDOMS
> + case VIR_VCPU_UNKNOWN:
> + return gettext_noop("unknown");
> +#endif
What is this 'UNKNOWN' CPU state ?
> 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 WITH_LDOMS is redundant - just unconditionally include the
enum field.
> @@ -253,6 +256,11 @@ typedef virDomainPtr
> const char *uri,
> unsigned long flags);
>
> +#ifdef WITH_LDOMS
> +typedef int
> + (*virDrvLDomConsole) (virDomainPtr domain);
> +#endif
> +
> typedef struct _virDriver virDriver;
> typedef virDriver *virDriverPtr;
>
> @@ -337,6 +345,9 @@ struct _virDriver {
> virDrvDomainInterfaceStats domainInterfaceStats;
> virDrvNodeGetCellsFreeMemory nodeGetCellsFreeMemory;
> virDrvNodeGetFreeMemory getFreeMemory;
> +#ifdef WITH_LDOMS
> + virDrvLDomConsole ldomConsole;
> +#endif
These have to go, as discussed above
> typedef int
> 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
If we need this, then it'll have to be added to the API unconditionally.
> +/* Don't really know what to return for this */
> +static const char *
> +ldomsGetType(virConnectPtr conn)
> +{
> + if (ldoms_debug) dprt("LDOMS_DEBUG: ldomsGetType(ENTER)\n");
Since you wrote this, we added a generic VIR_DEBUG call which you can use
instead of the LDOMS specific dprt. If we need to make VIR_DEBUG more
flexible then we can do that too.
> +virDomainPtr
> +ldomsDomainCreateXML(virConnectPtr conn, const char *xml, unsigned int flags)
> +{
> + virDomainPtr domPtr = NULL;
> + char *domainName = NULL;
> +
> + if (ldoms_debug) dprt("LDOMS_DEBUG: ldomsDomainCreateXML(ENTER) \n");
> + if (ldoms_detailed_debug) dprt("LDOMS_DETAILED_DEBUG: ldomsDomainCreateXML(ENTER) xmldoc=\n%s\n",xml);
> +
> + /* Send the XML file along with the lifecycle action */
> + domainName = send_ldom_create_domain((char *)xml, XML_ADD_DOMAIN);
> +
> + /*
> + * If the create/bind domain was successful, then we have to create a DomainInfo
> + * structure to pass back to the caller. We need the Domain name that was
> + * created, and the only way to get that is from parsing the input xml
> + * document. This is done in send_ldom_create_domain() and passed back as the return.
> + * Also we create the uuid for the domain name.
> + */
> + if (domainName != NULL) {
> +
> + /*
> + * The UUID is not important, cuz only the Domain name is printed
> + * out in the virsh. So just use the default UUID.
> + */
This comment is bogus. UUID is a mandatory identifier that needs to be assciated
with all VMs. The uniqueness rules are:
- ID - unique amongst all running VMs on a host
- Name - unique amongst all running & inacive VMs on a host
- UUID - globally unique across hosts.
All are mandatory, with exception that inactive VMs have an ID of -1.
> + /* Dump the response to memory */
> + xml = malloc(sizeof(char) * 10000);
A check for NULL needed....
> + xmlKeepBlanksDefault(0);
> + xmlDocDumpFormatMemory(xml_received, &xml, &xmlSize, 1);
> + if ( xmlSize > 0 ) {
> + xmlFree(xml_received);
> + if (ldoms_detailed_debug) dprt("LDOMS_DETAILED_DEBUG: ldomsDomainDumpXML() xml doc size is %d:\n%s\n",xmlSize,xml);
> + return ((char *)xml);
> + }
> +
> + return (NULL);
> +
> +} /* ldomsDomainDumpXML() */
Regards,
Dan.
--
|: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list