[libvirt] [PATCH 00/13] vbox: more error checking
Check for return values of libvirt's internal APIs in vboxDumpVideo and vboxDumpDisplay. Patch 10/13 should fix a complaint by Coverity (untested). Lots of whitespace changes, -b is your friend. Ján Tomko (13): Check return value of vboxDumpVideo vboxDumpDisplay: reduce indentation level vboxDumpDisplay: more indentation reducing vboxDumpDisplay: add addDesktop bool vboxDumpDisplay: remove extra virReportOOMError vboxDumpDisplay: split out def->graphics allocation vboxDumpDisplay: allocate the graphics structure upfront vboxDumpDisplay: fill out the graphics structure earlier vboxDumpDisplay: clean up VIR_STRDUP usage vboxDumpDisplay: check return of virDomainGraphicsListenSetAddress vboxDumpDisplay: use VIR_APPEND_ELEMENT vboxDumpDisplay: reuse the keyUtf16 variable vboxDumpDisplay: remove suspicious strlen src/vbox/vbox_common.c | 249 ++++++++++++++++++++++--------------------------- 1 file changed, 113 insertions(+), 136 deletions(-) -- 2.4.10
Error out on allocation failures instead of creating an incomplete definition. Fixes a possible crash when def->nvideos is 1, but def->videos is NULL. --- src/vbox/vbox_common.c | 59 +++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index d1eb09a..72ba987 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3258,38 +3258,42 @@ vboxDumpIDEHDDsNew(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) } } -static void +static int vboxDumpVideo(virDomainDefPtr def, vboxGlobalData *data ATTRIBUTE_UNUSED, IMachine *machine) { /* dump video options vram/2d/3d/directx/etc. */ + /* the default is: vram is 8MB, One monitor, 3dAccel Off */ + PRUint32 VRAMSize = 8; + PRUint32 monitorCount = 1; + PRBool accelerate3DEnabled = PR_FALSE; + PRBool accelerate2DEnabled = PR_FALSE; + /* Currently supports only one graphics card */ + if (VIR_ALLOC_N(def->videos, 1) < 0) + return -1; def->nvideos = 1; - if (VIR_ALLOC_N(def->videos, def->nvideos) >= 0) { - if (VIR_ALLOC(def->videos[0]) >= 0) { - /* the default is: vram is 8MB, One monitor, 3dAccel Off */ - PRUint32 VRAMSize = 8; - PRUint32 monitorCount = 1; - PRBool accelerate3DEnabled = PR_FALSE; - PRBool accelerate2DEnabled = PR_FALSE; - - gVBoxAPI.UIMachine.GetVRAMSize(machine, &VRAMSize); - gVBoxAPI.UIMachine.GetMonitorCount(machine, &monitorCount); - gVBoxAPI.UIMachine.GetAccelerate3DEnabled(machine, &accelerate3DEnabled); - if (gVBoxAPI.accelerate2DVideo) - gVBoxAPI.UIMachine.GetAccelerate2DVideoEnabled(machine, &accelerate2DEnabled); - - def->videos[0]->type = VIR_DOMAIN_VIDEO_TYPE_VBOX; - def->videos[0]->vram = VRAMSize * 1024; - def->videos[0]->heads = monitorCount; - if (VIR_ALLOC(def->videos[0]->accel) >= 0) { - def->videos[0]->accel->accel3d = accelerate3DEnabled ? - VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; - def->videos[0]->accel->accel2d = accelerate2DEnabled ? - VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; - } - } - } + + if (VIR_ALLOC(def->videos[0]) < 0) + return -1; + + gVBoxAPI.UIMachine.GetVRAMSize(machine, &VRAMSize); + gVBoxAPI.UIMachine.GetMonitorCount(machine, &monitorCount); + gVBoxAPI.UIMachine.GetAccelerate3DEnabled(machine, &accelerate3DEnabled); + if (gVBoxAPI.accelerate2DVideo) + gVBoxAPI.UIMachine.GetAccelerate2DVideoEnabled(machine, &accelerate2DEnabled); + + def->videos[0]->type = VIR_DOMAIN_VIDEO_TYPE_VBOX; + def->videos[0]->vram = VRAMSize * 1024; + def->videos[0]->heads = monitorCount; + if (VIR_ALLOC(def->videos[0]->accel) < 0) + return -1; + def->videos[0]->accel->accel3d = accelerate3DEnabled ? + VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + def->videos[0]->accel->accel2d = accelerate2DEnabled ? + VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + + return 0; } static void @@ -3967,7 +3971,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) * so locatime is always true here */ def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME; - vboxDumpVideo(def, data, machine); + if (vboxDumpVideo(def, data, machine) < 0) + goto cleanup; vboxDumpDisplay(def, data, machine); /* As the medium interface changed from 3.0 to 3.1. -- 2.4.10
On 05.02.2016 18:02, Ján Tomko wrote:
Error out on allocation failures instead of creating an incomplete definition.
Fixes a possible crash when def->nvideos is 1, but def->videos is NULL. --- src/vbox/vbox_common.c | 59 +++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 27 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index d1eb09a..72ba987 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3258,38 +3258,42 @@ vboxDumpIDEHDDsNew(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) } }
-static void +static int vboxDumpVideo(virDomainDefPtr def, vboxGlobalData *data ATTRIBUTE_UNUSED, IMachine *machine) { /* dump video options vram/2d/3d/directx/etc. */ + /* the default is: vram is 8MB, One monitor, 3dAccel Off */ + PRUint32 VRAMSize = 8; + PRUint32 monitorCount = 1; + PRBool accelerate3DEnabled = PR_FALSE; + PRBool accelerate2DEnabled = PR_FALSE; + /* Currently supports only one graphics card */ + if (VIR_ALLOC_N(def->videos, 1) < 0) + return -1; def->nvideos = 1; - if (VIR_ALLOC_N(def->videos, def->nvideos) >= 0) { - if (VIR_ALLOC(def->videos[0]) >= 0) { - /* the default is: vram is 8MB, One monitor, 3dAccel Off */ - PRUint32 VRAMSize = 8; - PRUint32 monitorCount = 1; - PRBool accelerate3DEnabled = PR_FALSE; - PRBool accelerate2DEnabled = PR_FALSE; - - gVBoxAPI.UIMachine.GetVRAMSize(machine, &VRAMSize); - gVBoxAPI.UIMachine.GetMonitorCount(machine, &monitorCount); - gVBoxAPI.UIMachine.GetAccelerate3DEnabled(machine, &accelerate3DEnabled); - if (gVBoxAPI.accelerate2DVideo) - gVBoxAPI.UIMachine.GetAccelerate2DVideoEnabled(machine, &accelerate2DEnabled); - - def->videos[0]->type = VIR_DOMAIN_VIDEO_TYPE_VBOX; - def->videos[0]->vram = VRAMSize * 1024; - def->videos[0]->heads = monitorCount; - if (VIR_ALLOC(def->videos[0]->accel) >= 0) { - def->videos[0]->accel->accel3d = accelerate3DEnabled ? - VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; - def->videos[0]->accel->accel2d = accelerate2DEnabled ? - VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; - } - } - } + + if (VIR_ALLOC(def->videos[0]) < 0) + return -1; + + gVBoxAPI.UIMachine.GetVRAMSize(machine, &VRAMSize); + gVBoxAPI.UIMachine.GetMonitorCount(machine, &monitorCount); + gVBoxAPI.UIMachine.GetAccelerate3DEnabled(machine, &accelerate3DEnabled); + if (gVBoxAPI.accelerate2DVideo) + gVBoxAPI.UIMachine.GetAccelerate2DVideoEnabled(machine, &accelerate2DEnabled); + + def->videos[0]->type = VIR_DOMAIN_VIDEO_TYPE_VBOX; + def->videos[0]->vram = VRAMSize * 1024; + def->videos[0]->heads = monitorCount;
While touching this, you can drop this weird indentation. I know we use it throughout whole driver, but one day I'd like to see that changed too. Same applies for the rest of patches.
+ if (VIR_ALLOC(def->videos[0]->accel) < 0) + return -1; + def->videos[0]->accel->accel3d = accelerate3DEnabled ? + VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + def->videos[0]->accel->accel2d = accelerate2DEnabled ? + VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO; + + return 0; }
static void @@ -3967,7 +3971,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) * so locatime is always true here */ def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME;
- vboxDumpVideo(def, data, machine); + if (vboxDumpVideo(def, data, machine) < 0) + goto cleanup; vboxDumpDisplay(def, data, machine);
/* As the medium interface changed from 3.0 to 3.1.
ACK Michal
Use STREQ_NULLABLE instead of deep nesting. --- src/vbox/vbox_common.c | 75 +++++++++++++++++++++++++------------------------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 72ba987..ac47728 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3321,54 +3321,53 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) if (valueTypeUtf16) { VBOX_UTF16_TO_UTF8(valueTypeUtf16, &valueTypeUtf8); VBOX_UTF16_FREE(valueTypeUtf16); + } - if (STREQ(valueTypeUtf8, "sdl") || STREQ(valueTypeUtf8, "gui")) { - PRUnichar *keyDislpayUtf16 = NULL; - PRUnichar *valueDisplayUtf16 = NULL; - char *valueDisplayUtf8 = NULL; + if (STREQ_NULLABLE(valueTypeUtf8, "sdl") || + STREQ_NULLABLE(valueTypeUtf8, "gui")) { + PRUnichar *keyDislpayUtf16 = NULL; + PRUnichar *valueDisplayUtf16 = NULL; + char *valueDisplayUtf8 = NULL; - VBOX_UTF8_TO_UTF16("FRONTEND/Display", &keyDislpayUtf16); - gVBoxAPI.UIMachine.GetExtraData(machine, keyDislpayUtf16, &valueDisplayUtf16); - VBOX_UTF16_FREE(keyDislpayUtf16); + VBOX_UTF8_TO_UTF16("FRONTEND/Display", &keyDislpayUtf16); + gVBoxAPI.UIMachine.GetExtraData(machine, keyDislpayUtf16, &valueDisplayUtf16); + VBOX_UTF16_FREE(keyDislpayUtf16); - if (valueDisplayUtf16) { - VBOX_UTF16_TO_UTF8(valueDisplayUtf16, &valueDisplayUtf8); - VBOX_UTF16_FREE(valueDisplayUtf16); + if (valueDisplayUtf16) { + VBOX_UTF16_TO_UTF8(valueDisplayUtf16, &valueDisplayUtf8); + VBOX_UTF16_FREE(valueDisplayUtf16); - if (strlen(valueDisplayUtf8) <= 0) - VBOX_UTF8_FREE(valueDisplayUtf8); - } + if (strlen(valueDisplayUtf8) <= 0) + VBOX_UTF8_FREE(valueDisplayUtf8); + } - if (STREQ(valueTypeUtf8, "sdl")) { - sdlPresent = 1; - if (VIR_STRDUP(sdlDisplay, valueDisplayUtf8) < 0) { - /* 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 - */ - } - totalPresent++; + if (STREQ(valueTypeUtf8, "sdl")) { + sdlPresent = 1; + if (VIR_STRDUP(sdlDisplay, valueDisplayUtf8) < 0) { + /* 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 + */ } + totalPresent++; + } - if (STREQ(valueTypeUtf8, "gui")) { - guiPresent = 1; - if (VIR_STRDUP(guiDisplay, valueDisplayUtf8) < 0) { - /* just don't go to cleanup yet as it is ok to have - * guiDisplay as NULL and we check it below if it - * exist and then only use it there - */ - } - totalPresent++; + if (STREQ(valueTypeUtf8, "gui")) { + guiPresent = 1; + if (VIR_STRDUP(guiDisplay, valueDisplayUtf8) < 0) { + /* just don't go to cleanup yet as it is ok to have + * guiDisplay as NULL and we check it below if it + * exist and then only use it there + */ } - VBOX_UTF8_FREE(valueDisplayUtf8); + totalPresent++; } - - if (STREQ(valueTypeUtf8, "vrdp")) - vrdpPresent = 1; - - VBOX_UTF8_FREE(valueTypeUtf8); + VBOX_UTF8_FREE(valueDisplayUtf8); } + if (STREQ_NULLABLE(valueTypeUtf8, "vrdp")) + vrdpPresent = 1; + 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; @@ -3441,6 +3440,8 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) } VBOX_RELEASE(VRDxServer); } + + VBOX_UTF8_FREE(valueTypeUtf8); } static void -- 2.4.10
VRDxEnabled is initialized to false. Put the if (VRDxEnabled) on the top level to reduce nesting. --- src/vbox/vbox_common.c | 62 +++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index ac47728..1c9d871 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3398,49 +3398,49 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) } gVBoxAPI.UIMachine.GetVRDxServer(machine, &VRDxServer); - if (VRDxServer) { + if (VRDxServer) gVBoxAPI.UIVRDxServer.GetEnabled(VRDxServer, &VRDxEnabled); - if (VRDxEnabled) { - totalPresent++; + if (VRDxEnabled) { - if ((VIR_REALLOC_N(def->graphics, totalPresent) >= 0) && - (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0)) { - PRUnichar *netAddressUtf16 = NULL; - char *netAddressUtf8 = NULL; - PRBool allowMultiConnection = PR_FALSE; - PRBool reuseSingleConnection = PR_FALSE; + totalPresent++; - gVBoxAPI.UIVRDxServer.GetPorts(data, VRDxServer, def->graphics[def->ngraphics]); + if ((VIR_REALLOC_N(def->graphics, totalPresent) >= 0) && + (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0)) { + PRUnichar *netAddressUtf16 = NULL; + char *netAddressUtf8 = NULL; + PRBool allowMultiConnection = PR_FALSE; + PRBool reuseSingleConnection = PR_FALSE; - def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP; + gVBoxAPI.UIVRDxServer.GetPorts(data, VRDxServer, def->graphics[def->ngraphics]); - gVBoxAPI.UIVRDxServer.GetNetAddress(data, VRDxServer, &netAddressUtf16); - if (netAddressUtf16) { - VBOX_UTF16_TO_UTF8(netAddressUtf16, &netAddressUtf8); - if (STRNEQ(netAddressUtf8, "")) - virDomainGraphicsListenSetAddress(def->graphics[def->ngraphics], 0, - netAddressUtf8, -1, true); - VBOX_UTF16_FREE(netAddressUtf16); - VBOX_UTF8_FREE(netAddressUtf8); - } + def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP; - gVBoxAPI.UIVRDxServer.GetAllowMultiConnection(VRDxServer, &allowMultiConnection); - if (allowMultiConnection) - def->graphics[def->ngraphics]->data.rdp.multiUser = true; + gVBoxAPI.UIVRDxServer.GetNetAddress(data, VRDxServer, &netAddressUtf16); + if (netAddressUtf16) { + VBOX_UTF16_TO_UTF8(netAddressUtf16, &netAddressUtf8); + if (STRNEQ(netAddressUtf8, "")) + virDomainGraphicsListenSetAddress(def->graphics[def->ngraphics], 0, + netAddressUtf8, -1, true); + VBOX_UTF16_FREE(netAddressUtf16); + VBOX_UTF8_FREE(netAddressUtf8); + } - gVBoxAPI.UIVRDxServer.GetReuseSingleConnection(VRDxServer, &reuseSingleConnection); - if (reuseSingleConnection) - def->graphics[def->ngraphics]->data.rdp.replaceUser = true; + gVBoxAPI.UIVRDxServer.GetAllowMultiConnection(VRDxServer, &allowMultiConnection); + if (allowMultiConnection) + def->graphics[def->ngraphics]->data.rdp.multiUser = true; - def->ngraphics++; - } else { - virReportOOMError(); - } + gVBoxAPI.UIVRDxServer.GetReuseSingleConnection(VRDxServer, &reuseSingleConnection); + if (reuseSingleConnection) + def->graphics[def->ngraphics]->data.rdp.replaceUser = true; + + def->ngraphics++; + } else { + virReportOOMError(); } - VBOX_RELEASE(VRDxServer); } + VBOX_RELEASE(VRDxServer); VBOX_UTF8_FREE(valueTypeUtf8); } -- 2.4.10
When FRONTEND/Type is not any of "sdl", "gui", "vrdp", we add a DESKTOP. Use a bool to track this, instead of checking that both totalPresent ("sdl" or "gui" present) and vrdpPresent are zero. --- src/vbox/vbox_common.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 1c9d871..dc00a3a 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3300,7 +3300,6 @@ static void vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) { /* dump display options vrdp/gui/sdl */ - int vrdpPresent = 0; int sdlPresent = 0; int guiPresent = 0; int totalPresent = 0; @@ -3311,6 +3310,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) char *valueTypeUtf8 = NULL; IVRDxServer *VRDxServer = NULL; PRBool VRDxEnabled = PR_FALSE; + bool addDesktop = false; def->ngraphics = 0; @@ -3363,11 +3363,10 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) totalPresent++; } VBOX_UTF8_FREE(valueDisplayUtf8); + } else if (STRNEQ_NULLABLE(valueTypeUtf8, "vrdp")) { + addDesktop = true; } - if (STREQ_NULLABLE(valueTypeUtf8, "vrdp")) - vrdpPresent = 1; - 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; @@ -3382,7 +3381,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) def->graphics[def->ngraphics]->data.sdl.display = sdlDisplay; def->ngraphics++; } - } else if ((vrdpPresent != 1) && (totalPresent == 0) && (VIR_ALLOC_N(def->graphics, 1) >= 0)) { + } else if (addDesktop && (VIR_ALLOC_N(def->graphics, 1) >= 0)) { if (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0) { const char *tmp; def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; -- 2.4.10
VIR_ALLOC* already reported an error. --- src/vbox/vbox_common.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index dc00a3a..54e7d47 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3434,8 +3434,6 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) def->graphics[def->ngraphics]->data.rdp.replaceUser = true; def->ngraphics++; - } else { - virReportOOMError(); } } -- 2.4.10
Separate allocation of the def->graphics array from the allocation and initialization of its first element. Note that the only possible values of totalPresent at this point are 0 or 1, because it equals to guiPresent + sdlPresent. --- src/vbox/vbox_common.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 54e7d47..e609e02 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3296,7 +3296,7 @@ vboxDumpVideo(virDomainDefPtr def, vboxGlobalData *data ATTRIBUTE_UNUSED, return 0; } -static void +static int vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) { /* dump display options vrdp/gui/sdl */ @@ -3311,6 +3311,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) IVRDxServer *VRDxServer = NULL; PRBool VRDxEnabled = PR_FALSE; bool addDesktop = false; + int ret = -1; def->ngraphics = 0; @@ -3367,7 +3368,12 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) addDesktop = true; } - if ((totalPresent > 0) && (VIR_ALLOC_N(def->graphics, totalPresent) >= 0)) { + if (totalPresent > 0 || addDesktop) { + if (VIR_ALLOC_N(def->graphics, 1) < 0) + goto cleanup; + } + + if (totalPresent > 0) { if ((guiPresent) && (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0)) { def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; if (guiDisplay) @@ -3381,7 +3387,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) def->graphics[def->ngraphics]->data.sdl.display = sdlDisplay; def->ngraphics++; } - } else if (addDesktop && (VIR_ALLOC_N(def->graphics, 1) >= 0)) { + } else if (addDesktop) { if (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0) { const char *tmp; def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; @@ -3401,11 +3407,10 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) gVBoxAPI.UIVRDxServer.GetEnabled(VRDxServer, &VRDxEnabled); if (VRDxEnabled) { + if (VIR_REALLOC_N(def->graphics, totalPresent) < 0) + goto cleanup; - totalPresent++; - - if ((VIR_REALLOC_N(def->graphics, totalPresent) >= 0) && - (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0)) { + if (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0) { PRUnichar *netAddressUtf16 = NULL; char *netAddressUtf8 = NULL; PRBool allowMultiConnection = PR_FALSE; @@ -3437,8 +3442,14 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) } } + ret = 0; + + cleanup: + VBOX_UTF8_FREE(guiDisplay); + VBOX_UTF8_FREE(sdlDisplay); VBOX_RELEASE(VRDxServer); VBOX_UTF8_FREE(valueTypeUtf8); + return ret; } static void @@ -3971,7 +3982,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) if (vboxDumpVideo(def, data, machine) < 0) goto cleanup; - vboxDumpDisplay(def, data, machine); + if (vboxDumpDisplay(def, data, machine) < 0) + goto cleanup; /* As the medium interface changed from 3.0 to 3.1. * There are two totally different implementations. -- 2.4.10
Allocate it as soon as we know we will need it. Add it to def->ngraphics if it's allocated, removing the need to use the addDesktop and totalPresent variables to track this. --- src/vbox/vbox_common.c | 77 +++++++++++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index e609e02..7a24de4 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3302,7 +3302,6 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) /* dump display options vrdp/gui/sdl */ int sdlPresent = 0; int guiPresent = 0; - int totalPresent = 0; char *guiDisplay = NULL; char *sdlDisplay = NULL; PRUnichar *keyTypeUtf16 = NULL; @@ -3310,6 +3309,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) char *valueTypeUtf8 = NULL; IVRDxServer *VRDxServer = NULL; PRBool VRDxEnabled = PR_FALSE; + virDomainGraphicsDefPtr graphics = NULL; bool addDesktop = false; int ret = -1; @@ -3330,6 +3330,9 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) PRUnichar *valueDisplayUtf16 = NULL; char *valueDisplayUtf8 = NULL; + if (VIR_ALLOC(graphics) < 0) + goto cleanup; + VBOX_UTF8_TO_UTF16("FRONTEND/Display", &keyDislpayUtf16); gVBoxAPI.UIMachine.GetExtraData(machine, keyDislpayUtf16, &valueDisplayUtf16); VBOX_UTF16_FREE(keyDislpayUtf16); @@ -3350,7 +3353,6 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) * exist and then only use it there */ } - totalPresent++; } if (STREQ(valueTypeUtf8, "gui")) { @@ -3361,34 +3363,36 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) * exist and then only use it there */ } - totalPresent++; } VBOX_UTF8_FREE(valueDisplayUtf8); } else if (STRNEQ_NULLABLE(valueTypeUtf8, "vrdp")) { - addDesktop = true; + if (VIR_ALLOC(graphics) < 0) + goto cleanup; } - if (totalPresent > 0 || addDesktop) { + if (graphics) { if (VIR_ALLOC_N(def->graphics, 1) < 0) goto cleanup; + + def->graphics[def->ngraphics] = graphics; + graphics = NULL; } - if (totalPresent > 0) { - if ((guiPresent) && (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0)) { + if (guiPresent || sdlPresent) { + if ((guiPresent)) { def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; if (guiDisplay) def->graphics[def->ngraphics]->data.desktop.display = guiDisplay; def->ngraphics++; } - if ((sdlPresent) && (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0)) { + if ((sdlPresent)) { def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_SDL; if (sdlDisplay) def->graphics[def->ngraphics]->data.sdl.display = sdlDisplay; def->ngraphics++; } } else if (addDesktop) { - if (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0) { const char *tmp; def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; tmp = virGetEnvBlockSUID("DISPLAY"); @@ -3397,9 +3401,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) * display as NULL */ } - totalPresent++; def->ngraphics++; - } } gVBoxAPI.UIMachine.GetVRDxServer(machine, &VRDxServer); @@ -3407,39 +3409,42 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) gVBoxAPI.UIVRDxServer.GetEnabled(VRDxServer, &VRDxEnabled); if (VRDxEnabled) { - if (VIR_REALLOC_N(def->graphics, totalPresent) < 0) + PRUnichar *netAddressUtf16 = NULL; + char *netAddressUtf8 = NULL; + PRBool allowMultiConnection = PR_FALSE; + PRBool reuseSingleConnection = PR_FALSE; + + if (VIR_ALLOC(graphics) < 0) goto cleanup; - if (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0) { - PRUnichar *netAddressUtf16 = NULL; - char *netAddressUtf8 = NULL; - PRBool allowMultiConnection = PR_FALSE; - PRBool reuseSingleConnection = PR_FALSE; + if (VIR_REALLOC_N(def->graphics, def->ngraphics + 1) < 0) + goto cleanup; - gVBoxAPI.UIVRDxServer.GetPorts(data, VRDxServer, def->graphics[def->ngraphics]); + def->graphics[def->ngraphics] = graphics; - def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP; + gVBoxAPI.UIVRDxServer.GetPorts(data, VRDxServer, def->graphics[def->ngraphics]); - gVBoxAPI.UIVRDxServer.GetNetAddress(data, VRDxServer, &netAddressUtf16); - if (netAddressUtf16) { - VBOX_UTF16_TO_UTF8(netAddressUtf16, &netAddressUtf8); - if (STRNEQ(netAddressUtf8, "")) - virDomainGraphicsListenSetAddress(def->graphics[def->ngraphics], 0, - netAddressUtf8, -1, true); - VBOX_UTF16_FREE(netAddressUtf16); - VBOX_UTF8_FREE(netAddressUtf8); - } + def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP; + + gVBoxAPI.UIVRDxServer.GetNetAddress(data, VRDxServer, &netAddressUtf16); + if (netAddressUtf16) { + VBOX_UTF16_TO_UTF8(netAddressUtf16, &netAddressUtf8); + if (STRNEQ(netAddressUtf8, "")) + virDomainGraphicsListenSetAddress(def->graphics[def->ngraphics], 0, + netAddressUtf8, -1, true); + VBOX_UTF16_FREE(netAddressUtf16); + VBOX_UTF8_FREE(netAddressUtf8); + } - gVBoxAPI.UIVRDxServer.GetAllowMultiConnection(VRDxServer, &allowMultiConnection); - if (allowMultiConnection) - def->graphics[def->ngraphics]->data.rdp.multiUser = true; + gVBoxAPI.UIVRDxServer.GetAllowMultiConnection(VRDxServer, &allowMultiConnection); + if (allowMultiConnection) + def->graphics[def->ngraphics]->data.rdp.multiUser = true; - gVBoxAPI.UIVRDxServer.GetReuseSingleConnection(VRDxServer, &reuseSingleConnection); - if (reuseSingleConnection) - def->graphics[def->ngraphics]->data.rdp.replaceUser = true; + gVBoxAPI.UIVRDxServer.GetReuseSingleConnection(VRDxServer, &reuseSingleConnection); + if (reuseSingleConnection) + def->graphics[def->ngraphics]->data.rdp.replaceUser = true; - def->ngraphics++; - } + def->ngraphics++; } ret = 0; -- 2.4.10
Remove the need to track what type of graphics were present by temporary variables. --- src/vbox/vbox_common.c | 68 +++++++++++++++++++------------------------------- 1 file changed, 25 insertions(+), 43 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 7a24de4..e273de9 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3300,8 +3300,6 @@ static int vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) { /* dump display options vrdp/gui/sdl */ - int sdlPresent = 0; - int guiPresent = 0; char *guiDisplay = NULL; char *sdlDisplay = NULL; PRUnichar *keyTypeUtf16 = NULL; @@ -3310,7 +3308,6 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) IVRDxServer *VRDxServer = NULL; PRBool VRDxEnabled = PR_FALSE; virDomainGraphicsDefPtr graphics = NULL; - bool addDesktop = false; int ret = -1; def->ngraphics = 0; @@ -3346,28 +3343,41 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) } if (STREQ(valueTypeUtf8, "sdl")) { - sdlPresent = 1; if (VIR_STRDUP(sdlDisplay, valueDisplayUtf8) < 0) { /* 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 */ } + graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_SDL; + if (sdlDisplay) + graphics->data.sdl.display = sdlDisplay; } if (STREQ(valueTypeUtf8, "gui")) { - guiPresent = 1; if (VIR_STRDUP(guiDisplay, valueDisplayUtf8) < 0) { /* just don't go to cleanup yet as it is ok to have * guiDisplay as NULL and we check it below if it * exist and then only use it there */ } + graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; + if (guiDisplay) + graphics->data.desktop.display = guiDisplay; } VBOX_UTF8_FREE(valueDisplayUtf8); } else if (STRNEQ_NULLABLE(valueTypeUtf8, "vrdp")) { + const char *tmp; if (VIR_ALLOC(graphics) < 0) goto cleanup; + + graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; + tmp = virGetEnvBlockSUID("DISPLAY"); + if (VIR_STRDUP(graphics->data.desktop.display, tmp) < 0) { + /* just don't go to cleanup yet as it is ok to have + * display as NULL + */ + } } if (graphics) { @@ -3376,32 +3386,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) def->graphics[def->ngraphics] = graphics; graphics = NULL; - } - - if (guiPresent || sdlPresent) { - if ((guiPresent)) { - def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; - if (guiDisplay) - def->graphics[def->ngraphics]->data.desktop.display = guiDisplay; - def->ngraphics++; - } - - if ((sdlPresent)) { - def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_SDL; - if (sdlDisplay) - def->graphics[def->ngraphics]->data.sdl.display = sdlDisplay; - def->ngraphics++; - } - } else if (addDesktop) { - const char *tmp; - def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; - tmp = virGetEnvBlockSUID("DISPLAY"); - if (VIR_STRDUP(def->graphics[def->ngraphics]->data.desktop.display, tmp) < 0) { - /* just don't go to cleanup yet as it is ok to have - * display as NULL - */ - } - def->ngraphics++; + def->ngraphics++; } gVBoxAPI.UIMachine.GetVRDxServer(machine, &VRDxServer); @@ -3417,20 +3402,15 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) if (VIR_ALLOC(graphics) < 0) goto cleanup; - if (VIR_REALLOC_N(def->graphics, def->ngraphics + 1) < 0) - goto cleanup; + gVBoxAPI.UIVRDxServer.GetPorts(data, VRDxServer, graphics); - def->graphics[def->ngraphics] = graphics; - - gVBoxAPI.UIVRDxServer.GetPorts(data, VRDxServer, def->graphics[def->ngraphics]); - - def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP; + graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP; gVBoxAPI.UIVRDxServer.GetNetAddress(data, VRDxServer, &netAddressUtf16); if (netAddressUtf16) { VBOX_UTF16_TO_UTF8(netAddressUtf16, &netAddressUtf8); if (STRNEQ(netAddressUtf8, "")) - virDomainGraphicsListenSetAddress(def->graphics[def->ngraphics], 0, + virDomainGraphicsListenSetAddress(graphics, 0, netAddressUtf8, -1, true); VBOX_UTF16_FREE(netAddressUtf16); VBOX_UTF8_FREE(netAddressUtf8); @@ -3438,20 +3418,22 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) gVBoxAPI.UIVRDxServer.GetAllowMultiConnection(VRDxServer, &allowMultiConnection); if (allowMultiConnection) - def->graphics[def->ngraphics]->data.rdp.multiUser = true; + graphics->data.rdp.multiUser = true; gVBoxAPI.UIVRDxServer.GetReuseSingleConnection(VRDxServer, &reuseSingleConnection); if (reuseSingleConnection) - def->graphics[def->ngraphics]->data.rdp.replaceUser = true; + graphics->data.rdp.replaceUser = true; + + if (VIR_REALLOC_N(def->graphics, def->ngraphics + 1) < 0) + goto cleanup; + def->graphics[def->ngraphics] = graphics; def->ngraphics++; } ret = 0; cleanup: - VBOX_UTF8_FREE(guiDisplay); - VBOX_UTF8_FREE(sdlDisplay); VBOX_RELEASE(VRDxServer); VBOX_UTF8_FREE(valueTypeUtf8); return ret; -- 2.4.10
Two VIR_STRDUP calls are redundant - just steal the string converted by VBOX_UTF16_TO_UTF8. Report an error when the third one fails. --- src/vbox/vbox_common.c | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index e273de9..43de343 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3300,8 +3300,6 @@ static int vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) { /* dump display options vrdp/gui/sdl */ - char *guiDisplay = NULL; - char *sdlDisplay = NULL; PRUnichar *keyTypeUtf16 = NULL; PRUnichar *valueTypeUtf16 = NULL; char *valueTypeUtf8 = NULL; @@ -3343,41 +3341,25 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) } if (STREQ(valueTypeUtf8, "sdl")) { - if (VIR_STRDUP(sdlDisplay, valueDisplayUtf8) < 0) { - /* 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 - */ - } graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_SDL; - if (sdlDisplay) - graphics->data.sdl.display = sdlDisplay; + graphics->data.sdl.display = valueDisplayUtf8; + valueDisplayUtf8 = NULL; } if (STREQ(valueTypeUtf8, "gui")) { - if (VIR_STRDUP(guiDisplay, valueDisplayUtf8) < 0) { - /* just don't go to cleanup yet as it is ok to have - * guiDisplay as NULL and we check it below if it - * exist and then only use it there - */ - } graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; - if (guiDisplay) - graphics->data.desktop.display = guiDisplay; + graphics->data.desktop.display = valueDisplayUtf8; + valueDisplayUtf8 = NULL; } VBOX_UTF8_FREE(valueDisplayUtf8); } else if (STRNEQ_NULLABLE(valueTypeUtf8, "vrdp")) { - const char *tmp; if (VIR_ALLOC(graphics) < 0) goto cleanup; graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; - tmp = virGetEnvBlockSUID("DISPLAY"); - if (VIR_STRDUP(graphics->data.desktop.display, tmp) < 0) { - /* just don't go to cleanup yet as it is ok to have - * display as NULL - */ - } + if (VIR_STRDUP(graphics->data.desktop.display, + virGetEnvBlockSUID("DISPLAY")) < 0) + goto cleanup; } if (graphics) { -- 2.4.10
Error out if the allocation failed. --- src/vbox/vbox_common.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 43de343..1c2a432 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3303,6 +3303,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) PRUnichar *keyTypeUtf16 = NULL; PRUnichar *valueTypeUtf16 = NULL; char *valueTypeUtf8 = NULL; + char *netAddressUtf8 = NULL; IVRDxServer *VRDxServer = NULL; PRBool VRDxEnabled = PR_FALSE; virDomainGraphicsDefPtr graphics = NULL; @@ -3377,7 +3378,6 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) if (VRDxEnabled) { PRUnichar *netAddressUtf16 = NULL; - char *netAddressUtf8 = NULL; PRBool allowMultiConnection = PR_FALSE; PRBool reuseSingleConnection = PR_FALSE; @@ -3391,13 +3391,14 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) gVBoxAPI.UIVRDxServer.GetNetAddress(data, VRDxServer, &netAddressUtf16); if (netAddressUtf16) { VBOX_UTF16_TO_UTF8(netAddressUtf16, &netAddressUtf8); - if (STRNEQ(netAddressUtf8, "")) - virDomainGraphicsListenSetAddress(graphics, 0, - netAddressUtf8, -1, true); VBOX_UTF16_FREE(netAddressUtf16); - VBOX_UTF8_FREE(netAddressUtf8); } + if (STRNEQ_NULLABLE(netAddressUtf8, "") && + virDomainGraphicsListenSetAddress(graphics, 0, + netAddressUtf8, -1, true) < 0) + goto cleanup; + gVBoxAPI.UIVRDxServer.GetAllowMultiConnection(VRDxServer, &allowMultiConnection); if (allowMultiConnection) graphics->data.rdp.multiUser = true; @@ -3418,6 +3419,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) cleanup: VBOX_RELEASE(VRDxServer); VBOX_UTF8_FREE(valueTypeUtf8); + VBOX_UTF8_FREE(netAddressUtf8); return ret; } -- 2.4.10
Instead of open-coding it. --- src/vbox/vbox_common.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 1c2a432..236e048 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3363,14 +3363,9 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) goto cleanup; } - if (graphics) { - if (VIR_ALLOC_N(def->graphics, 1) < 0) - goto cleanup; - - def->graphics[def->ngraphics] = graphics; - graphics = NULL; - def->ngraphics++; - } + if (graphics && + VIR_APPEND_ELEMENT(def->graphics, def->ngraphics, graphics) < 0) + goto cleanup; gVBoxAPI.UIMachine.GetVRDxServer(machine, &VRDxServer); if (VRDxServer) @@ -3407,11 +3402,8 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) if (reuseSingleConnection) graphics->data.rdp.replaceUser = true; - if (VIR_REALLOC_N(def->graphics, def->ngraphics + 1) < 0) + if (VIR_APPEND_ELEMENT(def->graphics, def->ngraphics, graphics) < 0) goto cleanup; - - def->graphics[def->ngraphics] = graphics; - def->ngraphics++; } ret = 0; -- 2.4.10
We free the key right after calling the API. Reuse a single variable to remove the typo. --- src/vbox/vbox_common.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 236e048..71fc62b 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3300,7 +3300,7 @@ static int vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) { /* dump display options vrdp/gui/sdl */ - PRUnichar *keyTypeUtf16 = NULL; + PRUnichar *keyUtf16 = NULL; PRUnichar *valueTypeUtf16 = NULL; char *valueTypeUtf8 = NULL; char *netAddressUtf8 = NULL; @@ -3311,9 +3311,9 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) def->ngraphics = 0; - VBOX_UTF8_TO_UTF16("FRONTEND/Type", &keyTypeUtf16); - gVBoxAPI.UIMachine.GetExtraData(machine, keyTypeUtf16, &valueTypeUtf16); - VBOX_UTF16_FREE(keyTypeUtf16); + VBOX_UTF8_TO_UTF16("FRONTEND/Type", &keyUtf16); + gVBoxAPI.UIMachine.GetExtraData(machine, keyUtf16, &valueTypeUtf16); + VBOX_UTF16_FREE(keyUtf16); if (valueTypeUtf16) { VBOX_UTF16_TO_UTF8(valueTypeUtf16, &valueTypeUtf8); @@ -3322,16 +3322,15 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) if (STREQ_NULLABLE(valueTypeUtf8, "sdl") || STREQ_NULLABLE(valueTypeUtf8, "gui")) { - PRUnichar *keyDislpayUtf16 = NULL; PRUnichar *valueDisplayUtf16 = NULL; char *valueDisplayUtf8 = NULL; if (VIR_ALLOC(graphics) < 0) goto cleanup; - VBOX_UTF8_TO_UTF16("FRONTEND/Display", &keyDislpayUtf16); - gVBoxAPI.UIMachine.GetExtraData(machine, keyDislpayUtf16, &valueDisplayUtf16); - VBOX_UTF16_FREE(keyDislpayUtf16); + VBOX_UTF8_TO_UTF16("FRONTEND/Display", &keyUtf16); + gVBoxAPI.UIMachine.GetExtraData(machine, keyUtf16, &valueDisplayUtf16); + VBOX_UTF16_FREE(keyUtf16); if (valueDisplayUtf16) { VBOX_UTF16_TO_UTF8(valueDisplayUtf16, &valueDisplayUtf8); -- 2.4.10
The return type of strlen is 'size_t', which is unsigned and therefore never less than zero. Use STREQ to make the check obvious. --- src/vbox/vbox_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 71fc62b..0c0dc96 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3336,7 +3336,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) VBOX_UTF16_TO_UTF8(valueDisplayUtf16, &valueDisplayUtf8); VBOX_UTF16_FREE(valueDisplayUtf16); - if (strlen(valueDisplayUtf8) <= 0) + if (STREQ(valueDisplayUtf8, "")) VBOX_UTF8_FREE(valueDisplayUtf8); } -- 2.4.10
On 05.02.2016 18:02, Ján Tomko wrote:
Check for return values of libvirt's internal APIs in vboxDumpVideo and vboxDumpDisplay.
Patch 10/13 should fix a complaint by Coverity (untested).
Lots of whitespace changes, -b is your friend.
Ján Tomko (13): Check return value of vboxDumpVideo vboxDumpDisplay: reduce indentation level vboxDumpDisplay: more indentation reducing vboxDumpDisplay: add addDesktop bool vboxDumpDisplay: remove extra virReportOOMError vboxDumpDisplay: split out def->graphics allocation vboxDumpDisplay: allocate the graphics structure upfront vboxDumpDisplay: fill out the graphics structure earlier vboxDumpDisplay: clean up VIR_STRDUP usage vboxDumpDisplay: check return of virDomainGraphicsListenSetAddress vboxDumpDisplay: use VIR_APPEND_ELEMENT vboxDumpDisplay: reuse the keyUtf16 variable vboxDumpDisplay: remove suspicious strlen
src/vbox/vbox_common.c | 249 ++++++++++++++++++++++--------------------------- 1 file changed, 113 insertions(+), 136 deletions(-)
ACK series. Any intention to continue or is this just a one timer? Michal
participants (2)
-
Ján Tomko -
Michal Privoznik