8:21 p.m.
libvir-list-bounces(a)redhat.com wrote on 10/22/2010 06:27:30 PM:
libvir-list
On 10/22/2010 05:19 AM, Stefan Berger wrote:
> Using automated replacement with sed and editing I have now replaced
all
> occurrences of close() with VIR_(FORCE_)CLOSE() except for one,
of
> course. Some replacements were straight forward, others I needed to
pay
> attention. I hope I payed attention in all the right places...
Please
> have a look. This should have at least solved one more double-close
> error.
Can you isolate any of those double-close errors into separate patches
which we can apply now, rather than drowning them in the giant patch?
>
> Signed-off-by: Stefan Berger<stefanb(a)us.ibm.com>
>
> src/phyp/phyp_driver.c | 13 ++--
Resuming...
[...]
> Index: libvirt-acl/src/qemu/qemu_monitor.c
>
> @@ -694,8 +695,7 @@ void qemuMonitorClose(qemuMonitorPtr mon
> if (!mon->closed) {
> if (mon->watch)
> virEventRemoveHandle(mon->watch);
> - if (mon->fd != -1)
> - close(mon->fd);
> + VIR_FORCE_CLOSE(mon->fd);
> /* NB: ordinarily one might immediately set mon->watch to -1
> * and mon->fd to -1, but there may be a callback active
> * that is still relying on these fields being valid. So
Ouch - given that comment, could we be frying a callback by setting
mon->fd to -1 in VIR_FORCE_CLOSE? We need to double check this, and
possibly use a temporary variable if the callback indeed needs a
non-negative mon->fd for a bit longer, or tighten the specification and
all existing callbacks to tolerate mon->fd changing to -1.
Probably worth splitting this particular hunk into its own commit,
rather than part of the giant patch.
Yes, good idea. Obviously I did not read the comment.
> Index: libvirt-acl/src/remote/remote_driver.c
> ===================================================================
> --- libvirt-acl.orig/src/remote/remote_driver.c
> +++ libvirt-acl/src/remote/remote_driver.c
> @@ -82,6 +82,7 @@
> #include "util.h"
> #include "event.h"
> #include "ignore-value.h"
> +#include "files.h"
>
> #define VIR_FROM_THIS VIR_FROM_REMOTE
>
> @@ -711,7 +712,7 @@ doRemoteOpen (virConnectPtr conn,
> if (errno == ECONNREFUSED&&
> flags& VIR_DRV_OPEN_REMOTE_AUTOSTART&&
> trials< 20) {
> - close(priv->sock);
> + VIR_FORCE_CLOSE(priv->sock);
> priv->sock = -1;
This line is now redundant.
Done.
> Index: libvirt-acl/src/uml/uml_driver.c
> @@ -811,7 +811,7 @@ static int umlStartVMDaemon(virConnectPt
> virDomainObjPtr vm) {
> const char **argv = NULL, **tmp;
> const char **progenv = NULL;
> - int i, ret;
> + int i, ret, tmpfd;
> pid_t pid;
> char *logfile;
> int logfd = -1;
> for (i = 0; i< FD_SETSIZE; i++)
> - if (FD_ISSET(i,&keepfd))
> - close(i);
> + if (FD_ISSET(i,&keepfd)) {
> + tmpfd = i;
> + VIR_FORCE_CLOSE(tmpfd);
Another awfully large scope for a needed temporary.
Ok.
> Index: libvirt-acl/src/util/bridge.c
> @@ -107,7 +108,7 @@ brShutdown(brControl *ctl)
> if (!ctl)
> return;
>
> - close(ctl->fd);
> + VIR_FORCE_CLOSE(ctl->fd);
> ctl->fd = 0;
Huh - is this an existing logic bug? Can we end up accidentally
double-closing stdin?
I'ld leave the patch for now as it is, i.e., do the VIR_FORCE_CLOSE and
remember to investigate. So far, the changed does not have any further
negative impact that already isn't there - but it also doesn't solve a
potential problem.
> Index: libvirt-acl/src/util/logging.c
> ===================================================================
> --- libvirt-acl.orig/src/util/logging.c
> +++ libvirt-acl/src/util/logging.c
> @@ -40,6 +40,7 @@
> #include "util.h"
> #include "buf.h"
> #include "threads.h"
> +#include "files.h"
>
> /*
> * Macro used to format the message as a string in virLogMessage
> @@ -603,8 +604,7 @@ static int virLogOutputToFd(const char *
> static void virLogCloseFd(void *data) {
> int fd = (long) data;
>
> - if (fd>= 0)
> - close(fd);
> + VIR_FORCE_CLOSE(fd);
Should we fix this function to return an int value, and return
VIR_CLOSE(fd) so that callers can choose to detect log close failures?
Also that I would delay until further 'cause this may have consequences
for those calling the function.
> Index: libvirt-acl/src/util/macvtap.c
> ===================================================================
> --- libvirt-acl.orig/src/util/macvtap.c
> +++ libvirt-acl/src/util/macvtap.c
> @@ -52,6 +52,7 @@
> # include "conf/domain_conf.h"
> # include "virterror_internal.h"
> # include "uuid.h"
> +# include "files.h"
>
> # define VIR_FROM_THIS VIR_FROM_NET
>
> @@ -94,7 +95,7 @@ static int nlOpen(void)
>
> static void nlClose(int fd)
> {
> - close(fd);
> + VIR_FORCE_CLOSE(fd);
Likewise?
This here is a close of a netlink socket, which was used to communicate
with the kernel. I would just close it and discard the returned value.
> Index: libvirt-acl/src/util/util.c
> ===================================================================
> --- libvirt-acl.orig/src/util/util.c
> +++ libvirt-acl/src/util/util.c
> @@ -71,6 +71,7 @@
> #include "memory.h"
> #include "threads.h"
> #include "verify.h"
> +#include "files.h"
>
> #ifndef NSIG
> # define NSIG 32
> @@ -461,6 +462,7 @@ __virExec(const char *const*argv,
> int pipeerr[2] = {-1,-1};
> int childout = -1;
> int childerr = -1;
> + int tmpfd;
> @@ -568,8 +570,10 @@ __virExec(const char *const*argv,
> i != childout&&
> i != childerr&&
> (!keepfd ||
> - !FD_ISSET(i, keepfd)))
> - close(i);
> + !FD_ISSET(i, keepfd))) {
> + tmpfd = i;
> + VIR_FORCE_CLOSE(tmpfd);
I thought this was another large scope for a temporary....
> + }
>
> if (dup2(infd>= 0 ? infd : null, STDIN_FILENO)< 0) {
> virReportSystemError(errno,
> @@ -589,14 +593,15 @@ __virExec(const char *const*argv,
> goto fork_error;
> }
>
> - if (infd> 0)
> - close(infd);
> - close(null);
> - if (childout> 0)
> - close(childout);
> + VIR_FORCE_CLOSE(infd);
> + VIR_FORCE_CLOSE(null);
> + tmpfd = childout; /* preserve childout value */
> + VIR_FORCE_CLOSE(tmpfd);
...but you needed it here too.
Yes, here I need it twice. So not changing it.
> if (childerr> 0&&
> - childerr != childout)
> - close(childerr);
> + childerr != childout) {
> + VIR_FORCE_CLOSE(childerr);
> + childout = -1;
> + }
Looks okay after all - certainly not one of the trivial conversions
though :)
> Index: libvirt-acl/src/util/virtaudit.c
> ===================================================================
> --- libvirt-acl.orig/src/util/virtaudit.c
> +++ libvirt-acl/src/util/virtaudit.c
> @@ -30,6 +30,7 @@
> #include "virterror_internal.h"
> #include "logging.h"
> #include "virtaudit.h"
> +#include "files.h"
>
> /* Provide the macros in case the header file is old.
> FIXME: should be removed. */
> @@ -133,6 +134,6 @@ void virAuditSend(const char *file ATTRI
> void virAuditClose(void)
> {
> #if HAVE_AUDIT
> - close(auditfd);
> + VIR_CLOSE(auditfd);
Must be VIR_FORCE_CLOSE; I'm guessing you didn't have audit turned on in
your compile.
Right...
> Index: libvirt-acl/src/xen/proxy_internal.c
> @@ -236,12 +237,11 @@ virProxyCloseSocket(xenUnifiedPrivatePtr
> if (priv->proxy< 0)
> return(-1);
>
> - ret = close(priv->proxy);
> + ret = VIR_CLOSE(priv->proxy);
> if (ret != 0)
> VIR_WARN("Failed to close socket %d", priv->proxy);
> else
> VIR_DEBUG("Closed socket %d", priv->proxy);
Subtle unintended semantic change; you're now printing -1 instead of the
fd you just closed; you'll need a temporary.
There was another one like this that I got right :)
> Index: libvirt-acl/src/xen/xend_internal.c
> @@ -137,9 +137,7 @@ do_connect(virConnectPtr xend)
>
>
> if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) ==
-1) {
> - serrno = errno;
> - close(s);
> - errno = serrno;
> + VIR_FORCE_CLOSE(s);
> s = -1;
Redundant line. Overall, I'm impressed with how many lines this is
shaving off the code base! I think we settled on a pretty good calling
convention.
Fixed. Will post v2 and if useful a diff(v1,v2).
Stefan
And with that, I've completed my review of v1.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list