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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org