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

[Eric Blake]

On 11/13/2010 05:39 PM, Stefan Berger wrote:
> V2:
>   - following Eric's suggestion, deprecating fdclose(), introducing VIR_FDCLOSE()
>   - following some other nits that Eric pointed out
>   - replaced some more fclose () I previously had missed (last 2 files in patch)
> 
> Similarly to deprecating close(), I am now deprecating fclose() and
> introduce VIR_FORCE_FCLOSE() and VIR_FCLOSE(). Also, fdopen() is replaced with 
> VIR_FDOPEN().
> 
> +FILE *virFdopen(int *fdptr, const char *mode)
> +{
> +    FILE *file = NULL;
> +
> +    if (*fdptr >= 0) {
> +        file = fdopen(*fdptr, mode);
> +        if (file)
> +            *fdptr = -1;
> +    }

else {
    errno = EBADF;
}

> +
> +    return file;
> +}

[That is, we should guarantee errno is valid if file is NULL on exit,
even in the case when *fdptr was -1 on entry]

>  
>  /* Don't call this directly - use the macros below */

s/this/these/

>  int virClose(int *fdptr, bool preserve_errno) ATTRIBUTE_RETURN_CHECK;
> +int virFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK;
> +FILE *virFdopen(int *fdptr, const char *mode);

add ATTRIBUTE_RETURN_CHECK

>  
>  /* For use on normal paths; caller must check return value,
>     and failure sets errno per close(). */
>  # define VIR_CLOSE(FD) virClose(&(FD), false)
> +# define VIR_FCLOSE(FILE) virFclose(&(FILE), false)
> +# define VIR_FDOPEN(FD, MODE) virFdopen(&(FD), MODE)

The comment is a bit misleading for VIR_FDOPEN; how about adding a new
comment:

/* Wrapper around fdopen that consumes fd on success.  */
# define VIR_FDOPEN...

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

Leaks the FILE and fd for fp if the fprintf fails.  We need to float the
declaration of FILE *fp to the front of the method, and add
VIR_FORCE_FCLOSE(fp) in the cleanup label.

> @@ -216,10 +217,10 @@ xenUnifiedProbe (void)
>          return 1;
>  #endif
>  #ifdef __sun
> -    FILE *fh;
> +    int fd;
>  
> -    if (fh = fopen("/dev/xen/domcaps", "r")) {
> -        fclose(fh);
> +    if (fd = open("/dev/xen/domcaps", O_RDONLY)) {

if ((fd = open(...)) >= 0)

[you can't guarantee that the open will land on stdin]

> -    <ul>
> +   <ul>
> +      <li><p>eg opening a file from a file descriptor</p>

The correct spelling is 'e.g. opening ...'; but using e.g. at the start
of every <li> in this list seems redundant.

But that can be a follow-on patch to clean it up (in fact, I've already
got such a patch in the wings for another <ul> in the same document), so
don't worry about it.

Hmm; I found enough issues that it's probably worth a v3 (or at least an
incremental diff).

-- 
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/20101115/92d7fd5d/attachment-0001.sig>