[Libvir] [PATCH][RFC] libvirt ldoms support
Eunice Moon
Eunice.Moon at Sun.COM
Thu Apr 10 20:10:04 UTC 2008
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);
>> +#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.
>
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;
>> +#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.
>
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;
>> +#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 ?
>
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;
>> +#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.
>
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.
More information about the libvir-list
mailing list