[libvirt] [PATCH v3] bye to close(), welcome to VIR_(FORCE_)CLOSE()

[Stefan Berger]

On 11/08/2010 05:17 PM, Eric Blake wrote:
>
>> Index: libvirt-acl/src/libvirt.c
>> ===================================================================
>> --- libvirt-acl.orig/src/libvirt.c
>> +++ libvirt-acl/src/libvirt.c
>> @@ -10794,7 +10794,7 @@ virStreamRef(virStreamPtr stream)
>>    *      ... report an error ....
>>    * done:
>>    *   virStreamFree(st);
>> - *   close(fd);
>> + *   VIR_FORCE_CLOSE(fd);
>>    *
>>    * Returns the number of bytes written, which may be less
>>    * than requested.
>> @@ -10884,8 +10884,8 @@ error:
>>    *      ... report an error ....
>>    * done:
>>    *   virStreamFree(st);
>> - *   close(fd);
>> - *
>> + *   if (VIR_CLOSE(fd)<  0)
>> + *       virReportSystemError(errno, "%s", _("failed to close file"));
>>    *
>>    * Returns the number of bytes read, which may be less
>>    * than requested.
>> @@ -10964,7 +10964,7 @@ error:
>>    *   if (virStreamFinish(st)<  0)
>>    *      ...report an error...
>>    *   virStreamFree(st);
>> - *   close(fd);
>> + *   VIR_FORCE_CLOSE(fd);
>>    *
>>    * Returns 0 if all the data was successfully sent. The caller
>>    * should invoke virStreamFinish(st) to flush the stream upon
> These first three are okay.
>
>> @@ -11061,7 +11061,7 @@ cleanup:
>>    *   if (virStreamFinish(st)<  0)
>>    *      ...report an error...
>>    *   virStreamFree(st);
>> - *   close(fd);
>> + *   VIR_FORCE_CLOSE(fd);
> But the comment for virStreamRecvAll should match the comment for
> virStreamRecv.  (Comments only, so trivial to fix).
>
The comment for virStreamRecvAll is correct in terms of the return 
values. They are different from those of virStreamRecv, if that's what 
you meant.
>> Index: libvirt-acl/src/phyp/phyp_driver.c
>> ===================================================================
>> @@ -764,7 +769,11 @@ phypUUIDTable_Pull(virConnectPtr conn)
>>           }
>>           break;
>>       }
>> -    close(fd);
>> +    if (VIR_CLOSE(fd)<  0) {
>> +        virReportSystemError(errno, _("Could not close %s\n"),
>> +                             local_file);
>> +        goto err;
>> +    }
>>       goto exit;
>>
>>     exit:
> Looks funny to goto the label on the next line, but no harm in leaving
> it (at any rate, unrelated to your actual patch).
>
>> Index: libvirt-acl/src/xen/proxy_internal.c
>> ===================================================================
>> --- libvirt-acl.orig/src/xen/proxy_internal.c
>> +++ libvirt-acl/src/xen/proxy_internal.c
>> Index: libvirt-acl/proxy/libvirt_proxy.c
>> ===================================================================
>> --- libvirt-acl.orig/proxy/libvirt_proxy.c
>> +++ libvirt-acl/proxy/libvirt_proxy.c
> Merge conflict - you can ignore any changes to these two files, now that
> they is deleted.
> ACK.  You may find some more instances of close() pop up as you rebase
> on top of the latest tree, but it shouldn't be too hard to figure out.
>

This is what I may have missed before or was added recently:

Index: libvirt-acl/tools/virsh.c
===================================================================
--- libvirt-acl.orig/tools/virsh.c
+++ libvirt-acl/tools/virsh.c
@@ -11269,11 +11269,9 @@ static void
  vshCloseLogFile(vshControl *ctl)
  {
      /* log file close */
-    if (ctl->log_fd >= 0) {
-        if (close(ctl->log_fd) < 0)
-            vshError(ctl, _("%s: failed to write log file: %s"),
-                     ctl->logfile ? ctl->logfile : "?", strerror (errno));
-        ctl->log_fd = -1;
+    if (VIR_CLOSE(ctl->log_fd) < 0)
+        vshError(ctl, _("%s: failed to write log file: %s"),
+                 ctl->logfile ? ctl->logfile : "?", strerror (errno));
      }

      if (ctl->logfile) {

I haven't pushed it yet, but would do so tomorrow, unless there are 
concerns.

    Stefan