[libvirt] [PATCH 01/13] Check return value of vboxDumpVideo

[Michal Privoznik]

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