[libvirt] [PATCH 1/1] Support for rdp/desktop types in graphics tag
Hi All, As per the discussion earlier on the list I have modified the rdp type and added a new desktop type and posting the patch for same. Regards, Pritesh
On Thu, Apr 30, 2009 at 04:15:27PM +0200, Pritesh Kothari wrote:
Hi All,
As per the discussion earlier on the list I have modified the rdp type and added a new desktop type and posting the patch for same.
ACK, this looks good to me. Daniel
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 2f784e1..5e4aa5b 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -711,6 +711,63 @@ </attribute> </optional> </group> + <group> + <attribute name="type"> + <value>rdp</value> + </attribute> + <optional> + <attribute name="port"> + <ref name="PortNumber"/> + </attribute> + </optional> + <optional> + <attribute name="autoport"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="replaceUser"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="multiUser"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="listen"> + <ref name="addrIP"/> + </attribute> + </optional> + </group> + <group> + <attribute name="type"> + <value>desktop</value> + </attribute> + <optional> + <attribute name="display"> + <text/> + </attribute> + </optional> + <optional> + <attribute name="fullscreen"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> + </group> </choice> </element> </define> diff --git a/src/domain_conf.c b/src/domain_conf.c index ed4b8e5..4030fa0 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -150,7 +150,9 @@ VIR_ENUM_IMPL(virDomainInputBus, VIR_DOMAIN_INPUT_BUS_LAST,
VIR_ENUM_IMPL(virDomainGraphics, VIR_DOMAIN_GRAPHICS_TYPE_LAST, "sdl", - "vnc") + "vnc", + "rdp", + "desktop")
VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST, "subsystem", @@ -244,6 +246,14 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) VIR_FREE(def->data.sdl.display); VIR_FREE(def->data.sdl.xauth); break; + + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + VIR_FREE(def->data.rdp.listenAddr); + break; + + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: + VIR_FREE(def->data.desktop.display); + break; }
VIR_FREE(def); @@ -1501,6 +1511,68 @@ virDomainGraphicsDefParseXML(virConnectPtr conn, def->data.sdl.fullscreen = 0; def->data.sdl.xauth = virXMLPropString(node, "xauth"); def->data.sdl.display = virXMLPropString(node, "display"); + } else if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_RDP) { + char *port = virXMLPropString(node, "port"); + char *autoport; + char *replaceUser; + char *multiUser; + + if (port) { + if (virStrToLong_i(port, NULL, 10, &def->data.rdp.port) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot parse rdp port %s"), port); + VIR_FREE(port); + goto error; + } + VIR_FREE(port); + } else { + def->data.rdp.port = 0; + def->data.rdp.autoport = 1; + } + + if ((autoport = virXMLPropString(node, "autoport")) != NULL) { + if (STREQ(autoport, "yes")) { + if (flags & VIR_DOMAIN_XML_INACTIVE) + def->data.rdp.port = 0; + def->data.rdp.autoport = 1; + } + VIR_FREE(autoport); + } + + if ((replaceUser = virXMLPropString(node, "replaceUser")) != NULL) { + if (STREQ(replaceUser, "yes")) { + def->data.rdp.replaceUser = 1; + } + VIR_FREE(replaceUser); + } + + if ((multiUser = virXMLPropString(node, "multiUser")) != NULL) { + if (STREQ(multiUser, "yes")) { + def->data.rdp.multiUser = 1; + } + VIR_FREE(multiUser); + } + + def->data.rdp.listenAddr = virXMLPropString(node, "listen"); + } else if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP) { + char *fullscreen = virXMLPropString(node, "fullscreen"); + + if (fullscreen != NULL) { + if (STREQ(fullscreen, "yes")) { + def->data.desktop.fullscreen = 1; + } else if (STREQ(fullscreen, "no")) { + def->data.desktop.fullscreen = 0; + } else { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown fullscreen value '%s'"), fullscreen); + VIR_FREE(fullscreen); + goto error; + } + VIR_FREE(fullscreen); + } else + def->data.desktop.fullscreen = 0; + + def->data.desktop.display = virXMLPropString(node, "display"); }
cleanup: @@ -3269,6 +3341,38 @@ virDomainGraphicsDefFormat(virConnectPtr conn, virBufferAddLit(buf, " fullscreen='yes'");
break; + + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + if (def->data.rdp.port) + virBufferVSprintf(buf, " port='%d'", + def->data.rdp.port); + else if (def->data.rdp.autoport) + virBufferAddLit(buf, " port='0'"); + + if (def->data.rdp.autoport) + virBufferVSprintf(buf, " autoport='yes'"); + + if (def->data.rdp.replaceUser) + virBufferVSprintf(buf, " replaceUser='yes'"); + + if (def->data.rdp.multiUser) + virBufferVSprintf(buf, " multiUser='yes'"); + + if (def->data.rdp.listenAddr) + virBufferVSprintf(buf, " listen='%s'", def->data.rdp.listenAddr); + + break; + + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: + if (def->data.desktop.display) + virBufferEscapeString(buf, " display='%s'", + def->data.desktop.display); + + if (def->data.desktop.fullscreen) + virBufferAddLit(buf, " fullscreen='yes'"); + + break; + }
virBufferAddLit(buf, "/>\n"); diff --git a/src/domain_conf.h b/src/domain_conf.h index 84fc477..1d12fea 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -264,6 +264,8 @@ struct _virDomainSoundDef { enum virDomainGraphicsType { VIR_DOMAIN_GRAPHICS_TYPE_SDL, VIR_DOMAIN_GRAPHICS_TYPE_VNC, + VIR_DOMAIN_GRAPHICS_TYPE_RDP, + VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP,
VIR_DOMAIN_GRAPHICS_TYPE_LAST, }; @@ -285,6 +287,17 @@ struct _virDomainGraphicsDef { char *xauth; int fullscreen; } sdl; + struct { + int port; + char *listenAddr; + int autoport : 1; + int replaceUser : 1; + int multiUser : 1; + } rdp; + struct { + char *display; + int fullscreen : 1; + } desktop; } data; };
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- |: Red Hat, Engineering, London -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 :|
Hi All, I have added support for vrdp/sdl/gui modes for VirtualBox driver in libvirt. Tha patch's are as below: [PATCH 1/3]: contains support for vrdp/sdl/gui while defining a machine. [PATCH 2/3]: contains support for vrdp/sdl/gui while dumping xml [PATCH 3/3]: contains support for vrdp/sdl/gui while starting the machine Regards, Pritesh
On Thu, May 07, 2009 at 03:49:28PM +0200, Pritesh Kothari wrote:
Hi All,
I have added support for vrdp/sdl/gui modes for VirtualBox driver in libvirt. Tha patch's are as below:
[PATCH 1/3]: contains support for vrdp/sdl/gui while defining a machine. [PATCH 2/3]: contains support for vrdp/sdl/gui while dumping xml [PATCH 3/3]: contains support for vrdp/sdl/gui while starting the machine
Regards, Pritesh
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index b25e93b..87db6ab 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3103,9 +3093,86 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml) { VRDPServer->vtbl->nsisupports.Release((nsISupports *)VRDPServer); } } + + if ((def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP) && (guiPresent == 0)) { + guiPresent = 1; + guiDisplay = strdup(def->graphics[i]->data.desktop.display); + } + + if ((def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) && (sdlPresent == 0)) { + sdlPresent = 1; + sdlDisplay = strdup(def->graphics[i]->data.sdl.display); + } + }
Need to check for OOM failure here.
+ + if ((vrdpPresent == 1) && (guiPresent == 0) && (sdlPresent == 0)) { + /* store extradata key that frontend is set to vrdp */ + PRUnichar *keyTypeUtf16 = NULL; + PRUnichar *valueTypeUtf16 = NULL; + + data->pFuncs->pfnUtf8ToUtf16("FRONTEND/Type", &keyTypeUtf16); + data->pFuncs->pfnUtf8ToUtf16("vrdp", &valueTypeUtf16); + + machine->vtbl->SetExtraData(machine, keyTypeUtf16, valueTypeUtf16); + + data->pFuncs->pfnUtf16Free(keyTypeUtf16); + data->pFuncs->pfnUtf16Free(valueTypeUtf16); + + } else if ((guiPresent == 0) && (sdlPresent == 1)) { + /* store extradata key that frontend is set to sdl */ + PRUnichar *keyTypeUtf16 = NULL; + PRUnichar *valueTypeUtf16 = NULL; + PRUnichar *keyDislpayUtf16 = NULL; + PRUnichar *valueDisplayUtf16 = NULL; + + data->pFuncs->pfnUtf8ToUtf16("FRONTEND/Type", &keyTypeUtf16); + data->pFuncs->pfnUtf8ToUtf16("sdl", &valueTypeUtf16); + + machine->vtbl->SetExtraData(machine, keyTypeUtf16, valueTypeUtf16); + + data->pFuncs->pfnUtf16Free(keyTypeUtf16); + data->pFuncs->pfnUtf16Free(valueTypeUtf16); + + if (sdlDisplay) { + data->pFuncs->pfnUtf8ToUtf16("FRONTEND/Display", &keyDislpayUtf16); + data->pFuncs->pfnUtf8ToUtf16(sdlDisplay, &valueDisplayUtf16); + + machine->vtbl->SetExtraData(machine, keyDislpayUtf16, valueDisplayUtf16); + + data->pFuncs->pfnUtf16Free(keyDislpayUtf16); + data->pFuncs->pfnUtf16Free(valueDisplayUtf16); + } + + } else { + /* if all are set then default is gui, with vrdp turned on */ + PRUnichar *keyTypeUtf16 = NULL; + PRUnichar *valueTypeUtf16 = NULL; + PRUnichar *keyDislpayUtf16 = NULL; + PRUnichar *valueDisplayUtf16 = NULL; + + data->pFuncs->pfnUtf8ToUtf16("FRONTEND/Type", &keyTypeUtf16); + data->pFuncs->pfnUtf8ToUtf16("gui", &valueTypeUtf16); + + machine->vtbl->SetExtraData(machine, keyTypeUtf16, valueTypeUtf16); + + data->pFuncs->pfnUtf16Free(keyTypeUtf16); + data->pFuncs->pfnUtf16Free(valueTypeUtf16); + + if (guiDisplay) { + data->pFuncs->pfnUtf8ToUtf16("FRONTEND/Display", &keyDislpayUtf16); + data->pFuncs->pfnUtf8ToUtf16(guiDisplay, &valueDisplayUtf16); + + machine->vtbl->SetExtraData(machine, keyDislpayUtf16, valueDisplayUtf16); + + data->pFuncs->pfnUtf16Free(keyDislpayUtf16); + data->pFuncs->pfnUtf16Free(valueDisplayUtf16); + } } + + VIR_FREE(guiDisplay); + VIR_FREE(sdlDisplay); + } /* Finished:Block to attach the Remote Display to VM */ -#endif
{ /* Started:Block to attach USB Devices to VM */ if (def->nhostdevs > 0) {
Generally, looks fine apart from the OOM check Daniel -- |: Red Hat, Engineering, London -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 :|
[PATCH 1/3]: contains support for vrdp/sdl/gui while defining a machine.
+ guiDisplay = strdup(def->graphics[i]->data.desktop.display); + } + + if ((def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) && (sdlPresent == 0)) { + sdlPresent = 1; + sdlDisplay = strdup(def->graphics[i]->data.sdl.display); + } + }
Need to check for OOM failure here.
Done and reposting the patch Regards, Pritesh
On Mon, May 11, 2009 at 03:56:35PM +0200, Pritesh Kothari wrote:
[PATCH 1/3]: contains support for vrdp/sdl/gui while defining a machine.
+ + if ((def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) && (sdlPresent == 0)) { + sdlPresent = 1; + sdlDisplay = strdup(def->graphics[i]->data.sdl.display); + if (sdlDisplay == NULL) { + vboxError(conn, VIR_ERR_SYSTEM_ERROR, "%s", "strdup failed"); + /* just don't go to cleanup yet as it is ok to have + * sdlDisplay as NULL and we check it below if it + * exist and then only use it there + */ + } + }
This wasn't exactly what I anticipated. It intended that it should report the error *and* return as an error to the caller, rather than carrying on. That said, I notice that none of the existing code we have committed for VirtualBox driver deals with OOM on strdup() calls, so this patch is not making things any worse. Thus I think we can commit these patches and fix all the strdup calls in one go, later. Daniel -- |: Red Hat, Engineering, London -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 :|
This wasn't exactly what I anticipated. It intended that it should report the error *and* return as an error to the caller, rather than carrying on.
hmm.. I thought it more like handling NULL returns from strdup.
That said, I notice that none of the existing code we have committed for VirtualBox driver deals with OOM on strdup() calls, so this patch is not making things any worse. Thus I think we can commit these patches and fix all the strdup calls in one go, later.
oops, Sorry this seems to be pretty big slip, yes will try to fix all of them but help is definitely appreciated. Also if strdup() is going to be handle like alloc/VIR_ALLOC way, then i would wait for that and then do the changes. Regards, Pritesh.
Hi All, I have added support for vrdp/sdl/gui modes for VirtualBox driver in libvirt. Tha patch's are as below: [PATCH 1/3]: contains support for vrdp/sdl/gui while defining a machine. [PATCH 2/3]: contains support for vrdp/sdl/gui while dumping xml [PATCH 3/3]: contains support for vrdp/sdl/gui while starting the machine Regards, Pritesh
On Thu, May 07, 2009 at 03:49:38PM +0200, Pritesh Kothari wrote:
Hi All,
I have added support for vrdp/sdl/gui modes for VirtualBox driver in libvirt. Tha patch's are as below:
[PATCH 1/3]: contains support for vrdp/sdl/gui while defining a machine. [PATCH 2/3]: contains support for vrdp/sdl/gui while dumping xml [PATCH 3/3]: contains support for vrdp/sdl/gui while starting the machine
Regards, Pritesh
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 87db6ab..223009a 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c + + if (valueDisplayUtf16) { + data->pFuncs->pfnUtf16ToUtf8(valueDisplayUtf16, &valueDisplayUtf8); + data->pFuncs->pfnUtf16Free(valueDisplayUtf16); + + if (strlen(valueDisplayUtf8) <= 0) { + data->pFuncs->pfnUtf8Free(valueDisplayUtf8); + valueDisplayUtf8 = getenv("DISPLAY"); + valueDisplayFree = 0; + } } else { - def->graphics->data.rdp.autoport = 1; + valueDisplayUtf8 = getenv("DISPLAY"); + valueDisplayFree = 0; }
Using getenv() here is not right, since that's running in the context of the app using libvirt. If there's no display available via virtualbox's API, then just leave this attribute blank when generating the XML.
- VRDPServer->vtbl->GetNetAddress(VRDPServer, &netAddressUtf16); - if (netAddressUtf16) { - data->pFuncs->pfnUtf16ToUtf8(netAddressUtf16, &netAddressUtf8); - if (STRNEQ(netAddressUtf8, "")) - def->graphics->data.rdp.listenAddr = strdup(netAddressUtf8); - data->pFuncs->pfnUtf16Free(netAddressUtf16); - data->pFuncs->pfnUtf8Free(netAddressUtf8); + if (STREQ(valueTypeUtf8, "sdl")) { + sdlPresent = 1; + sdlDisplay = strdup(valueDisplayUtf8); + totalPresent++; }
- VRDPServer->vtbl->GetAuthType(VRDPServer, &authType); - if (authType == VRDPAuthType_External) { - PRUint32 authTimeout = 0; + if (STREQ(valueTypeUtf8, "gui")) { + guiPresent = 1; + guiDisplay = strdup(valueDisplayUtf8); + totalPresent++; + } + if (valueDisplayFree) + data->pFuncs->pfnUtf8Free(valueDisplayUtf8); + }
- VRDPServer->vtbl->GetAuthTimeout(VRDPServer, &authTimeout); + if (STREQ(valueTypeUtf8, "vrdp")) + vrdpPresent = 1;
- def->graphics->data.rdp.auth = strdup("external"); - def->graphics->data.rdp.authtimeout = authTimeout; - } else if (authType == VRDPAuthType_Guest) { - PRUint32 authTimeout = 0; + data->pFuncs->pfnUtf8Free(valueTypeUtf8); + }
- VRDPServer->vtbl->GetAuthTimeout(VRDPServer, &authTimeout); + if ((totalPresent > 0) && (VIR_ALLOC_N(def->graphics, totalPresent) >= 0)) { + if ((guiPresent) && (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0)) { + def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; + def->graphics[def->ngraphics]->data.desktop.display = guiDisplay; + def->ngraphics++; + }
- def->graphics->data.rdp.auth = strdup("guest"); - def->graphics->data.rdp.authtimeout = authTimeout; - } + if ((sdlPresent) && (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0)) { + def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_SDL; + def->graphics[def->ngraphics]->data.sdl.display = sdlDisplay; + def->ngraphics++; + } + } else if ((vrdpPresent != 1) && (totalPresent == 0) && (VIR_ALLOC_N(def->graphics, 1) >= 0)) { + if (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0) { + def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; + def->graphics[def->ngraphics]->data.desktop.display = strdup(getenv("DISPLAY")); + totalPresent++; + def->ngraphics++; + } + } + + machine->vtbl->GetVRDPServer(machine, &VRDPServer); + if (VRDPServer) { + VRDPServer->vtbl->GetEnabled(VRDPServer, &VRDPEnabled); + if (VRDPEnabled) {
+ totalPresent++;
- VRDPServer->vtbl->GetAllowMultiConnection(VRDPServer, &allowMultiConnection); - if (allowMultiConnection) { - def->graphics->data.rdp.multiconnections = 1; - } + if ((VIR_REALLOC_N(def->graphics, totalPresent) >= 0) && + (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0)) { + PRUint32 VRDPport = 0; + PRUnichar *netAddressUtf16 = NULL; + char *netAddressUtf8 = NULL; + PRBool allowMultiConnection = PR_FALSE; + PRBool reuseSingleConnection = PR_FALSE;
- VRDPServer->vtbl->GetReuseSingleConnection(VRDPServer, &reuseSingleConnection); - if (reuseSingleConnection) { - def->graphics->data.rdp.reuseconnection = 1; - } + def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP;
- machine->vtbl->GetSessionType(machine, &sessionTypeUtf16); - DEBUG0("Session Type:"); - if (sessionTypeUtf16) { - data->pFuncs->pfnUtf16ToUtf8(sessionTypeUtf16, &sessionTypeUtf8); - DEBUG("Session Type: %s", sessionTypeUtf8); - if (STREQ(sessionTypeUtf8, "vrdp")) { - def->graphics->data.rdp.headless = 1; + VRDPServer->vtbl->GetPort(VRDPServer, &VRDPport); + if (VRDPport) { + def->graphics[def->ngraphics]->data.rdp.port = VRDPport; + } else { + def->graphics[def->ngraphics]->data.rdp.autoport = 1; } - data->pFuncs->pfnUtf16Free(sessionTypeUtf16); - data->pFuncs->pfnUtf8Free(sessionTypeUtf8); + + VRDPServer->vtbl->GetNetAddress(VRDPServer, &netAddressUtf16); + if (netAddressUtf16) { + data->pFuncs->pfnUtf16ToUtf8(netAddressUtf16, &netAddressUtf8); + if (STRNEQ(netAddressUtf8, "")) + def->graphics[def->ngraphics]->data.rdp.listenAddr = strdup(netAddressUtf8); + data->pFuncs->pfnUtf16Free(netAddressUtf16); + data->pFuncs->pfnUtf8Free(netAddressUtf8); + } + + VRDPServer->vtbl->GetAllowMultiConnection(VRDPServer, &allowMultiConnection); + if (allowMultiConnection) { + def->graphics[def->ngraphics]->data.rdp.multiUser = 1; + } + + VRDPServer->vtbl->GetReuseSingleConnection(VRDPServer, &reuseSingleConnection); + if (reuseSingleConnection) { + def->graphics[def->ngraphics]->data.rdp.replaceUser = 1; + } + + def->ngraphics++; } } + VRDPServer->vtbl->nsisupports.Release((nsISupports *)VRDPServer); } - VRDPServer->vtbl->nsisupports.Release((nsISupports *)VRDPServer); } -#endif
There's generally quite a few uses of strdup() here without OOM checking Regards, Daniel -- |: Red Hat, Engineering, London -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 :|
- def->graphics->data.rdp.autoport = 1; + valueDisplayUtf8 = getenv("DISPLAY"); + valueDisplayFree = 0; }
Using getenv() here is not right, since that's running in the context of the app using libvirt. If there's no display available via virtualbox's API, then just leave this attribute blank when generating the XML.
There's generally quite a few uses of strdup() here without OOM checking
Fixed these two and reposting the patch. Regards, Pritesh
Hi All, I have added support for vrdp/sdl/gui modes for VirtualBox driver in libvirt. Tha patch's are as below: [PATCH 1/3]: contains support for vrdp/sdl/gui while defining a machine. [PATCH 2/3]: contains support for vrdp/sdl/gui while dumping xml [PATCH 3/3]: contains support for vrdp/sdl/gui while starting the machine Regards, Pritesh
On Thu, May 07, 2009 at 03:50:13PM +0200, Pritesh Kothari wrote:
Hi All,
I have added support for vrdp/sdl/gui modes for VirtualBox driver in libvirt. Tha patch's are as below:
[PATCH 1/3]: contains support for vrdp/sdl/gui while defining a machine. [PATCH 2/3]: contains support for vrdp/sdl/gui while dumping xml [PATCH 3/3]: contains support for vrdp/sdl/gui while starting the machine
Regards, Pritesh
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 223009a..97a686f 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2322,20 +2322,12 @@ static int vboxDomainCreate(virDomainPtr dom) { IProgress *progress = NULL; PRUint32 machineCnt = 0; PRUnichar *env = NULL; - const char *display = getenv("DISPLAY"); PRUnichar *sessionType = NULL; char displayutf8[32] = {0}; unsigned char iidl[VIR_UUID_BUFLEN] = {0}; int i, ret = -1;
- if (display) { - sprintf(displayutf8, "DISPLAY=%s", display); - data->pFuncs->pfnUtf8ToUtf16(displayutf8, &env); - } - - data->pFuncs->pfnUtf8ToUtf16("gui", &sessionType); - if (!dom->name) { vboxError(dom->conn, VIR_ERR_INTERNAL_ERROR,"%s", "Error while reading the domain name"); @@ -2373,6 +2365,92 @@ static int vboxDomainCreate(virDomainPtr dom) { if ( (state == MachineState_PoweredOff) || (state == MachineState_Saved) || (state == MachineState_Aborted) ) { + int vrdpPresent = 0; + int sdlPresent = 0; + int guiPresent = 0; + char *guiDisplay = NULL; + char *sdlDisplay = NULL; + PRUnichar *keyTypeUtf16 = NULL; + PRUnichar *valueTypeUtf16 = NULL; + char *valueTypeUtf8 = NULL; + PRUnichar *keyDislpayUtf16 = NULL; + PRUnichar *valueDisplayUtf16 = NULL; + char *valueDisplayUtf8 = NULL; + int valueDisplayFree = 1; + + data->pFuncs->pfnUtf8ToUtf16("FRONTEND/Type", &keyTypeUtf16); + machine->vtbl->GetExtraData(machine, keyTypeUtf16, &valueTypeUtf16); + data->pFuncs->pfnUtf16Free(keyTypeUtf16); + + if (valueTypeUtf16) { + data->pFuncs->pfnUtf16ToUtf8(valueTypeUtf16, &valueTypeUtf8); + data->pFuncs->pfnUtf16Free(valueTypeUtf16); + + if ( STREQ(valueTypeUtf8, "sdl") || STREQ(valueTypeUtf8, "gui") ) { + + data->pFuncs->pfnUtf8ToUtf16("FRONTEND/Display", &keyDislpayUtf16); + machine->vtbl->GetExtraData(machine, keyDislpayUtf16, &valueDisplayUtf16); + data->pFuncs->pfnUtf16Free(keyDislpayUtf16); + + if (valueDisplayUtf16) { + data->pFuncs->pfnUtf16ToUtf8(valueDisplayUtf16, &valueDisplayUtf8); + data->pFuncs->pfnUtf16Free(valueDisplayUtf16); + + if (strlen(valueDisplayUtf8) <= 0) { + data->pFuncs->pfnUtf8Free(valueDisplayUtf8); + valueDisplayUtf8 = getenv("DISPLAY"); + valueDisplayFree = 0; + } + } else { + valueDisplayUtf8 = getenv("DISPLAY"); + valueDisplayFree = 0; + } + + if (STREQ(valueTypeUtf8, "sdl")) { + sdlPresent = 1; + sdlDisplay = strdup(valueDisplayUtf8); + } + + if (STREQ(valueTypeUtf8, "gui")) { + guiPresent = 1; + guiDisplay = strdup(valueDisplayUtf8); + } + } + + if (STREQ(valueTypeUtf8, "vrdp")) { + vrdpPresent = 1; + } + + data->pFuncs->pfnUtf8Free(valueTypeUtf8); + + } else { + guiPresent = 1; + guiDisplay = strdup(getenv("DISPLAY")); + } + if (valueDisplayFree) + data->pFuncs->pfnUtf8Free(valueDisplayUtf8); + + if (guiPresent) { + sprintf(displayutf8, "DISPLAY=%.24s", guiDisplay); + data->pFuncs->pfnUtf8ToUtf16(displayutf8, &env); + + data->pFuncs->pfnUtf8ToUtf16("gui", &sessionType); + + VIR_FREE(guiDisplay); + } + + if (sdlPresent) { + sprintf(displayutf8, "DISPLAY=%.24s", sdlDisplay); + data->pFuncs->pfnUtf8ToUtf16(displayutf8, &env); + + data->pFuncs->pfnUtf8ToUtf16("sdl", &sessionType); + + VIR_FREE(sdlDisplay); + } + + if (vrdpPresent) { + data->pFuncs->pfnUtf8ToUtf16("vrdp", &sessionType); + }
Same issue about use of 'getenv' & strdup OOM handling here. Regards, Daniel -- |: Red Hat, Engineering, London -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 :|
On Mon, May 11, 2009 at 03:56:50PM +0200, Pritesh Kothari wrote:
Same issue about use of 'getenv' & strdup OOM handling here.
Fixed these two and reposting the patch.
Okay, I have applied the 3 patches, we still have the strdup() checks missing but it's easier now just as a diff on top of CVS head, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Apr 30, 2009 at 04:15:27PM +0200, Pritesh Kothari wrote:
Hi All,
As per the discussion earlier on the list I have modified the rdp type and added a new desktop type and posting the patch for same.
Okay, this looks fine, but we also need a description of the format extensions in docs/formatdomain.html.in in the section elementsGraphics Applied and commited, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Pritesh Kothari