Re: [libvirt] [PATCH v2] deprecate fclose() and introduce VIR_{FORCE_}FCLOSE()
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)