[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 26/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/vbox/*



On 05/03/2013 04:53 PM, Michal Privoznik wrote:
> ---
>  src/vbox/vbox_XPCOMCGlue.c |   6 +-
>  src/vbox/vbox_tmpl.c       | 278 +++++++++++++++++++--------------------------
>  2 files changed, 117 insertions(+), 167 deletions(-)
> 
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 43ddac8..4ac7b91 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -2290,7 +2288,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) {
>              def->virtType = VIR_DOMAIN_VIRT_VBOX;
>              def->id = dom->id;
>              memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN);
> -            def->name = strdup(dom->name);
> +            if (VIR_STRDUP(def->name, dom->name) < 0)
> +                goto cleanup;

Bailing out after one unsuccessful strdup? Other parts of this function don't
share this defeatist attitude.

>  
>              machine->vtbl->GetMemorySize(machine, &memorySize);
>              def->mem.cur_balloon = memorySize * 1024;

> @@ -2460,10 +2460,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) {
>  
>                          if (STREQ(valueTypeUtf8, "sdl")) {
>                              sdlPresent = 1;
> -                            if (valueDisplayUtf8)
> -                                sdlDisplay = strdup(valueDisplayUtf8);
> -                            if (sdlDisplay == NULL) {
> -                                virReportOOMError();
> +                            if (valueDisplayUtf8 &&
> +                                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
> @@ -2474,10 +2472,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) {
>  
>                          if (STREQ(valueTypeUtf8, "gui")) {
>                              guiPresent = 1;
> -                            if (valueDisplayUtf8)
> -                                guiDisplay = strdup(valueDisplayUtf8);
> -                            if (guiDisplay == NULL) {
> -                                virReportOOMError();
> +                            if (valueDisplayUtf8 &&
> +                                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
> @@ -2512,14 +2508,11 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) {
>                      if (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0) {
>                          def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP;
>                          tmp = getenv("DISPLAY");
> -                        if (tmp != NULL) {
> -                            def->graphics[def->ngraphics]->data.desktop.display = strdup(tmp);
> -                            if (def->graphics[def->ngraphics]->data.desktop.display == NULL) {
> -                                virReportOOMError();
> -                                /* just don't go to cleanup yet as it is ok to have
> -                                 * display as NULL
> -                                 */
> -                            }
> +                        if (tmp &&
> +                            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
> +                             */
>                          }
>                          totalPresent++;
>                          def->ngraphics++;

You have preserved the existing behavior in these three hunks...

> @@ -2649,9 +2642,9 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) {
>  
>                  if (hddType == HardDiskType_Immutable)
>                      def->disks[hddNum]->readonly = true;
> -                def->disks[hddNum]->src = strdup(hddlocation);
> -                def->disks[hddNum]->dst = strdup("hda");
> -                hddNum++;
> +                if (VIR_STRDUP(def->disks[hddNum]->src, hddlocation) == 0 &&
> +                    VIR_STRDUP(def->disks[hddNum]->dst, "hda") == 0)
> +                    hddNum++;
>  
>                  VBOX_UTF8_FREE(hddlocation);
>                  VBOX_UTF16_FREE(hddlocationUtf16);
> @@ -2670,9 +2663,9 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) {
>  
>                  if (hddType == HardDiskType_Immutable)
>                      def->disks[hddNum]->readonly = true;
> -                def->disks[hddNum]->src = strdup(hddlocation);
> -                def->disks[hddNum]->dst = strdup("hdb");
> -                hddNum++;
> +                if (VIR_STRDUP(def->disks[hddNum]->src, hddlocation) == 0 &&
> +                    VIR_STRDUP(def->disks[hddNum]->dst, "hdb") == 0)
> +                    hddNum++;
>  
>                  VBOX_UTF8_FREE(hddlocation);
>                  VBOX_UTF16_FREE(hddlocationUtf16);
> @@ -2691,9 +2684,9 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) {
>  
>                  if (hddType == HardDiskType_Immutable)
>                      def->disks[hddNum]->readonly = true;
> -                def->disks[hddNum]->src = strdup(hddlocation);
> -                def->disks[hddNum]->dst = strdup("hdd");
> -                hddNum++;
> +                if (VIR_STRDUP_QUIET(def->disks[hddNum]->src, hddlocation) == 0 &&
> +                    VIR_STRDUP_QUIET(def->disks[hddNum]->dst, "hdd") == 0)
> +                    hddNum++;
>  
>                  VBOX_UTF8_FREE(hddlocation);
>                  VBOX_UTF16_FREE(hddlocationUtf16);

.. but not in these three. I think you should call ignore_value(VIR_STRDUP())
in all three. Or fix it properly.

> @@ -2780,7 +2773,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) {
>                  medium->vtbl->GetLocation(medium, &mediumLocUtf16);
>                  VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
>                  VBOX_UTF16_FREE(mediumLocUtf16);
> -                def->disks[diskCount]->src = strdup(mediumLocUtf8);
> +                ignore_value(VIR_STRDUP(def->disks[diskCount]->src, mediumLocUtf8));

VIR_STRDUP_QUIET, or remove the virReportOOMError a few lines below.

>                  VBOX_UTF8_FREE(mediumLocUtf8);
>  
>                  if (!(def->disks[diskCount]->src)) {

> @@ -3120,8 +3111,9 @@ sharedFoldersCleanup:
>                                  def->disks[def->ndisks - 1]->bus = VIR_DOMAIN_DISK_BUS_IDE;
>                                  def->disks[def->ndisks - 1]->type = VIR_DOMAIN_DISK_TYPE_FILE;
>                                  def->disks[def->ndisks - 1]->readonly = true;
> -                                def->disks[def->ndisks - 1]->src = strdup(location);
> -                                def->disks[def->ndisks - 1]->dst = strdup("hdc");
> +                                if (VIR_STRDUP(def->disks[def->ndisks - 1]->src, location) < 0 ||
> +                                    VIR_STRDUP(def->disks[def->ndisks - 1]->dst, "hdc") < 0)
> +                                    def->ndisks--;
>                              } else {
>                                  def->ndisks--;
>                                  virReportOOMError();
> @@ -3167,8 +3159,9 @@ sharedFoldersCleanup:
>                                      def->disks[def->ndisks - 1]->bus = VIR_DOMAIN_DISK_BUS_FDC;
>                                      def->disks[def->ndisks - 1]->type = VIR_DOMAIN_DISK_TYPE_FILE;
>                                      def->disks[def->ndisks - 1]->readonly = false;
> -                                    def->disks[def->ndisks - 1]->src = strdup(location);
> -                                    def->disks[def->ndisks - 1]->dst = strdup("fda");
> +                                    if (VIR_STRDUP(def->disks[def->ndisks - 1]->src, location) < 0 ||
> +                                        VIR_STRDUP(def->disks[def->ndisks - 1]->dst, "fda") < 0)
> +                                        def->ndisks--;
>                                  } else {
>                                      def->ndisks--;
>                                      virReportOOMError();

ignore_value(VIR_STRDUP) in both hunks.

> @@ -6247,12 +6225,12 @@ vboxDomainSnapshotListNames(virDomainPtr dom,
>          }
>          VBOX_UTF16_TO_UTF8(nameUtf16, &name);
>          VBOX_UTF16_FREE(nameUtf16);
> -        names[i] = strdup(name);
> -        VBOX_UTF8_FREE(name);
> -        if (!names[i]) {
> +        if (VIR_STRDUP(names[i], name) < 0) {
>              virReportOOMError();

redundant virReportOOMError()

> +            VBOX_UTF8_FREE(name);
>              goto cleanup;
>          }
> +        VBOX_UTF8_FREE(name);
>      }
>  
>      if (count <= nameslen)

> @@ -8117,8 +8087,7 @@ static char *vboxNetworkGetXMLDesc(virNetworkPtr network,
>          networkInterface->vtbl->GetInterfaceType(networkInterface, &interfaceType);
>  
>          if (interfaceType == HostNetworkInterfaceType_HostOnly) {
> -            def->name = strdup(network->name);
> -            if (def->name != NULL) {
> +            if (VIR_STRDUP(def->name, network->name) == 0) {
>                  PRUnichar *networkNameUtf16 = NULL;
>                  IDHCPServer *dhcpServer     = NULL;
>                  vboxIID vboxnet0IID = VBOX_IID_INITIALIZER;

You can delete the else branch with virReportOOMError()

The error handling is horrible in a few places, but that's pre-existing.

ACK

Jan


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]