Re: [libvirt] [PATCH] deprecate fclose() and introduce VIR_{FORCE_}FCLOSE()
On 11/12/2010 11:58 AM, Eric Blake wrote:
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(a)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 :)
I can rename that.
> @@ -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);
Will fix it.
> +++ 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),
:-( In that
case, what about wrapping fdopen with FDOPEN and set fd to
-1 if the function succeeds, i.e., returns != NULL?
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.
OK.
> @@ -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?
Ok, can change that also.
> 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>
Will do.
Stefan