Hi Daniel, Please see my responses inline below. I've skipped some responses which are already given in another threads. Thanks for your feedback. Eunice Daniel P. Berrange wrote:
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); +#endifThis 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.
OK. I will remove the return (0) statement here.
+#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; +#endifAs 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.
OK. I will change the code to not open an xterm. I thought it would be more useful to open up another xterm so that the user can continue to run the virsh cmds, but it looks like the user probably doesn't want to have an xterm window popping up each time for the console connection.
@@ -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; +#endifTHis 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; } +#endifif (virDomainCreate(dom) == 0) {vshPrint(ctl, _("Domain %s started\n"), @@ -1660,11 +1695,13 @@ cmdVcpuinfo(vshControl * ctl, vshCmd * cvshPrint(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"); +#endifIf 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"); +#endifWhat is this 'UNKNOWN' CPU state ?
We use the processor_info() to get the CPU state and this only works for the CPUs running on the primary domain. For the CPUs on the guest domains, we just map the value to the "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; +#endifThese have to go, as discussed abovetypedef 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 */ +#endifIf 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 useinstead 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.
OK. I will update the comments here.
+ /* Dump the response to memory */ + xml = malloc(sizeof(char) * 10000);A check for NULL needed....
OK. Thanks.
+ 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.
-- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list