v2:
- added comments about the monitor lock being used to protect the
file descriptor 'fd'
- eofcb is not set anywhere, so also removing it; the if statement
checking on it seems dead code
I am replacing the last instances of close() I found with VIR_CLOSE() /
VIR_FORCE_CLOSE respectively.
The first part patches virsh, which I missed out on previously.
The 2nd patch I had left out intentionally to look at it more carefully:
The 'closed' variable could be easily removed since it wasn't used
anywhere else. The possible race condition that could result from the
filedescriptor being closed and not set to -1 (and possibly let us write
into 'something' totally different if the fd was allocated by another
thread) seems to be prevented by the qemuMonitorLock() already placed
around the code that reads from or writes to the fd. So the change of
this code as shown in the patch should not have any side-effects.
Signed-off-by: Stefan Berger<stefanb(a)us.ibm.com>
---
src/qemu/qemu_monitor.c | 32 +++++++++++---------------------
tools/virsh.c | 4 ++--
2 files changed, 13 insertions(+), 23 deletions(-)
Index: libvirt-acl/tools/virsh.c
===================================================================
--- libvirt-acl.orig/tools/virsh.c
+++ libvirt-acl/tools/virsh.c
@@ -8994,12 +8994,12 @@ editWriteToTempFile (vshControl *ctl, co
if (safewrite (fd, doc, strlen (doc)) == -1) {
vshError(ctl, _("write: %s: failed to write to temporary file: %s"),
ret, strerror(errno));
- close (fd);
+ VIR_FORCE_CLOSE(fd);
unlink (ret);
VIR_FREE(ret);
return NULL;
}
- if (close (fd) == -1) {
+ if (VIR_CLOSE(fd)< 0) {
vshError(ctl, _("close: %s: failed to write or close temporary file:
%s"),
ret, strerror(errno));
unlink (ret);
Index: libvirt-acl/src/qemu/qemu_monitor.c
===================================================================
--- libvirt-acl.orig/src/qemu/qemu_monitor.c
+++ libvirt-acl/src/qemu/qemu_monitor.c
@@ -44,7 +44,7 @@
#define DEBUG_RAW_IO 0
struct _qemuMonitor {
- virMutex lock;
+ virMutex lock; /* also used to protect fd */
virCond notify;
int refs;
@@ -71,11 +71,6 @@ struct _qemuMonitor {
* the next monitor msg */
int lastErrno;
- /* If the monitor EOF callback is currently active (stops more commands being run)
*/
- unsigned eofcb: 1;
- /* If the monitor is in process of shutting down */
- unsigned closed: 1;
-
unsigned json: 1;
};
@@ -356,6 +351,7 @@ qemuMonitorIOProcess(qemuMonitorPtr mon)
}
+/* Call this function while holding the monitor lock. */
static int
qemuMonitorIOWriteWithFD(qemuMonitorPtr mon,
const char *data,
@@ -400,7 +396,10 @@ qemuMonitorIOWriteWithFD(qemuMonitorPtr
return ret;
}
-/* Called when the monitor is able to write data */
+/*
+ * Called when the monitor is able to write data
+ * Call this function while holding the monitor lock.
+ */
static int
qemuMonitorIOWrite(qemuMonitorPtr mon)
{
@@ -433,6 +432,7 @@ qemuMonitorIOWrite(qemuMonitorPtr mon)
/*
* Called when the monitor has incoming data to read
+ * Call this function while holding the monitor lock.
*
* Returns -1 on error, or number of bytes read
*/
@@ -505,6 +505,7 @@ qemuMonitorIO(int watch, int fd, int eve
qemuMonitorPtr mon = opaque;
int quit = 0, failed = 0;
+ /* lock access to the monitor and protect fd */
qemuMonitorLock(mon);
qemuMonitorRef(mon);
#if DEBUG_IO
@@ -692,17 +693,11 @@ void qemuMonitorClose(qemuMonitorPtr mon
VIR_DEBUG("mon=%p", mon);
qemuMonitorLock(mon);
- if (!mon->closed) {
+
+ if (mon->fd>= 0) {
if (mon->watch)
virEventRemoveHandle(mon->watch);
- if (mon->fd != -1)
- 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
- * we merely close them, but not clear their values and
- * use this explicit 'closed' flag to track this state */
- mon->closed = 1;
+ VIR_FORCE_CLOSE(mon->fd);
}
if (qemuMonitorUnref(mon)> 0)
@@ -715,11 +710,6 @@ int qemuMonitorSend(qemuMonitorPtr mon,
{
int ret = -1;
- if (mon->eofcb) {
- msg->lastErrno = EIO;
- return -1;
- }
-
mon->msg = msg;
qemuMonitorUpdateWatch(mon);
Show replies by date
On Tue, Nov 16, 2010 at 09:58:21PM -0500, Stefan Berger wrote:
v2:
- added comments about the monitor lock being used to protect the
file descriptor 'fd'
- eofcb is not set anywhere, so also removing it; the if statement
checking on it seems dead code
I am replacing the last instances of close() I found with VIR_CLOSE() /
VIR_FORCE_CLOSE respectively.
The first part patches virsh, which I missed out on previously.
The 2nd patch I had left out intentionally to look at it more carefully:
The 'closed' variable could be easily removed since it wasn't used
anywhere else. The possible race condition that could result from the
filedescriptor being closed and not set to -1 (and possibly let us write
into 'something' totally different if the fd was allocated by another
thread) seems to be prevented by the qemuMonitorLock() already placed
around the code that reads from or writes to the fd. So the change of
this code as shown in the patch should not have any side-effects.
Signed-off-by: Stefan Berger<stefanb(a)us.ibm.com>
ACK
Daniel
--
|: Red Hat, Engineering, London -o-
http://people.redhat.com/berrange/ :|
|:
http://libvirt.org -o-
http://virt-manager.org -o-
http://deltacloud.org :|
|:
http://autobuild.org -o-
http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 11/17/2010 09:36 AM, Daniel P. Berrange wrote:
On Tue, Nov 16, 2010 at 09:58:21PM -0500, Stefan Berger wrote:
> v2:
> - added comments about the monitor lock being used to protect the
> file descriptor 'fd'
> - eofcb is not set anywhere, so also removing it; the if statement
> checking on it seems dead code
>
> I am replacing the last instances of close() I found with VIR_CLOSE() /
> VIR_FORCE_CLOSE respectively.
>
> The first part patches virsh, which I missed out on previously.
>
> The 2nd patch I had left out intentionally to look at it more carefully:
> The 'closed' variable could be easily removed since it wasn't used
> anywhere else. The possible race condition that could result from the
> filedescriptor being closed and not set to -1 (and possibly let us write
> into 'something' totally different if the fd was allocated by another
> thread) seems to be prevented by the qemuMonitorLock() already placed
> around the code that reads from or writes to the fd. So the change of
> this code as shown in the patch should not have any side-effects.
>
> Signed-off-by: Stefan Berger<stefanb(a)us.ibm.com>
ACK
Daniel
Pushed.
Stefan