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

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.