[libvirt] [PATCH] deprecate fclose() and introduce VIR_{FORCE_}FCLOSE()

[Eric Blake]

On 11/12/2010 09:38 AM, Stefan Berger wrote:
> Similarly to deprecating close(), I am now deprecating fclose() and
> introduce VIR_FORCE_FCLOSE() and VIR_FCLOSE().
> 
> Most of the files are opened in read-only mode, so usage of
> VIR_FORCE_CLOSE() seemed appropriate. Others that are opened in write
> mode already had the fclose() < 0 check and I converted those to
> VIR_FCLOSE() < 0.
> 
> I did not find occurences of possible double-closed files on the way.
> 
> Signed-off-by: Stefan Berger <stefanb at us.ibm.com>
> 
> +
> +int virFClose(FILE **file, bool preserve_errno)

I would have named it virFclose, but it doesn't matter (the _only_ place
that cares is the macro definition in files.h :)

> @@ -909,7 +909,7 @@ openvzSetDefinedUUID(int vpsid, unsigned
>          /* Record failure if fprintf or fclose fails,
>             and be careful always to close the stream.  */
>          if ((fprintf(fp, "\n#UUID: %s\n", uuidstr) < 0)
> -            + (fclose(fp) == EOF))
> +            + (VIR_FCLOSE(fp) == EOF))
>              goto cleanup;

Ouch!  This is a pre-existing bug.  C does NOT guarantee order of
operations across + (unlike Java).  Therefore, you MUST split this into
a sequence point, in order to avoid risking fprintf(NULL) because
close(fp) (pre-patch, or VIR_FCLOSE post-patch) got scheduled first by
the compiler.

Thankfully, || is a sequence point, and the only other trick is to
realize that it is safe to blindly call VIR_FORCE_FCLOSE(fp) in the
cleanup: label if the fprintf failed, and harmless if the VIR_FCLOSE was
reached:

if ((fprintf(fp, "\n#UUID: %s\n", uuidstr) < 0) ||
    (VIR_FCLOSE(fp) == EOF))
    goto cleanup;
...
cleanup:
VIR_FORCE_FCLOSE(fp);

> +++ libvirt-acl/src/storage/storage_backend.c
> @@ -1458,10 +1458,8 @@ virStorageBackendRunProgRegex(virStorage
>  
>      VIR_FREE(reg);
>  
> -    if (list)
> -        fclose(list);
> -    else
> -        VIR_FORCE_CLOSE(fd);
> +    VIR_FORCE_FCLOSE(list);
> +    VIR_FORCE_CLOSE(fd);

You just introduced a double close.  list was created via fdopen(fd),
which effectively consumes fd.  Maybe we need to add VIR_FDOPEN which
auto-sets an fdopen'd fd to -1 on success?  Maybe not, but then you need
to adjust the fdopen() call site to invalidate fd so we don't re-close it.

> +++ libvirt-acl/src/storage/storage_backend_iscsi.c
> @@ -235,11 +235,8 @@ out:
>      }
>  
>      VIR_FREE(line);
> -    if (fp != NULL) {
> -        fclose(fp);
> -    } else {
> -        VIR_FORCE_CLOSE(fd);
> -    }
> +    VIR_FORCE_FCLOSE(fp);
> +    VIR_FORCE_CLOSE(fd);

Same double-close bug.

> @@ -219,7 +220,7 @@ xenUnifiedProbe (void)
>      FILE *fh;
>  
>      if (fh = fopen("/dev/xen/domcaps", "r")) {
> -        fclose(fh);
> +        VIR_FORCE_FCLOSE(fh);

This is wasteful.  If we aren't going to use the FILE*, why not go with
the much-faster open("/dev/xen/domcaps", O_RDONLY), VIR_FORCE_CLOSE
sequence, to avoid malloc() overhead in stdio?

> Index: libvirt-acl/docs/hacking.html.in
> ===================================================================
> --- libvirt-acl.orig/docs/hacking.html.in
> +++ libvirt-acl/docs/hacking.html.in
> @@ -392,9 +392,10 @@
>      <h2><a name="file_handling">File handling</a></h2>
>  
>      <p>
> -      Use of the close() API is deprecated in libvirt code base to help
> -      avoiding double-closing of a file descriptor. Instead of this API,
> -      use the macro from files.h
> +      Usage of the close() and fclose() APIs is deprecated in libvirt code base to help

How about we mark these up: <code>close()</code> and <code>fclose()</code>

-- 
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/20101112/c47260a1/attachment-0001.sig>