[libvirt] [PATCH v3] bye to close(), welcome to VIR_(FORCE_)CLOSE()

[Eric Blake]

On 11/01/2010 05:16 AM, Stefan Berger wrote:
> Now that 0.8.5 is out, here is another posting of this big cleanup patch.

And I'm finally getting time to look at it.

We're early enough in the release cycle that even if I missed something
in this review, hopefully we get enough test exposure before the actual
release, so I'm pretty comfortable with pushing it.

> There were some problems with Eric's suggested cfg.mk extension (showing contents of error messages with 'close' as text), so I could not add the 'sc_avoid_close' to it.

I'll have to look into that, then; but it can be a separate patch so as
to not hold up this one.

> 
> I again put the diff of tree+v3 against tree+v1 to the end of the patch. I forward-ported V1 for that.

That was helpful; thanks.

> Index: libvirt-acl/src/libvirt.c
> ===================================================================
> --- libvirt-acl.orig/src/libvirt.c
> +++ libvirt-acl/src/libvirt.c
> @@ -10794,7 +10794,7 @@ virStreamRef(virStreamPtr stream)
>   *      ... report an error ....
>   * done:
>   *   virStreamFree(st);
> - *   close(fd);
> + *   VIR_FORCE_CLOSE(fd);
>   *
>   * Returns the number of bytes written, which may be less
>   * than requested.
> @@ -10884,8 +10884,8 @@ error:
>   *      ... report an error ....
>   * done:
>   *   virStreamFree(st);
> - *   close(fd);
> - *
> + *   if (VIR_CLOSE(fd) < 0)
> + *       virReportSystemError(errno, "%s", _("failed to close file"));
>   *
>   * Returns the number of bytes read, which may be less
>   * than requested.
> @@ -10964,7 +10964,7 @@ error:
>   *   if (virStreamFinish(st) < 0)
>   *      ...report an error...
>   *   virStreamFree(st);
> - *   close(fd);
> + *   VIR_FORCE_CLOSE(fd);
>   *
>   * Returns 0 if all the data was successfully sent. The caller
>   * should invoke virStreamFinish(st) to flush the stream upon

These first three are okay.

> @@ -11061,7 +11061,7 @@ cleanup:
>   *   if (virStreamFinish(st) < 0)
>   *      ...report an error...
>   *   virStreamFree(st);
> - *   close(fd);
> + *   VIR_FORCE_CLOSE(fd);

But the comment for virStreamRecvAll should match the comment for
virStreamRecv.  (Comments only, so trivial to fix).

> Index: libvirt-acl/src/phyp/phyp_driver.c
> ===================================================================
> @@ -764,7 +769,11 @@ phypUUIDTable_Pull(virConnectPtr conn)
>          }
>          break;
>      }
> -    close(fd);
> +    if (VIR_CLOSE(fd) < 0) {
> +        virReportSystemError(errno, _("Could not close %s\n"),
> +                             local_file);
> +        goto err;
> +    }
>      goto exit;
>  
>    exit:

Looks funny to goto the label on the next line, but no harm in leaving
it (at any rate, unrelated to your actual patch).

> Index: libvirt-acl/src/xen/proxy_internal.c
> ===================================================================
> --- libvirt-acl.orig/src/xen/proxy_internal.c
> +++ libvirt-acl/src/xen/proxy_internal.c

> Index: libvirt-acl/proxy/libvirt_proxy.c
> ===================================================================
> --- libvirt-acl.orig/proxy/libvirt_proxy.c
> +++ libvirt-acl/proxy/libvirt_proxy.c

Merge conflict - you can ignore any changes to these two files, now that
they is deleted.

ACK.  You may find some more instances of close() pop up as you rebase
on top of the latest tree, but it shouldn't be too hard to figure out.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101108/2e812aa2/attachment-0001.sig>