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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list