Re: [Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate()
On Mon, Jun 24, 2013 at 05:17:35PM +0200, Kevin Wolf wrote:
Am 21.06.2013 um 18:39 hat mdroth geschrieben:
On Wed, Jun 19, 2013 at 06:28:05PM +0200, Kevin Wolf wrote:
Don't duplicate more code than is really necessary.
Signed-off-by: Kevin Wolf kw...@redhat.com
---
scripts/qapi.py | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 02ad668..3a64769 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -76,12 +76,18 @@ def parse(tokens):
return tokens[0], tokens[1:]
def evaluate(string):
-return parse(map(lambda x: x, tokenize(string)))[0]
+expr_eval = parse(map(lambda x: x, tokenize(string)))[0]
+
+if expr_eval.has_key('enum'):
+add_enum(expr_eval['enum'])
+elif expr_eval.has_key('union'):
+add_enum('%sKind' % expr_eval['union'])
+
+return expr_eval
add_enum() adds stuff to a global table, so I don't really like the idea
of pushing it further down the stack (however inconsequential it may be
in this case...)
As if it made any difference if it's one level more or less... It's
already buried in the library.
I think the real reason we have repetition is the extra 'flush' we do
after the for loop below to handle the last expression we read from a
schema, which leads to a repeat of one of the clauses in the loop body.
I've wanted to get rid of it a few times in the past so this is probably
a good opportunity to do so.
Could you try adapting something like the following to keep the
global stuff in parse_schema()?
I don't think there's any value in keeping the global stuff in
parse_schema() (or, to be precise, in functions directly called by
parse_schema()) , and I found it quite nice to have evaluate()
actually evaluate something instead of just parsing it.
But I agree that duplicating code, as small as it may be now, isn't
nice, so I can try to use the get_expr() thing. I just would prefer to
move the evaluation to evaluate() anyway.
evaluate() is basically the qapi version of python's eval(), but with
specially handling for JSON objects that retains the original ordering
of the keys by mapping JSON strings to OrderedDicts instead of dicts. If
it weren't for that requirement we'd be call eval() in place of
evaluate() and drop all our awesome parsing/tokenizing code.
It's still not a huge deal, but given an alternative that's also
beneficial in other ways I think we should avoid it.
Kevin
Re: [Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate()
On Wed, Jun 19, 2013 at 06:28:05PM +0200, Kevin Wolf wrote:
Don't duplicate more code than is really necessary.
Signed-off-by: Kevin Wolf kw...@redhat.com
---
scripts/qapi.py | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 02ad668..3a64769 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -76,12 +76,18 @@ def parse(tokens):
return tokens[0], tokens[1:]
def evaluate(string):
-return parse(map(lambda x: x, tokenize(string)))[0]
+expr_eval = parse(map(lambda x: x, tokenize(string)))[0]
+
+if expr_eval.has_key('enum'):
+add_enum(expr_eval['enum'])
+elif expr_eval.has_key('union'):
+add_enum('%sKind' % expr_eval['union'])
+
+return expr_eval
add_enum() adds stuff to a global table, so I don't really like the idea
of pushing it further down the stack (however inconsequential it may be
in this case...)
I think the real reason we have repetition is the extra 'flush' we do
after the for loop below to handle the last expression we read from a
schema, which leads to a repeat of one of the clauses in the loop body.
I've wanted to get rid of it a few times in the past so this is probably
a good opportunity to do so.
Could you try adapting something like the following to keep the
global stuff in parse_schema()?
def get_expr(fp):
expr =
for line in fp:
if line.startswith('#') or line == '\n':
continue
if line.startswith(' '):
expr += line
elif expr:
yield expr
expr = line
else:
expr += line
yield expr
def parse_schema(fp):
exprs = []
expr_eval
for expr in get_expr(fp):
expr_eval = evaluate(expr)
if expr_eval.has_key('enum'):
add_enum(expr_eval['enum'])
...
return exprs
def parse_schema(fp):
exprs = []
expr = ''
-expr_eval = None
for line in fp:
if line.startswith('#') or line == '\n':
@@ -90,23 +96,13 @@ def parse_schema(fp):
if line.startswith(' '):
expr += line
elif expr:
-expr_eval = evaluate(expr)
-if expr_eval.has_key('enum'):
-add_enum(expr_eval['enum'])
-elif expr_eval.has_key('union'):
-add_enum('%sKind' % expr_eval['union'])
-exprs.append(expr_eval)
+exprs.append(evaluate(expr))
expr = line
else:
expr += line
if expr:
-expr_eval = evaluate(expr)
-if expr_eval.has_key('enum'):
-add_enum(expr_eval['enum'])
-elif expr_eval.has_key('union'):
-add_enum('%sKind' % expr_eval['union'])
-exprs.append(expr_eval)
+exprs.append(evaluate(expr))
return exprs
--
1.8.1.4
Re: [Qemu-devel] qemu-ga behavior on virtio-serial unplug
On Wed, Jun 19, 2013 at 01:17:57PM +0200, Laszlo Ersek wrote:
Hello Michael,
this is with reference to
https://bugzilla.redhat.com/show_bug.cgi?id=907733.
Ever since the initial qemu-ga commit AFAICS an exception for
virtio-serial has existed, when reading EOF from the channel.
For isa-serial, EOF results in the client connection being closed. I
assume this exits the glib main loop somehow (otherwise qemu-ga would
just sit there and do nothing, as no further connections are accepted I
think).
I think it would actually do the latter, unfortunately. It's distinct
from virtio-serial handling in that we remove the GSource by return false
in the event handler (qga/main.c:channel_event_cb), but I think we'd
need to drop in a g_main_loop_quit() to actually get qemu-ga to exit in
that scenario.
This doesn't normally get triggered though, as isa-serial does not send
EOF when nothing is connected to the chardev backend, but instead just
blocks. Might till make sense to make qemu-ga exit in this case though
since it won't be doing anything useful and wrapper scripts would at
least have some indication that something is up.
For a unix domain socket, we can continue accepting new connections
after reading EOF.
For virtio-serial, EOF means no host-side process is connected. In
this case we sleep a bit and go back to reading from the same fd (and
this turns into a sleep loop until the next host-side process connects).
Can we tell virtio-serial port unplug from no host-side process?
Because in the former case qemu-ga should really close the channel (and
maybe exit (*)), otherwise the unplug won't complete in the guest kernel.
According to Amit's comments in the RHBZ, at unplug a SIGIO is
delivered, and a POLLHUP event is reported. However,
(a) I think the glib iochannel abstraction doesn't allow us to tell
POLLHUP apart from reading EOF;
AFAICT we can actually access the POLLHUP event via the 'condition' param
that gets passed to the cb, but the issue is we also get POLLUP when
the chardev backend isn't open.
(b) delivery of an unhandled SIGIO seems to terminate the victim
process. qemu-ga doesn't seem to either catch or block SIGIO, which is
why I think a SIGIO signal is not sent in reality (maybe we should ask
for it first?)
... Actually I'm confused about this as well. The virtio-serial port
*is* opened with O_ASYNC (and on Solaris, it is replaced with an
I_SETSIG ioctl()). What's the reason for this? g_io_channel_unix_new()
doesn't seem to list it as a requirement, and qemu-ga doesn't seem to
handle SIGIO.
At some point I played around with trying to use SIGIO to handle channel
resets and whatnot (since we're also supposed to get one when someone
opens the chardev backend and causes VIRTIO_CONSOLE_PORT_OPEN to get
sent). I don't think I ever got it working, SIGIO doesn't seem to get
sent, so that O_ASYNC might just be a relic from that.
I tried installing a handler retested host-connect as well as hot
unplug and still don't seem to be getting the signal. Not sure if i'm
doing something wrong or if there's an issue with the guest driver.
I did notice something interesting though:
1371740628.596505: debug: cb: condition: 17, status: 2
1371740628.596541: debug: received EOF
1371740628.395726: debug: cb: condition: 17, status: 2
1371740628.395760: debug: received EOF
1371740628.496035: debug: cb: condition: 17, status: 2
1371740628.496072: debug: received EOF
1371740628.596505: debug: cb: condition: 17, status: 2
1371740628.596541: debug: received EOF
host opened chardev backend
1371740634.195524: debug: cb: condition: 1, status: 1
1371740634.195556: debug: read data, count: 25, data:
{'execute':'guest-ping'}
1371740634.195634: debug: process_event: called
1371740634.195660: debug: processing command
1371740634.196007: debug: sending data, count: 15
virtio-serial unplugged
1371740644.113346: debug: cb: condition: 16, status: 2
1371740644.113379: debug: received EOF
1371740644.213694: debug: cb: condition: 16, status: 2
1371740644.213725: debug: received EOF
1371740644.314041: debug: cb: condition: 16, status: 2
1371740644.314168: debug: received EOF
i.e. we got the POLLHUP if we read from an
unconnected-but-present port, and we *don't* get the POLLHUP
if the port has been unplugged.
And in none of these cases do the SIGIO seem to be sent.
Here's the debug stuff i added:
diff --git a/qga/main.c b/qga/main.c
index 0e04e73..7f9a628 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -140,6 +140,11 @@ static void quit_handler(int sig)
}
}
+static void sigio_handler(int sig)
+{
+g_debug(got sigio: %d, sig);
+}
+
#ifndef _WIN32
static gboolean register_signal_handlers(void)
{
@@ -158,6 +163,13 @@ static gboolean register_signal_handlers(void)
g_error(error configuring signal handler: %s, strerror(errno));
}
+memset(sigact, 0, sizeof(struct sigaction));
+sigact.sa_handler = sigio_handler;
+ret = sigaction(SIGIO, sigact,
Re: [Qemu-devel] qemu-ga behavior on virtio-serial unplug
On Thu, Jun 20, 2013 at 10:12:30AM -0500, mdroth wrote:
On Wed, Jun 19, 2013 at 01:17:57PM +0200, Laszlo Ersek wrote:
Hello Michael,
this is with reference to
https://bugzilla.redhat.com/show_bug.cgi?id=907733.
Ever since the initial qemu-ga commit AFAICS an exception for
virtio-serial has existed, when reading EOF from the channel.
For isa-serial, EOF results in the client connection being closed. I
assume this exits the glib main loop somehow (otherwise qemu-ga would
just sit there and do nothing, as no further connections are accepted I
think).
I think it would actually do the latter, unfortunately. It's distinct
from virtio-serial handling in that we remove the GSource by return false
in the event handler (qga/main.c:channel_event_cb), but I think we'd
need to drop in a g_main_loop_quit() to actually get qemu-ga to exit in
that scenario.
This doesn't normally get triggered though, as isa-serial does not send
EOF when nothing is connected to the chardev backend, but instead just
blocks. Might till make sense to make qemu-ga exit in this case though
since it won't be doing anything useful and wrapper scripts would at
least have some indication that something is up.
For a unix domain socket, we can continue accepting new connections
after reading EOF.
For virtio-serial, EOF means no host-side process is connected. In
this case we sleep a bit and go back to reading from the same fd (and
this turns into a sleep loop until the next host-side process connects).
Can we tell virtio-serial port unplug from no host-side process?
Because in the former case qemu-ga should really close the channel (and
maybe exit (*)), otherwise the unplug won't complete in the guest kernel.
According to Amit's comments in the RHBZ, at unplug a SIGIO is
delivered, and a POLLHUP event is reported. However,
(a) I think the glib iochannel abstraction doesn't allow us to tell
POLLHUP apart from reading EOF;
AFAICT we can actually access the POLLHUP event via the 'condition' param
that gets passed to the cb, but the issue is we also get POLLUP when
the chardev backend isn't open.
(b) delivery of an unhandled SIGIO seems to terminate the victim
process. qemu-ga doesn't seem to either catch or block SIGIO, which is
why I think a SIGIO signal is not sent in reality (maybe we should ask
for it first?)
... Actually I'm confused about this as well. The virtio-serial port
*is* opened with O_ASYNC (and on Solaris, it is replaced with an
I_SETSIG ioctl()). What's the reason for this? g_io_channel_unix_new()
doesn't seem to list it as a requirement, and qemu-ga doesn't seem to
handle SIGIO.
At some point I played around with trying to use SIGIO to handle channel
resets and whatnot (since we're also supposed to get one when someone
opens the chardev backend and causes VIRTIO_CONSOLE_PORT_OPEN to get
sent). I don't think I ever got it working, SIGIO doesn't seem to get
sent, so that O_ASYNC might just be a relic from that.
I tried installing a handler retested host-connect as well as hot
unplug and still don't seem to be getting the signal. Not sure if i'm
doing something wrong or if there's an issue with the guest driver.
I did notice something interesting though:
1371740628.596505: debug: cb: condition: 17, status: 2
1371740628.596541: debug: received EOF
1371740628.395726: debug: cb: condition: 17, status: 2
1371740628.395760: debug: received EOF
1371740628.496035: debug: cb: condition: 17, status: 2
1371740628.496072: debug: received EOF
1371740628.596505: debug: cb: condition: 17, status: 2
1371740628.596541: debug: received EOF
host opened chardev backend
1371740634.195524: debug: cb: condition: 1, status: 1
1371740634.195556: debug: read data, count: 25, data:
{'execute':'guest-ping'}
1371740634.195634: debug: process_event: called
1371740634.195660: debug: processing command
1371740634.196007: debug: sending data, count: 15
virtio-serial unplugged
1371740644.113346: debug: cb: condition: 16, status: 2
1371740644.113379: debug: received EOF
1371740644.213694: debug: cb: condition: 16, status: 2
1371740644.213725: debug: received EOF
1371740644.314041: debug: cb: condition: 16, status: 2
1371740644.314168: debug: received EOF
i.e. we got the POLLHUP if we read from an
unconnected-but-present port, and we *don't* get the POLLHUP
if the port has been unplugged.
Er...silly me, POLLHUP=16, POLLIN=1 for glib, so I mixed them
up. For unplugged case we get POLLHUP, for unconnected case we get
POLLIN | POLLHUP, so that might actually be enough to distinguish
unplug if this is the intended behavior.
Amit, can you confirm?
And in none of these cases do the SIGIO seem to be sent.
Here's the debug stuff i added:
diff --git a/qga/main.c b/qga/main.c
index 0e04e73..7f9a628 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -140,6 +140,11 @@ static void quit_handler(int sig
Re: [Qemu-devel] [PATCH v2 2/2] QEMUBH: make AioContext's bh re-entrant
On Sun, Jun 16, 2013 at 07:21:21PM +0800, Liu Ping Fan wrote:
BH will be used outside big lock, so introduce lock to protect
between the writers, ie, bh's adders and deleter.
Note that the lock only affects the writers and bh's callback does
not take this extra lock.
Signed-off-by: Liu Ping Fan pingf...@linux.vnet.ibm.com
---
async.c | 21 +
include/block/aio.h | 3 +++
2 files changed, 24 insertions(+)
diff --git a/async.c b/async.c
index 90fe906..6a3269f 100644
--- a/async.c
+++ b/async.c
@@ -47,8 +47,12 @@ QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void
*opaque)
bh-ctx = ctx;
bh-cb = cb;
bh-opaque = opaque;
+qemu_mutex_lock(ctx-bh_lock);
bh-next = ctx-first_bh;
+/* Make sure the memebers ready before putting bh into list */
+smp_wmb();
ctx-first_bh = bh;
+qemu_mutex_unlock(ctx-bh_lock);
return bh;
}
@@ -61,12 +65,18 @@ int aio_bh_poll(AioContext *ctx)
ret = 0;
for (bh = ctx-first_bh; bh; bh = next) {
+/* Make sure fetching bh before accessing its members */
+smp_read_barrier_depends();
next = bh-next;
if (!bh-deleted bh-scheduled) {
bh-scheduled = 0;
if (!bh-idle)
ret = 1;
bh-idle = 0;
+/* Paired with write barrier in bh schedule to ensure reading for
+ * callbacks coming after bh's scheduling.
+ */
+smp_rmb();
bh-cb(bh-opaque);
Could we possibly simplify this by introducing a recursive mutex that we
could use to protect the whole list loop and hold even during the cb?
I assume we can't hold the lock during the cb currently since we might
try to reschedule, but if it's a recursive mutex would that simplify
things?
I've been doing something similar with IOHandlers for the QContext
stuff, and that's the approach I took. This patch introduces the
recursive mutex:
https://github.com/mdroth/qemu/commit/c7ee0844da62283c9466fcb10ddbfadd0b8bfc53
}
}
@@ -75,6 +85,7 @@ int aio_bh_poll(AioContext *ctx)
/* remove deleted bhs */
if (!ctx-walking_bh) {
+qemu_mutex_lock(ctx-bh_lock);
bhp = ctx-first_bh;
while (*bhp) {
bh = *bhp;
@@ -85,6 +96,7 @@ int aio_bh_poll(AioContext *ctx)
bhp = bh-next;
}
}
+qemu_mutex_unlock(ctx-bh_lock);
}
return ret;
@@ -94,6 +106,10 @@ void qemu_bh_schedule_idle(QEMUBH *bh)
{
if (bh-scheduled)
return;
+/* Make sure any writes that are needed by the callback are done
+ * before the locations are read in the aio_bh_poll.
+ */
+smp_wmb();
bh-scheduled = 1;
bh-idle = 1;
}
@@ -102,6 +118,10 @@ void qemu_bh_schedule(QEMUBH *bh)
{
if (bh-scheduled)
return;
+/* Make sure any writes that are needed by the callback are done
+ * before the locations are read in the aio_bh_poll.
+ */
+smp_wmb();
bh-scheduled = 1;
bh-idle = 0;
aio_notify(bh-ctx);
@@ -211,6 +231,7 @@ AioContext *aio_context_new(void)
ctx = (AioContext *) g_source_new(aio_source_funcs, sizeof(AioContext));
ctx-pollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
ctx-thread_pool = NULL;
+qemu_mutex_init(ctx-bh_lock);
event_notifier_init(ctx-notifier, false);
aio_set_event_notifier(ctx, ctx-notifier,
(EventNotifierHandler *)
diff --git a/include/block/aio.h b/include/block/aio.h
index 1836793..971fbef 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -17,6 +17,7 @@
#include qemu-common.h
#include qemu/queue.h
#include qemu/event_notifier.h
+#include qemu/thread.h
typedef struct BlockDriverAIOCB BlockDriverAIOCB;
typedef void BlockDriverCompletionFunc(void *opaque, int ret);
@@ -53,6 +54,8 @@ typedef struct AioContext {
*/
int walking_handlers;
+/* lock to protect between bh's adders and deleter */
+QemuMutex bh_lock;
/* Anchor of the list of Bottom Halves belonging to the context */
struct QEMUBH *first_bh;
--
1.8.1.4
Re: [Qemu-devel] [PATCH v2 2/2] QEMUBH: make AioContext's bh re-entrant
On Tue, Jun 18, 2013 at 10:14:38AM -0500, mdroth wrote:
On Sun, Jun 16, 2013 at 07:21:21PM +0800, Liu Ping Fan wrote:
BH will be used outside big lock, so introduce lock to protect
between the writers, ie, bh's adders and deleter.
Note that the lock only affects the writers and bh's callback does
not take this extra lock.
Signed-off-by: Liu Ping Fan pingf...@linux.vnet.ibm.com
---
async.c | 21 +
include/block/aio.h | 3 +++
2 files changed, 24 insertions(+)
diff --git a/async.c b/async.c
index 90fe906..6a3269f 100644
--- a/async.c
+++ b/async.c
@@ -47,8 +47,12 @@ QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void
*opaque)
bh-ctx = ctx;
bh-cb = cb;
bh-opaque = opaque;
+qemu_mutex_lock(ctx-bh_lock);
bh-next = ctx-first_bh;
+/* Make sure the memebers ready before putting bh into list */
+smp_wmb();
ctx-first_bh = bh;
+qemu_mutex_unlock(ctx-bh_lock);
return bh;
}
@@ -61,12 +65,18 @@ int aio_bh_poll(AioContext *ctx)
ret = 0;
for (bh = ctx-first_bh; bh; bh = next) {
+/* Make sure fetching bh before accessing its members */
+smp_read_barrier_depends();
next = bh-next;
if (!bh-deleted bh-scheduled) {
bh-scheduled = 0;
if (!bh-idle)
ret = 1;
bh-idle = 0;
+/* Paired with write barrier in bh schedule to ensure reading
for
+ * callbacks coming after bh's scheduling.
+ */
+smp_rmb();
bh-cb(bh-opaque);
Could we possibly simplify this by introducing a recursive mutex that we
could use to protect the whole list loop and hold even during the cb?
I assume we can't hold the lock during the cb currently since we might
try to reschedule, but if it's a recursive mutex would that simplify
things?
I've been doing something similar with IOHandlers for the QContext
stuff, and that's the approach I took. This patch introduces the
recursive mutex:
https://github.com/mdroth/qemu/commit/c7ee0844da62283c9466fcb10ddbfadd0b8bfc53
Doh, missing this fix:
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 0623eca..a4d8170 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -81,7 +81,7 @@ int qemu_mutex_trylock(QemuMutex *mutex)
void qemu_mutex_unlock(QemuMutex *mutex)
{
assert(mutex-owner == GetCurrentThreadId());
-if (!mutex-recursive || mutex-recursion_depth == 0) {
+if (!mutex-recursive || --mutex-recursion_depth == 0) {
mutex-owner = 0;
}
LeaveCriticalSection(mutex-lock);
}
}
@@ -75,6 +85,7 @@ int aio_bh_poll(AioContext *ctx)
/* remove deleted bhs */
if (!ctx-walking_bh) {
+qemu_mutex_lock(ctx-bh_lock);
bhp = ctx-first_bh;
while (*bhp) {
bh = *bhp;
@@ -85,6 +96,7 @@ int aio_bh_poll(AioContext *ctx)
bhp = bh-next;
}
}
+qemu_mutex_unlock(ctx-bh_lock);
}
return ret;
@@ -94,6 +106,10 @@ void qemu_bh_schedule_idle(QEMUBH *bh)
{
if (bh-scheduled)
return;
+/* Make sure any writes that are needed by the callback are done
+ * before the locations are read in the aio_bh_poll.
+ */
+smp_wmb();
bh-scheduled = 1;
bh-idle = 1;
}
@@ -102,6 +118,10 @@ void qemu_bh_schedule(QEMUBH *bh)
{
if (bh-scheduled)
return;
+/* Make sure any writes that are needed by the callback are done
+ * before the locations are read in the aio_bh_poll.
+ */
+smp_wmb();
bh-scheduled = 1;
bh-idle = 0;
aio_notify(bh-ctx);
@@ -211,6 +231,7 @@ AioContext *aio_context_new(void)
ctx = (AioContext *) g_source_new(aio_source_funcs,
sizeof(AioContext));
ctx-pollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
ctx-thread_pool = NULL;
+qemu_mutex_init(ctx-bh_lock);
event_notifier_init(ctx-notifier, false);
aio_set_event_notifier(ctx, ctx-notifier,
(EventNotifierHandler *)
diff --git a/include/block/aio.h b/include/block/aio.h
index 1836793..971fbef 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -17,6 +17,7 @@
#include qemu-common.h
#include qemu/queue.h
#include qemu/event_notifier.h
+#include qemu/thread.h
typedef struct BlockDriverAIOCB BlockDriverAIOCB;
typedef void BlockDriverCompletionFunc(void *opaque, int ret);
@@ -53,6 +54,8 @@ typedef struct AioContext {
*/
int walking_handlers;
+/* lock to protect between bh's adders and deleter */
+QemuMutex bh_lock;
/* Anchor of the list of Bottom Halves belonging to the context */
struct QEMUBH *first_bh;
--
1.8.1.4
Re: [Qemu-devel] [PATCH v2] wdt_i6300esb: fix vmstate versioning
On Tue, Jun 11, 2013 at 04:53:51PM -0500, mdroth wrote:
On Wed, May 22, 2013 at 11:32:51AM -0500, Michael Roth wrote:
When this VMSD was introduced it's version fields were set to
sizeof(I6300State), making them essentially random from build to build,
version to version.
To fix this, we lock in a high version id and low minimum version id to
support old-new migration from all prior versions of this device's
state. This should work since the device state has not changed since
its introduction.
The potentially breaks migration from 1.5+ to 1.5, but since the
versioning was essentially random prior to this patch, new-old
migration was not consistently functional to begin with.
Reported-by: Nicholas Thomas n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Ping, looking to pull this in for 1.5.1
Anthony, Juan? Not sure if this is on your radar. Looking to get it
applied prior to stable freeze tomorrow.
---
v2:
* Fixed s/except/accept/ typo (Laszlo)
hw/watchdog/wdt_i6300esb.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index 1407fba..05af0b1 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -374,9 +374,22 @@ static const MemoryRegionOps i6300esb_ops = {
static const VMStateDescription vmstate_i6300esb = {
.name = i6300esb_wdt,
-.version_id = sizeof(I6300State),
-.minimum_version_id = sizeof(I6300State),
-.minimum_version_id_old = sizeof(I6300State),
+/* With this VMSD's introduction, version_id/minimum_version_id were
+ * erroneously set to sizeof(I6300State), causing a somewhat random
+ * version_id to be set for every build. This eventually broke
+ * migration.
+ *
+ * To correct this without breaking old-new migration for older
versions
+ * of QEMU, we've set version_id to a value high enough to exceed all
past
+ * values of sizeof(I6300State) across various build environments, and
have
+ * reset minimum_version_id_old/minimum_version_id to 1, since this
VMSD
+ * has never changed and thus can accept all past versions.
+ *
+ * For future changes we can treat these values as we normally would.
+ */
+.version_id = 1,
+.minimum_version_id = 1,
+.minimum_version_id_old = 1,
.fields = (VMStateField []) {
VMSTATE_PCI_DEVICE(dev, I6300State),
VMSTATE_INT32(reboot_enabled, I6300State),
--
1.7.9.5
Re: [Qemu-devel] [Qemu-stable] [PATCH] acl: acl_add can't insert before last list element, fix
On Tue, Jun 18, 2013 at 10:05:23AM +0200, Markus Armbruster wrote:
Watch this:
$ upstream-qemu -nodefaults -S -vnc :0,acl,sasl -monitor stdio
QEMU 1.5.50 monitor - type 'help' for more information
(qemu) acl_add vnc.username drei allow
acl: added rule at position 1
(qemu) acl_show vnc.username
policy: deny
1: allow drei
(qemu) acl_add vnc.username zwei allow 1
acl: added rule at position 2
(qemu) acl_show vnc.username
policy: deny
1: allow drei
2: allow zwei
(qemu) acl_add vnc.username eins allow 1
acl: added rule at position 1
(qemu) acl_show vnc.username
policy: deny
1: allow eins
2: allow drei
3: allow zwei
The second acl_add inserts at position 2 instead of 1.
Root cause is an off-by-one in qemu_acl_insert(): when index ==
acl-nentries, it appends instead of inserting before the last list
element.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Markus Armbruster arm...@redhat.com
Reviewed-by: Michael Roth mdr...@linux.vnet.ibm.com
---
util/acl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/util/acl.c b/util/acl.c
index a7f33ff..938b7ae 100644
--- a/util/acl.c
+++ b/util/acl.c
@@ -138,9 +138,9 @@ int qemu_acl_insert(qemu_acl *acl,
if (index = 0)
return -1;
-if (index = acl-nentries)
+if (index acl-nentries) {
return qemu_acl_append(acl, deny, match);
-
+}
entry = g_malloc(sizeof(*entry));
entry-match = g_strdup(match);
--
1.7.11.7
Re: [Qemu-devel] [PATCH v2 2/2] QEMUBH: make AioContext's bh re-entrant
On Tue, Jun 18, 2013 at 09:20:26PM +0200, Paolo Bonzini wrote: Il 18/06/2013 17:14, mdroth ha scritto: Could we possibly simplify this by introducing a recursive mutex that we could use to protect the whole list loop and hold even during the cb? If it is possible, we should avoid recursive locks. It makes impossible to establish a lock hierarchy. For example: I assume we can't hold the lock during the cb currently since we might try to reschedule, but if it's a recursive mutex would that simplify things? If you have two callbacks in two different AioContexts, both of which do bdrv_drain_all(), you get an AB-BA deadlock I think I see what you mean. That problem exists regardless of whether we introduce a recursive mutex though right? I guess the main issue is the fact that we'd be encouraging sloppy locking practices without addressing the root problem? I'm just worried what other subtle problems pop up if we instead rely heavily on memory barriers and inevitably forget one here or there, but maybe that's just me not having a good understanding of when to use them. But doesn't rcu provide higher-level interfaces for these kinds of things? Is it possible to hide any of this behind our list interfaces? Also, the barriers for qemu_bh_schedule are needed anyway. It's only those that order bh-next vs. ctx-first_bh that would go away. I see, I guess it's unavoidable for some cases. Paolo I've been doing something similar with IOHandlers for the QContext stuff, and that's the approach I took. This patch introduces the recursive mutex: https://github.com/mdroth/qemu/commit/c7ee0844da62283c9466fcb10ddbfadd0b8bfc53
Re: [Qemu-devel] Patch Round-up for stable 1.5.1, freeze on 2013-06-19
On Thu, Jun 13, 2013 at 02:36:20PM +0200, Andreas Färber wrote: Am 13.06.2013 14:27, schrieb Paolo Bonzini: Il 12/06/2013 17:41, Michael Roth ha scritto: Hi everyone, The following new patches are queued for QEMU stable v1.5.1: https://github.com/mdroth/qemu/commits/stable-1.5-staging The release is planned for 2013-06-26: http://wiki.qemu.org/Planning/1.5 Please respond here or CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 2013-06-19 for new patches. I'm going to send pull requests for SCSI and NBD on Monday, both of them will include 1.5.1 patches. Thanks! Ditto for my in-flight qom-cpu pull. I'm planning on pulling in these: pc: Fix crash when attempting to hotplug CPU with negative ID target-i386: cpu: Fix potential buffer overrun in get_register_name_32() Were there any others from that PULL? BTW v1.4.2 was still missing in qemu.git yesterday, is there a pull pending already? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] Patch Round-up for stable 1.5.1, freeze on 2013-06-19
On Wed, Jun 12, 2013 at 04:41:12PM -0500, Michael Roth wrote: Hi everyone, The following new patches are queued for QEMU stable v1.5.1: https://github.com/mdroth/qemu/commits/stable-1.5-staging Repo updated with everything I'm tracking that's been committed so far. As far as I'm aware there are also the patches in Paolo's upcoming scsi pull, and the following that haven't yet been applied: wdt_i6300esb: fix vmstate versioning virtio-rng: Fix crash with non-default backend virtio-scsi: forward scsibus for virtio-scsi-pci ppc: do not register IABR SPR twice for 603e savevm: avoid leaking popen(3) file pointer raw-posix: Fix /dev/cdrom magic on OS X s390/virtio-ccw: Fix virtio reset 390/ipl: Fix boot order Freeze is EOD Wednesday so please work with the respective maintainers to get these upstream if you're targetting anything for 1.5.1. The release is planned for 2013-06-26: http://wiki.qemu.org/Planning/1.5 Please respond here or CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 2013-06-19 for new patches. Testing/feedback is greatly appreciated. Thanks! Amos Kong (1): qdev: fix get_fw_dev_path to support to add nothing to fw_dev_path Andreas Färber (1): ide: Set BSY bit during FLUSH Aneesh Kumar K.V (2): hw/9pfs: Fix segfault with 9p2000.u hw/9pfs: use O_NOFOLLOW for mapped readlink operation Brad Smith (2): Remove OSS support for OpenBSD ui/gtk.c: Fix *BSD build of Gtk+ UI Cornelia Huck (2): s390x/css: Fix concurrent sense. virtio-ccw: Fix unsetting of indicators. Ed Maste (1): host-libusb: Correct test for USB packet state Gerd Hoffmann (3): chardev: fix info chardev output Revert roms: switch oldnoconfig to olddefconfig update seabios to release 1.7.2.2 Luiz Capitulino (1): target-i386: fix abort on bad PML4E/PDPTE/PDE/PTE addresses Michael Marineau (1): Fix usage of USB_DEV_FLAG_IS_HOST flag. Michael Roth (1): qemu-char: don't issue CHR_EVENT_OPEN in a BH Michael S. Tsirkin (1): q35: set fw_name Paolo Bonzini (1): do not check pointers after dereferencing them Peter Crosthwaite (1): qom/object: Don't poll cast cache for NULL objects Richard Henderson (1): target-i386: Fix aflag logic for CODE64 and the 0x67 prefix Stefan Hajnoczi (2): rtl8139: flush queued packets when RxBufPtr is written vmxnet3: fix NICState cleanup Stefano Stabellini (4): xen: simplify xen_enabled main_loop: do not set nonblocking if xen_enabled() xen_machine_pv: do not create a dummy CPU in machine-init xen: start PCI hole at 0xe000 (same as pc_init1 and qemu-xen-traditional) Wendy Liang (1): xilinx_axidma: Do not set DMA .notify to NULL after notify audio/ossaudio.c |4 backends/baum.c |2 -- backends/msmouse.c|1 + configure |5 ++--- hw/9pfs/virtio-9p-local.c |2 +- hw/9pfs/virtio-9p.c |2 +- hw/core/qdev.c| 10 - hw/dma/xilinx_axidma.c|3 ++- hw/i386/pc_piix.c |6 +++--- hw/i386/xen_machine_pv.c | 16 --- hw/ide/core.c |1 + hw/net/rtl8139.c |3 +++ hw/net/vmxnet3.c |2 +- hw/pci-host/q35.c |1 + hw/s390x/css.c|2 +- hw/s390x/virtio-ccw.c |8 hw/usb/core.c |2 +- hw/usb/host-libusb.c |2 +- hw/virtio/virtio-bus.c|6 ++ include/hw/i386/pc.h |3 +++ include/hw/xen/xen.h |4 include/qemu-common.h |1 + include/sysemu/char.h |2 +- monitor.c |2 +- pc-bios/bios.bin | Bin 131072 - 131072 bytes qemu-char.c | 41 ++--- qom/object.c |4 ++-- roms/configure-seabios.sh |2 +- roms/seabios |2 +- savevm.c |8 spice-qemu-char.c |1 + target-i386/arch_memory_mapping.c | 10 + target-i386/translate.c | 30 +-- ui/console.c |4 ui/gtk.c |2 ++ vl.c |2 +- xen-all.c | 12 +-- 37 files changed, 107 insertions(+), 101 deletions(-)
Re: [Qemu-devel] Patch Round-up for stable 1.5.1, freeze on 2013-06-19
On Mon, Jun 17, 2013 at 04:04:31PM -0700, Richard Henderson wrote: On 06/17/2013 03:56 PM, mdroth wrote: On Wed, Jun 12, 2013 at 04:41:12PM -0500, Michael Roth wrote: Hi everyone, The following new patches are queued for QEMU stable v1.5.1: https://github.com/mdroth/qemu/commits/stable-1.5-staging Repo updated with everything I'm tracking that's been committed so far. As far as I'm aware there are also the patches in Paolo's upcoming scsi pull, and the following that haven't yet been applied: wdt_i6300esb: fix vmstate versioning virtio-rng: Fix crash with non-default backend virtio-scsi: forward scsibus for virtio-scsi-pci ppc: do not register IABR SPR twice for 603e savevm: avoid leaking popen(3) file pointer raw-posix: Fix /dev/cdrom magic on OS X s390/virtio-ccw: Fix virtio reset 390/ipl: Fix boot order Freeze is EOD Wednesday so please work with the respective maintainers to get these upstream if you're targetting anything for 1.5.1. Anton's tcg/ppc64 patch set ought to go to 1.5.1. It was merged to mainline today; the patches from the pull request were cc'd to qemu-stable. The emails were still were trickling into qemu-stable out of order when I sent that for some reason, so I only had 2-4 of that pull when I sent my last email. When 1/4 finally landed I went ahead and rebased/re-applied them all in order so they should all be there now. r~
Re: [Qemu-devel] [Qemu-stable] [PATCH v2 0/6] Some -smbios work
On Mon, Jun 17, 2013 at 01:36:04PM +0200, Markus Armbruster wrote: Would this make sense for -stable? Impact is modest: fix a rather obscure feature of x86 targets on bigendian hosts, and improve error messages around it). On the other hand, the patches look pretty safe to me. 5/6 was certainly a good candidate. I was going to cherry-pick it but I agree the other patches looked safe to I went ahead and applied the series: https://github.com/mdroth/qemu/commits/stable-1.5-staging Markus Armbruster arm...@redhat.com writes: Better error messages, a bit of code cleanup, and a big endian fix. Not addressed: qemu_uuid_parse() sets an SMBIOS field by side effect. Gross! Testing: * Verify error messages improve for -smbios gaga -smbios file=gaga -smbios type=42 -smbios type=1,uuid=gaga -smbios type=0,release=gaga * Verify SMBIOS table remains unchanged -smbios type=0,vendor=me,version=42,date=today,release=1.2 -smbios type=1,manufacturer=me,product=crap,version=6,serial=77,uuid=988bc9dd-0986-440f-ac24-cf9626c5aa88,sku=888,family=flintstones by sticking qemu_hexdump(smbios_entries, stdout, SMBIOS, smbios_entries_len); into smbios_get_table() v2: Address Hawkeye Laszlo's review * 1-3/7 unchanged * Drop 4/7 because it's buggy, and the fixed version isn't worthwhile * Spelling fix in commit message of 5/7 * Correct scanf format in 5-6/7 Markus Armbruster (6): error-report.h: Supply missing include log.h: Supply missing includes smbios: Convert to error_report() smbios: Clean up smbios_add_field() parameters smbios: Fix -smbios type=0,release=... for big endian hosts smbios: Check R in -smbios type=0,release=R parses okay arch_init.c | 3 +-- hw/i386/smbios.c| 57 - include/hw/i386/smbios.h| 2 +- include/qemu/error-report.h | 1 + include/qemu/log.h | 3 +++ 5 files changed, 37 insertions(+), 29 deletions(-)
Re: [Qemu-devel] Patch Round-up for stable 1.5.1, freeze on 2013-06-19
On Thu, Jun 13, 2013 at 02:36:20PM +0200, Andreas Färber wrote: Am 13.06.2013 14:27, schrieb Paolo Bonzini: Il 12/06/2013 17:41, Michael Roth ha scritto: Hi everyone, The following new patches are queued for QEMU stable v1.5.1: https://github.com/mdroth/qemu/commits/stable-1.5-staging The release is planned for 2013-06-26: http://wiki.qemu.org/Planning/1.5 Please respond here or CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 2013-06-19 for new patches. I'm going to send pull requests for SCSI and NBD on Monday, both of them will include 1.5.1 patches. Thanks! Ditto for my in-flight qom-cpu pull. BTW v1.4.2 was still missing in qemu.git yesterday, is there a pull pending already? Hmm, yah, it's pushed/tagging in the 1.4 repo: http://git.qemu.org/?p=qemu-stable-1.4.git;a=commit;h=89400a80f5827ae3696e3da73df0996154965a0a Anthony, do you still need to pull 1.4.2 into qemu.git? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] Patch Round-up for stable 1.5.1, freeze on 2013-06-19
On Thu, Jun 13, 2013 at 03:29:46PM +0200, Frederic Konrad wrote: On 13/06/2013 15:21, Andreas Färber wrote: Am 13.06.2013 14:44, schrieb Frederic Konrad: On 12/06/2013 23:41, Michael Roth wrote: Hi everyone, The following new patches are queued for QEMU stable v1.5.1: https://github.com/mdroth/qemu/commits/stable-1.5-staging The release is planned for 2013-06-26: http://wiki.qemu.org/Planning/1.5 Please respond here or CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 2013-06-19 for new patches. Testing/feedback is greatly appreciated. And it seems you forget virtio-rng: Fix crash with non-default backend. You said you'll pick it up for 1.5.1 ;) It's not yet committed to master, so cannot be cherry-picked into the queue yet. Waiting for it myself. ;) Andreas Oh ok, so it need to be in master for being merged in stable? Yes. There are exceptions in rare cases (such as preventing WW3), but for the most part we need patches to get the full scrutiny of going through upstream review process before we pull them into a stable branch. Same reason why: virtio-scsi: forward scsibus for virtio-scsi-pci isn't in yet. If you're targeting a patch for a stable release and think it may not make it upstream in time consider pinging the respective maintainers if it's had enough time for review. I didn't know that ;) Fred
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
On Tue, May 21, 2013 at 05:32:57PM -0500, Michael Roth wrote:
When this VMSD was introduced it's version fields were set to
sizeof(I6300State), making them essentially random from build to build,
version to version.
To fix this, we lock in a high version id and low minimum version id to
support old-new migration from all prior versions of this device's
state. This should work since the device state has not changed since
its introduction.
The potentially breaks migration from 1.5+ to 1.5, but since the
versioning was essentially random prior to this patch, new-old
migration was not consistently functional to begin with.
Reported-by: Nicholas Thomas n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
CC'ing qemu-trivial. Looking to get this in for 1.5.1
---
hw/watchdog/wdt_i6300esb.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index 1407fba..851b664 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -374,9 +374,22 @@ static const MemoryRegionOps i6300esb_ops = {
static const VMStateDescription vmstate_i6300esb = {
.name = i6300esb_wdt,
-.version_id = sizeof(I6300State),
-.minimum_version_id = sizeof(I6300State),
-.minimum_version_id_old = sizeof(I6300State),
+/* With this VMSD's introduction, version_id/minimum_version_id were
+ * erroneously set to sizeof(I6300State), causing a somewhat random
+ * version_id to be set for every build. This eventually broke
+ * migration.
+ *
+ * To correct this without breaking old-new migration for older versions
+ * of QEMU, we've set version_id to a value high enough to exceed all
past
+ * values of sizeof(I6300State) across various build environments, and
have
+ * reset minimum_version_id_old/minimum_version_id to 1, since this VMSD
+ * has never changed and thus can except all past versions.
+ *
+ * For future changes we can treat these values as we normally would.
+ */
+.version_id = 1,
+.minimum_version_id = 1,
+.minimum_version_id_old = 1,
.fields = (VMStateField []) {
VMSTATE_PCI_DEVICE(dev, I6300State),
VMSTATE_INT32(reboot_enabled, I6300State),
--
1.7.9.5
Re: [Qemu-devel] [PATCH v2] virtio-rng: Fix crash with non-default backend
On Fri, May 31, 2013 at 02:12:48PM -0400, Cole Robinson wrote: 'default_backend' isn't always set, but 'rng' is, so use that. $ ./x86_64-softmmu/qemu-system-x86_64 -object rng-random,id=rng0,filename=/dev/random -device virtio-rng-pci,rng=rng0 Segmentation fault (core dumped) Regressed with virtio refactoring in 59ccd20a9ac719cff82180429458728f03ec612f CC: qemu-sta...@nongnu.org Signed-off-by: Cole Robinson crobi...@redhat.com Tested-by: Michael Roth mdr...@linux.vnet.ibm.com Looking to pull this in for 1.5.1 --- Notes: v2: Fix other instances in s390x code, pointed out by Frederic hw/s390x/s390-virtio-bus.c | 2 +- hw/s390x/virtio-ccw.c | 2 +- hw/virtio/virtio-pci.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c index a1cdfb0..207eb82 100644 --- a/hw/s390x/s390-virtio-bus.c +++ b/hw/s390x/s390-virtio-bus.c @@ -300,7 +300,7 @@ static int s390_virtio_rng_init(VirtIOS390Device *s390_dev) } object_property_set_link(OBJECT(dev), - OBJECT(dev-vdev.conf.default_backend), rng, + OBJECT(dev-vdev.conf.rng), rng, NULL); return s390_virtio_device_init(s390_dev, VIRTIO_DEVICE(vdev)); diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 5f5e267..7dcd98d 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -744,7 +744,7 @@ static int virtio_ccw_rng_init(VirtioCcwDevice *ccw_dev) } object_property_set_link(OBJECT(dev), - OBJECT(dev-vdev.conf.default_backend), rng, + OBJECT(dev-vdev.conf.rng), rng, NULL); return virtio_ccw_device_init(ccw_dev, VIRTIO_DEVICE(vdev)); diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 444b71a..b070b64 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1455,7 +1455,7 @@ static int virtio_rng_pci_init(VirtIOPCIProxy *vpci_dev) } object_property_set_link(OBJECT(vrng), - OBJECT(vrng-vdev.conf.default_backend), rng, + OBJECT(vrng-vdev.conf.rng), rng, NULL); return 0; -- 1.8.2.1
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
On Wed, Jun 12, 2013 at 09:42:14PM +0100, Peter Maydell wrote: On 12 June 2013 21:11, mdroth mdr...@linux.vnet.ibm.com wrote: On Tue, May 21, 2013 at 05:32:57PM -0500, Michael Roth wrote: When this VMSD was introduced it's version fields were set to sizeof(I6300State), making them essentially random from build to build, version to version. To fix this, we lock in a high version id and low minimum version id to support old-new migration from all prior versions of this device's state. This should work since the device state has not changed since its introduction. The potentially breaks migration from 1.5+ to 1.5, but since the versioning was essentially random prior to this patch, new-old migration was not consistently functional to begin with. Reported-by: Nicholas Thomas n...@bytemark.co.uk Suggested-by: Peter Maydell peter.mayd...@linaro.org Cc: qemu-sta...@nongnu.org Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com CC'ing qemu-trivial. Looking to get this in for 1.5.1 This is a good patch but it definitely doesn't seem like -trivial material to me. -trivial isn't way to get patches in that would otherwise fall through the cracks :-) I honestly thought it was trivial once all the considerations were documented, but reading through it all again makes my head hurt a bit so you're probably right. Anthony, Juan? thanks -- PMM
Re: [Qemu-devel] [PATCH] wdt_i6300esb: fix vmstate versioning
On Wed, Jun 12, 2013 at 04:17:53PM -0500, Anthony Liguori wrote:
mdroth mdr...@linux.vnet.ibm.com writes:
On Tue, May 21, 2013 at 05:32:57PM -0500, Michael Roth wrote:
When this VMSD was introduced it's version fields were set to
sizeof(I6300State), making them essentially random from build to build,
version to version.
To fix this, we lock in a high version id and low minimum version id to
support old-new migration from all prior versions of this device's
state. This should work since the device state has not changed since
its introduction.
The potentially breaks migration from 1.5+ to 1.5, but since the
versioning was essentially random prior to this patch, new-old
migration was not consistently functional to begin with.
Reported-by: Nicholas Thomas n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
CC'ing qemu-trivial. Looking to get this in for 1.5.1
v2? There were some review comments that haven't been addressed.
Sorry, pinged the wrong email. v2 is to the latest:
http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg02991.html
Regards,
Anthony Liguori
---
hw/watchdog/wdt_i6300esb.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index 1407fba..851b664 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -374,9 +374,22 @@ static const MemoryRegionOps i6300esb_ops = {
static const VMStateDescription vmstate_i6300esb = {
.name = i6300esb_wdt,
-.version_id = sizeof(I6300State),
-.minimum_version_id = sizeof(I6300State),
-.minimum_version_id_old = sizeof(I6300State),
+/* With this VMSD's introduction, version_id/minimum_version_id were
+ * erroneously set to sizeof(I6300State), causing a somewhat random
+ * version_id to be set for every build. This eventually broke
+ * migration.
+ *
+ * To correct this without breaking old-new migration for older
versions
+ * of QEMU, we've set version_id to a value high enough to exceed all
past
+ * values of sizeof(I6300State) across various build environments,
and have
+ * reset minimum_version_id_old/minimum_version_id to 1, since this
VMSD
+ * has never changed and thus can except all past versions.
+ *
+ * For future changes we can treat these values as we normally would.
+ */
+.version_id = 1,
+.minimum_version_id = 1,
+.minimum_version_id_old = 1,
.fields = (VMStateField []) {
VMSTATE_PCI_DEVICE(dev, I6300State),
VMSTATE_INT32(reboot_enabled, I6300State),
--
1.7.9.5
Re: [Qemu-devel] [PATCH] ide-test: fix failure for test_flush
On Tue, Jun 11, 2013 at 12:05:20PM +0200, Andreas Färber wrote:
Am 10.06.2013 20:23, schrieb Michael Roth:
bd07684aacfb61668ae2c25b7dd00b64f3d7c7f3 added a test to ensure BSY
flag is set when a flush request is in flight. It does this by setting
a blkdebug breakpoint on flush_to_os before issuing a CMD_FLUSH_CACHE.
It then resumes CMD_FLUSH_CACHE operation and checks that BSY is unset.
The actual unsetting of BSY does not occur until ide_flush_cb gets
called in a bh, however, so in some cases this check will race with
the actual completion.
Fix this by polling the ide status register until BSY flag gets unset
before we do our final sanity checks. According to
f68ec8379e88502b4841a110c070e9b118d3151c this is in line with how a guest
would determine whether or not the device is still busy.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
tests/ide-test.c |5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tests/ide-test.c b/tests/ide-test.c
index 828e71a..7e2eb94 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -455,7 +455,10 @@ static void test_flush(void)
data = inb(IDE_BASE + reg_device);
g_assert_cmpint(data DEV, ==, 0);
-data = inb(IDE_BASE + reg_status);
+do {
+data = inb(IDE_BASE + reg_status);
+} while (data BSY);
Is a busy loop really a good idea for a qtest? CC'ing Anthony.
I'm not sure, my main justification was simply that we currently do it
in ide-test for send_dma_request()
For the theoretical case that BSY is not cleared it might be better to
terminate the loop with some timeout to get an assertion failure or at
least use some form of sleep() to yield the thread while waiting?
I don't think yielding/sleeping is too big a deal since we do a blocking
read() on each iteration which i assume will result in a yield while the
inb is being processed by qemu. Timeouts might be nice thing to add later
though. What about something like this?
inb_wait(addr, testfn, opaque, timeout) {
while (timeout == -1 || timeout-- 0) {
val = inb(addr);
if (testfn(val, opaque)) {
return val;
}
usleep(1000);
}
abort()
}
It's not particularly precise, but for imposing a rough limit it should
work.
+
assert_bit_set(data, DRDY);
assert_bit_clear(data, BSY | DF | ERR | DRQ);
This BSY clear assertion will always be true now due to the above while
condition; it won't if we change it though.
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [Qemu-stable] [PATCH for-1.5] ppc: do not register IABR SPR twice for 603e
On Mon, May 20, 2013 at 08:17:47PM +0200, Hervé Poussineau wrote: Ping. This should be IMO committed to stable, as it fixes a crash with qemu-system-ppc -M prep -cpu 603e Ping^2. Looking to pull this in for 1.5.1 Hervé Hervé Poussineau a écrit : IABR SPR is already registered in gen_spr_603(), called from init_proc_603E(). Signed-off-by: Hervé Poussineau hpous...@reactos.org --- target-ppc/translate_init.c |5 - 1 file changed, 5 deletions(-) diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 6feb62a..248d3e0 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -4980,11 +4980,6 @@ static void init_proc_603E (CPUPPCState *env) SPR_NOACCESS, SPR_NOACCESS, spr_read_generic, spr_write_generic, 0x); -/* XXX : not implemented */ -spr_register(env, SPR_IABR, IABR, - SPR_NOACCESS, SPR_NOACCESS, - spr_read_generic, spr_write_generic, - 0x); /* Memory management */ gen_low_BATs(env); gen_6xx_7xx_soft_tlb(env, 64, 2);
Re: [Qemu-devel] [PATCH v2] wdt_i6300esb: fix vmstate versioning
On Wed, May 22, 2013 at 11:32:51AM -0500, Michael Roth wrote:
When this VMSD was introduced it's version fields were set to
sizeof(I6300State), making them essentially random from build to build,
version to version.
To fix this, we lock in a high version id and low minimum version id to
support old-new migration from all prior versions of this device's
state. This should work since the device state has not changed since
its introduction.
The potentially breaks migration from 1.5+ to 1.5, but since the
versioning was essentially random prior to this patch, new-old
migration was not consistently functional to begin with.
Reported-by: Nicholas Thomas n...@bytemark.co.uk
Suggested-by: Peter Maydell peter.mayd...@linaro.org
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Ping, looking to pull this in for 1.5.1
---
v2:
* Fixed s/except/accept/ typo (Laszlo)
hw/watchdog/wdt_i6300esb.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index 1407fba..05af0b1 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -374,9 +374,22 @@ static const MemoryRegionOps i6300esb_ops = {
static const VMStateDescription vmstate_i6300esb = {
.name = i6300esb_wdt,
-.version_id = sizeof(I6300State),
-.minimum_version_id = sizeof(I6300State),
-.minimum_version_id_old = sizeof(I6300State),
+/* With this VMSD's introduction, version_id/minimum_version_id were
+ * erroneously set to sizeof(I6300State), causing a somewhat random
+ * version_id to be set for every build. This eventually broke
+ * migration.
+ *
+ * To correct this without breaking old-new migration for older versions
+ * of QEMU, we've set version_id to a value high enough to exceed all
past
+ * values of sizeof(I6300State) across various build environments, and
have
+ * reset minimum_version_id_old/minimum_version_id to 1, since this VMSD
+ * has never changed and thus can accept all past versions.
+ *
+ * For future changes we can treat these values as we normally would.
+ */
+.version_id = 1,
+.minimum_version_id = 1,
+.minimum_version_id_old = 1,
.fields = (VMStateField []) {
VMSTATE_PCI_DEVICE(dev, I6300State),
VMSTATE_INT32(reboot_enabled, I6300State),
--
1.7.9.5
Re: [Qemu-devel] [PULL 0/2] update seabios to release 1.7.2.2
On Tue, May 28, 2013 at 12:28:50PM +0200, Gerd Hoffmann wrote: Hi, This pull updates seabios to the 1.7.2.2 release tagged yesterday, bringing some fixes which unfortunaly missed the qemu 1.5 boat. please pull, Gerd The following changes since commit 6a4e17711442849bf2cc731ccddef5a2a2d92d29: Remove Sun4c, Sun4d and a few CPUs (2013-05-26 11:37:58 +) are available in the git repository at: git://git.kraxel.org/qemu seabios-1.7.2.2 for you to fetch changes up to 6683d7bc2759e9da8b5c4907a9de85d500933ffb: update seabios to release 1.7.2.2 (2013-05-28 12:19:02 +0200) Gerd Hoffmann (2): Revert roms: switch oldnoconfig to olddefconfig update seabios to release 1.7.2.2 Ping, looking to pull these for 1.5.1 pc-bios/bios.bin | Bin 131072 - 131072 bytes roms/configure-seabios.sh |2 +- roms/seabios |2 +- 3 files changed, 2 insertions(+), 2 deletions(-)
Re: [Qemu-devel] [PULL 0/2] update seabios to release 1.7.2.2
On Tue, Jun 11, 2013 at 05:05:59PM -0500, mdroth wrote: On Tue, May 28, 2013 at 12:28:50PM +0200, Gerd Hoffmann wrote: Hi, This pull updates seabios to the 1.7.2.2 release tagged yesterday, bringing some fixes which unfortunaly missed the qemu 1.5 boat. please pull, Gerd The following changes since commit 6a4e17711442849bf2cc731ccddef5a2a2d92d29: Remove Sun4c, Sun4d and a few CPUs (2013-05-26 11:37:58 +) are available in the git repository at: git://git.kraxel.org/qemu seabios-1.7.2.2 for you to fetch changes up to 6683d7bc2759e9da8b5c4907a9de85d500933ffb: update seabios to release 1.7.2.2 (2013-05-28 12:19:02 +0200) Gerd Hoffmann (2): Revert roms: switch oldnoconfig to olddefconfig update seabios to release 1.7.2.2 Ping, looking to pull these for 1.5.1 Sorry, already applied, git failure on my end pc-bios/bios.bin | Bin 131072 - 131072 bytes roms/configure-seabios.sh |2 +- roms/seabios |2 +- 3 files changed, 2 insertions(+), 2 deletions(-)
Re: [Qemu-devel] [PATCH 1/3] vhost-scsi: fix k-set_guest_notifiers() NULL dereference
On Thu, May 30, 2013 at 04:14:44PM +0200, Stefan Hajnoczi wrote:
Coverity picked up a copy-paste bug. In vhost_scsi_start() we check for
!k-set_guest_notifiers and error out. The check probably got copied
but instead of erroring we actually use the function pointer!
Cc: Nicholas Bellinger n...@linux-iscsi.org
Cc: Asias He as...@redhat.com
Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Hi Paolo,
Looking to pick this up for 1.5.1 along with a few other goodies in your
scsi branch:
iscsi: reorganize iscsi_readcapacity_sync
iscsi: simplify freeing of tasks
scsi-disk: scsi-block device for scsi pass-through should not be remo…
scsi-generic: check the return value of bdrv_aio_ioctl in execute_com…
scsi-generic: fix sign extension of READ CAPACITY(10) data
scsi: reset cdrom tray statuses on scsi_disk_reset
Freeze for 1.5.1 is planned for June 19. Willing to pluck from
maintainer branches for the more important ones but would prefer
upstream if you can send a PULL for these.
---
hw/scsi/vhost-scsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index d7a1c33..785e93f 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -123,7 +123,7 @@ static void vhost_scsi_stop(VHostSCSI *s)
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
int ret = 0;
-if (!k-set_guest_notifiers) {
+if (k-set_guest_notifiers) {
ret = k-set_guest_notifiers(qbus-parent, s-dev.nvqs, false);
if (ret 0) {
error_report(vhost guest notifier cleanup failed: %d\n,
ret);
--
1.8.1.4
Re: [Qemu-devel] [Qemu-stable] [PATCH 3/3] savevm: avoid leaking popen(3) file pointer
On Thu, May 30, 2013 at 04:14:46PM +0200, Stefan Hajnoczi wrote:
I'm not sure why we check the mode only after invoking popen(3) but we
need to close the file pointer.
Spotted by Coverity.
Cc: Juan Quintela quint...@redhat.com
Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Ping, looking to get this in for 1.5.1
---
savevm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/savevm.c b/savevm.c
index 31dcce9..75cc72e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -329,6 +329,7 @@ QEMUFile *qemu_popen_cmd(const char *command, const char
*mode)
if (mode == NULL || (mode[0] != 'r' mode[0] != 'w') || mode[1] != 0) {
fprintf(stderr, qemu_popen: Argument validity check failed\n);
+fclose(stdio_file);
return NULL;
}
--
1.8.1.4
Re: [Qemu-devel] [PATCHv2 1/2] Revert migration: do not sent zero pages in bulk stage
On Mon, Jun 10, 2013 at 12:14:19PM +0200, Peter Lieven wrote:
Not sending zero pages breaks migration if a page is zero
at the source but not at the destination. This can e.g. happen
if different BIOS versions are used at source and destination.
It has also been reported that migration on pseries is completely
broken with this patch.
This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.
Conflicts:
arch_init.c
Signed-off-by: Peter Lieven p...@kamp.de
Reviewed-by: Michael Roth mdr...@linux.vnet.ibm.com
Also CC'ing qemu-stable
---
arch_init.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 5d32ecf..08fccf6 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -457,15 +457,10 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
bytes_sent = -1;
if (is_zero_page(p)) {
acct_info.dup_pages++;
-if (!ram_bulk_stage) {
-bytes_sent = save_block_hdr(f, block, offset, cont,
-RAM_SAVE_FLAG_COMPRESS);
-qemu_put_byte(f, 0);
-bytes_sent++;
-} else {
-acct_info.skipped_pages++;
-bytes_sent = 0;
-}
+bytes_sent = save_block_hdr(f, block, offset, cont,
+RAM_SAVE_FLAG_COMPRESS);
+qemu_put_byte(f, 0);
+bytes_sent++;
} else if (!ram_bulk_stage migrate_use_xbzrle()) {
current_addr = block-offset + offset;
bytes_sent = save_xbzrle_page(f, p, current_addr, block,
--
1.7.9.5
Re: [Qemu-devel] [PATCHv2 2/2] migration: do not overwrite zero pages
On Mon, Jun 10, 2013 at 12:14:20PM +0200, Peter Lieven wrote:
on incoming migration do not memset pages to zero if they already read as
zero.
this will allocate a new zero page and consume memory unnecessarily. even
if we madvise a MADV_DONTNEED later this will only deallocate the memory
asynchronously.
Signed-off-by: Peter Lieven p...@kamp.de
---
arch_init.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 08fccf6..cf4e1d5 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int
version_id)
}
ch = qemu_get_byte(f);
-memset(host, ch, TARGET_PAGE_SIZE);
+if (ch != 0 || !is_zero_page(host)) {
+memset(host, ch, TARGET_PAGE_SIZE);
#ifndef _WIN32
-if (ch == 0
-(!kvm_enabled() || kvm_has_sync_mmu())
-getpagesize() = TARGET_PAGE_SIZE) {
-qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
-}
+if (ch == 0
+(!kvm_enabled() || kvm_has_sync_mmu())
+getpagesize() = TARGET_PAGE_SIZE) {
+qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
+}
Reviewed-by: Michael Roth mdr...@linux.vnet.ibm.com
Also CC'ing qemu-stable, but from what I gather this just mitigates the
performance impact of 1/2 and isn't strictly necessary to fix migration?
i.e. we can still do outgoing migration to 1.5.0 guests?
#endif
+}
} else if (flags RAM_SAVE_FLAG_PAGE) {
void *host;
--
1.7.9.5
Re: [Qemu-devel] [PATCHv2 2/2] migration: do not overwrite zero pages
On Mon, Jun 10, 2013 at 11:10:29AM -0500, mdroth wrote:
On Mon, Jun 10, 2013 at 12:14:20PM +0200, Peter Lieven wrote:
on incoming migration do not memset pages to zero if they already read as
zero.
this will allocate a new zero page and consume memory unnecessarily. even
if we madvise a MADV_DONTNEED later this will only deallocate the memory
asynchronously.
Signed-off-by: Peter Lieven p...@kamp.de
---
arch_init.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 08fccf6..cf4e1d5 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -832,14 +832,16 @@ static int ram_load(QEMUFile *f, void *opaque, int
version_id)
}
ch = qemu_get_byte(f);
-memset(host, ch, TARGET_PAGE_SIZE);
+if (ch != 0 || !is_zero_page(host)) {
+memset(host, ch, TARGET_PAGE_SIZE);
#ifndef _WIN32
-if (ch == 0
-(!kvm_enabled() || kvm_has_sync_mmu())
-getpagesize() = TARGET_PAGE_SIZE) {
-qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
-}
+if (ch == 0
+(!kvm_enabled() || kvm_has_sync_mmu())
+getpagesize() = TARGET_PAGE_SIZE) {
+qemu_madvise(host, TARGET_PAGE_SIZE,
QEMU_MADV_DONTNEED);
+}
Reviewed-by: Michael Roth mdr...@linux.vnet.ibm.com
Also CC'ing qemu-stable, but from what I gather this just mitigates the
*actually* CC'ing qemu-stable this time :)
Re: [Qemu-devel] [PATCH v4] qemu-char: don't issue CHR_EVENT_OPEN in a BH
On Fri, Jun 07, 2013 at 09:50:39AM -0400, Luiz Capitulino wrote: On Tue, 4 Jun 2013 16:35:09 -0500 Michael Roth mdr...@linux.vnet.ibm.com wrote: When CHR_EVENT_OPENED was initially added, it was CHR_EVENT_RESET, and it was issued as a bottom-half: 86e94dea5b740dad65446c857f6959eae43e0ba6 Which we basically used to print out a greeting/prompt for the monitor. AFAICT the only reason this was ever done in a BH was because in some cases we'd modify the chr_write handler for a new chardev backend *after* the site where we issued the reset (see: 86e94d:qemu_chr_open_stdio()) At some point this event was renamed to CHR_EVENT_OPENED, and we've maintained the use of this BH ever since. However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule the BH via g_idle_add(), which is causing events to sometimes be delivered after we've already begun processing data from backends, leading to: known bugs: QMP: session negotation resets with OPENED event, in some cases this is causing new sessions to get sporadically reset potential bugs: hw/usb/redirect.c: can_read handler checks for dev-parser != NULL, which may be true if CLOSED BH has not been executed yet. In the past, OPENED quiesced outstanding CLOSED events prior to us reading client data. If it's delayed, our check may allow reads to occur even though we haven't processed the OPENED event yet, and when we do finally get the OPENED event, our state may get reset. qtest.c: can begin session before OPENED event is processed, leading to a spurious reset of the system and irq_levels gdbstub.c: may start a gdb session prior to the machine being paused To fix these, let's just drop the BH. Since the initial reasoning for using it still applies to an extent, work around that by deferring the delivery of CHR_EVENT_OPENED until after the chardevs have been fully initialized, toward the end of qmp_chardev_add() (or some cases, qemu_chr_new_from_opts()). This defers delivery long enough that we can be assured a CharDriverState is fully initialized before CHR_EVENT_OPENED is sent. Also, rather than requiring each chardev to do an explicit open, do it automatically, and allow the small few who don't desire such behavior to suppress the OPENED-on-init behavior by setting a 'explicit_be_open' flag. We additionally add missing OPENED events for stdio backends on w32, which were previously not being issued, causing us to not recieve the banner and initial prompts for qmp/hmp. Reported-by: Stefan Priebe s.pri...@profihost.ag Cc: qemu-sta...@nongnu.org Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com I don't know if the QMP queue is the ideal queue for this patch, but I'd happily apply it if I get at least one Reviewed-by from the CC'ed people. Anthony actually added his Reviewed-by for v3, but I forgot to roll it in the commit. If you do take it in through your tree can you add that? Thanks! --- v3-v4: * renamed 'suppress_be_open_on_init' to 'explicit_be_open' to match existing 'explicit_fe_open' flag (Hans) * added missing 'explicit_be_open' flags for spice vmc/port and msmouse backends v2-v3: * removed artifact in from v1 in backends/baum.c, test build with BRLAPI=y (Anthony) * rebased on latest origin/master v1-v2: * default to sending OPENED on backend init, add flag to suppress it (Anthony) * fix missing OPENED for stdio backends on w32 * fix missing OPENED when qemu_chr_new_from_opts() doesn't use qmp_chardev_add() * clean up/update commit message backends/baum.c |2 -- backends/msmouse.c|1 + include/sysemu/char.h |2 +- qemu-char.c | 38 +- spice-qemu-char.c |1 + ui/console.c |1 - ui/gtk.c |1 - 7 files changed, 20 insertions(+), 26 deletions(-) diff --git a/backends/baum.c b/backends/baum.c index 4cba79f..62aa784 100644 --- a/backends/baum.c +++ b/backends/baum.c @@ -611,8 +611,6 @@ CharDriverState *chr_baum_init(void) qemu_set_fd_handler(baum-brlapi_fd, baum_chr_read, NULL, baum); -qemu_chr_be_generic_open(chr); - return chr; fail: diff --git a/backends/msmouse.c b/backends/msmouse.c index 0ac05a0..c0dbfcd 100644 --- a/backends/msmouse.c +++ b/backends/msmouse.c @@ -70,6 +70,7 @@ CharDriverState *qemu_chr_open_msmouse(void) chr = g_malloc0(sizeof(CharDriverState)); chr-chr_write = msmouse_chr_write; chr-chr_close = msmouse_chr_close; +chr-explicit_be_open = true; qemu_add_mouse_event_handler(msmouse_event, chr, 0, QEMU Microsoft Mouse); diff --git a/include/sysemu/char.h b/include/sysemu/char.h index 5e42c90..066c216 100644 ---
Re: [Qemu-devel] [PATCH v4] qemu-char: don't issue CHR_EVENT_OPEN in a BH
On Fri, Jun 07, 2013 at 11:36:16AM -0400, Luiz Capitulino wrote: On Fri, 7 Jun 2013 09:56:00 -0500 mdroth mdr...@linux.vnet.ibm.com wrote: On Fri, Jun 07, 2013 at 09:50:39AM -0400, Luiz Capitulino wrote: On Tue, 4 Jun 2013 16:35:09 -0500 Michael Roth mdr...@linux.vnet.ibm.com wrote: When CHR_EVENT_OPENED was initially added, it was CHR_EVENT_RESET, and it was issued as a bottom-half: 86e94dea5b740dad65446c857f6959eae43e0ba6 Which we basically used to print out a greeting/prompt for the monitor. AFAICT the only reason this was ever done in a BH was because in some cases we'd modify the chr_write handler for a new chardev backend *after* the site where we issued the reset (see: 86e94d:qemu_chr_open_stdio()) At some point this event was renamed to CHR_EVENT_OPENED, and we've maintained the use of this BH ever since. However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule the BH via g_idle_add(), which is causing events to sometimes be delivered after we've already begun processing data from backends, leading to: known bugs: QMP: session negotation resets with OPENED event, in some cases this is causing new sessions to get sporadically reset potential bugs: hw/usb/redirect.c: can_read handler checks for dev-parser != NULL, which may be true if CLOSED BH has not been executed yet. In the past, OPENED quiesced outstanding CLOSED events prior to us reading client data. If it's delayed, our check may allow reads to occur even though we haven't processed the OPENED event yet, and when we do finally get the OPENED event, our state may get reset. qtest.c: can begin session before OPENED event is processed, leading to a spurious reset of the system and irq_levels gdbstub.c: may start a gdb session prior to the machine being paused To fix these, let's just drop the BH. Since the initial reasoning for using it still applies to an extent, work around that by deferring the delivery of CHR_EVENT_OPENED until after the chardevs have been fully initialized, toward the end of qmp_chardev_add() (or some cases, qemu_chr_new_from_opts()). This defers delivery long enough that we can be assured a CharDriverState is fully initialized before CHR_EVENT_OPENED is sent. Also, rather than requiring each chardev to do an explicit open, do it automatically, and allow the small few who don't desire such behavior to suppress the OPENED-on-init behavior by setting a 'explicit_be_open' flag. We additionally add missing OPENED events for stdio backends on w32, which were previously not being issued, causing us to not recieve the banner and initial prompts for qmp/hmp. Reported-by: Stefan Priebe s.pri...@profihost.ag Cc: qemu-sta...@nongnu.org Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com I don't know if the QMP queue is the ideal queue for this patch, but I'd happily apply it if I get at least one Reviewed-by from the CC'ed people. Anthony actually added his Reviewed-by for v3, but I forgot to roll it in the commit. If you do take it in through your tree can you add that? Sorry for the bureaucracy, but I don't like to add other people's tags when the patch changes. Even when I'm sure they would be OK with the new version. Good call, v5 incoming with gtk/sdl that fixes Anthony caught Anthony, can you please add your Reviewed-by again? Thanks! --- v3-v4: * renamed 'suppress_be_open_on_init' to 'explicit_be_open' to match existing 'explicit_fe_open' flag (Hans) * added missing 'explicit_be_open' flags for spice vmc/port and msmouse backends v2-v3: * removed artifact in from v1 in backends/baum.c, test build with BRLAPI=y (Anthony) * rebased on latest origin/master v1-v2: * default to sending OPENED on backend init, add flag to suppress it (Anthony) * fix missing OPENED for stdio backends on w32 * fix missing OPENED when qemu_chr_new_from_opts() doesn't use qmp_chardev_add() * clean up/update commit message backends/baum.c |2 -- backends/msmouse.c|1 + include/sysemu/char.h |2 +- qemu-char.c | 38 +- spice-qemu-char.c |1 + ui/console.c |1 - ui/gtk.c |1 - 7 files changed, 20 insertions(+), 26 deletions(-) diff --git a/backends/baum.c b/backends/baum.c index 4cba79f..62aa784 100644 --- a/backends/baum.c +++ b/backends/baum.c @@ -611,8 +611,6 @@ CharDriverState *chr_baum_init(void) qemu_set_fd_handler(baum-brlapi_fd, baum_chr_read, NULL, baum
Re: [Qemu-devel] [PATCH v3] qemu-char: don't issue CHR_EVENT_OPEN in a BH
On Tue, Jun 04, 2013 at 11:58:12AM +0200, Hans de Goede wrote:
Hi,
On 06/03/2013 10:25 PM, Michael Roth wrote:
snip
To fix these, let's just drop the BH.
Since the initial reasoning for using it still applies to an extent,
work around that by deferring the delivery of CHR_EVENT_OPENED until
after the chardevs have been fully initialized, toward the end of
qmp_chardev_add() (or some cases, qemu_chr_new_from_opts()). This
defers delivery long enough that we can be assured a CharDriverState
is fully initialized before CHR_EVENT_OPENED is sent.
Also, rather than requiring each chardev to do an explicit open, do it
automatically, and allow the small few who don't desire such behavior to
suppress the OPENED-on-init behavior by setting a
'suppress_be_open_on_init' flag.
We additionally add missing OPENED events for stdio backends on w32,
which were previously not being issued, causing us to not recieve the
banner and initial prompts for qmp/hmp.
2 remarks:
1) We already have something very similar to suppress_be_open_on_init
on the frontend side, it is called explicit_fe_open, I suggest calling
the backend variant explicit_be_open for consistency
I think the naming is a bit confusing at first glance, but the behavior
does seem consistent with what we're trying to do for backends, so I'll
go ahead and make the change for v4.
2) It would be good to do a full audit to check if you've caught all
backends which need suppress_be_open_on_init:
a) check all callers of register_char_driver*
b) check if their open method calls qemu_chr_be_generic_open
c) if it does not they need suppress_be_open_on_init
Agreed; that was the intention here, but it looks like I missed some.
You've missed at least the spicevmc spiceport backends,
which both need suppress_be_open_on_init
Looks like we need one for qemu_chr_open_msmouse() as well. Will fix
these in v4.
Thanks!
Regards,
Hans
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
v2-v3:
* removed artifact in from v1 in backends/baum.c, test build with
BRLAPI=y (Anthony)
* rebased on latest origin/master
v1-v2:
* default to sending OPENED on backend init, add flag to suppress
it (Anthony)
* fix missing OPENED for stdio backends on w32
* fix missing OPENED when qemu_chr_new_from_opts() doesn't use
qmp_chardev_add()
* clean up/update commit message
backends/baum.c |2 --
include/sysemu/char.h |2 +-
qemu-char.c | 38 +-
ui/console.c |1 -
ui/gtk.c |1 -
5 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/backends/baum.c b/backends/baum.c
index 4cba79f..62aa784 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -611,8 +611,6 @@ CharDriverState *chr_baum_init(void)
qemu_set_fd_handler(baum-brlapi_fd, baum_chr_read, NULL, baum);
-qemu_chr_be_generic_open(chr);
-
return chr;
fail:
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 5e42c90..b0ae749 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -70,13 +70,13 @@ struct CharDriverState {
void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
void *opaque;
-int idle_tag;
char *label;
char *filename;
int be_open;
int fe_open;
int explicit_fe_open;
int avail_connections;
+bool suppress_be_open_on_init;
QemuOpts *opts;
QTAILQ_ENTRY(CharDriverState) next;
};
diff --git a/qemu-char.c b/qemu-char.c
index d04b429..8043f86 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -110,19 +110,9 @@ void qemu_chr_be_event(CharDriverState *s, int event)
s-chr_event(s-handler_opaque, event);
}
-static gboolean qemu_chr_be_generic_open_bh(gpointer opaque)
-{
-CharDriverState *s = opaque;
-qemu_chr_be_event(s, CHR_EVENT_OPENED);
-s-idle_tag = 0;
-return FALSE;
-}
-
void qemu_chr_be_generic_open(CharDriverState *s)
{
-if (s-idle_tag == 0) {
-s-idle_tag = g_idle_add(qemu_chr_be_generic_open_bh, s);
-}
+qemu_chr_be_event(s, CHR_EVENT_OPENED);
}
int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
@@ -247,6 +237,7 @@ static CharDriverState *qemu_chr_open_null(void)
chr = g_malloc0(sizeof(CharDriverState));
chr-chr_write = null_chr_write;
+chr-suppress_be_open_on_init = true;
return chr;
}
@@ -504,9 +495,6 @@ static CharDriverState
*qemu_chr_open_mux(CharDriverState *drv)
/* Frontend guest-open / -close notification is not support with muxes
*/
chr-chr_set_fe_open = NULL;
-/* Muxes are always open on creation */
-qemu_chr_be_generic_open(chr);
-
return chr;
Re: [Qemu-devel] [PATCH v2] qemu-char: don't issue CHR_EVENT_OPEN in a BH
On Mon, Jun 03, 2013 at 12:17:00PM -0500, Anthony Liguori wrote:
Michael Roth mdr...@linux.vnet.ibm.com writes:
When CHR_EVENT_OPEN was initially added, it was CHR_EVENT_RESET,
and it was issued as a bottom-half:
86e94dea5b740dad65446c857f6959eae43e0ba6
Which we basically used to print out a greeting/prompt for the
monitor.
AFAICT the only reason this was ever done in a BH was because in
some cases we'd modify the chr_write handler for a new chardev
backend *after* the site where we issued the reset (see:
86e94d:qemu_chr_open_stdio())
At some point this event was renamed to CHR_EVENT_OPEN, and we've
maintained the use of this BH ever since.
However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule
the BH via g_idle_add(), which is causing events to sometimes be
delivered after we've already begun processing data from backends,
leading to:
known bugs:
QMP:
session negotation resets with OPEN event, in some cases this
is causing new sessions to get sporadically reset
potential bugs:
hw/usb/redirect.c:
can_read handler checks for dev-parser != NULL, which may be
true if CLOSED BH has not been executed yet. In the past, OPENED
quiesced outstanding CLOSED events prior to us reading client
data. If it's delayed, our check may allow reads to occur even
though we haven't processed the OPENED event yet, and when we
do finally get the OPENED event, our state may get reset.
qtest.c:
can begin session before OPENED event is processed, leading to
a spurious reset of the system and irq_levels
gdbstub.c:
may start a gdb session prior to the machine being paused
To fix these, let's just drop the BH.
Since the initial reasoning for using it still applies to an extent,
work around that be deferring the delivery of CHR_EVENT_OPENED until
after the chardevs have been fully initialized, toward the end of
qmp_chardev_add() (or some cases, qemu_chr_new_from_opts()). This
defers delivery long enough that we can be assured a CharDriverState
is fully initialized before CHR_EVENT_OPENED is sent.
Also, rather than requiring each chardev to do an explicit open, do it
automatically, and allow the small few who don't desire such behavior to
suppress the OPENED on init behavior by setting a
'supress_be_open_on_init' flag.
We additionally add missing OPENED events for stdio backends on w32,
which were previously not being issued, causing us to not recieve the
banner and initial prompts for qmp/hmp.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
v1-v2:
* default to sending OPENED on backend init, add flag to suppress
it (Anthony)
* fix missing OPENED for stdio backends on w32
* fix missing OPENED when qemu_chr_new_from_opts() doesn't use
qmp_chardev_add()
* clean up/update commit message
backends/baum.c |2 +-
include/sysemu/char.h |2 +-
qemu-char.c | 38 ++
ui/console.c |1 -
ui/gtk.c |1 -
5 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/backends/baum.c b/backends/baum.c
index 4cba79f..8384ef2 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -611,7 +611,7 @@ CharDriverState *chr_baum_init(void)
qemu_set_fd_handler(baum-brlapi_fd, baum_chr_read, NULL, baum);
-qemu_chr_be_generic_open(chr);
+chr-be_open_on_init = true;
A carry over from the last patch... baum is installed on ccnode4 if you
want to test the patch there to make sure you have everything.
Shoot, thanks, I'll give it a whirl on ccnode4
I like this very a lot more. Will give my Reviewed-by on v3.
Regards,
Anthony Liguori
return chr;
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 5e42c90..b0ae749 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -70,13 +70,13 @@ struct CharDriverState {
void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
void *opaque;
-int idle_tag;
char *label;
char *filename;
int be_open;
int fe_open;
int explicit_fe_open;
int avail_connections;
+bool suppress_be_open_on_init;
QemuOpts *opts;
QTAILQ_ENTRY(CharDriverState) next;
};
diff --git a/qemu-char.c b/qemu-char.c
index 4f8382e..3071ca1 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -110,19 +110,9 @@ void qemu_chr_be_event(CharDriverState *s, int event)
s-chr_event(s-handler_opaque, event);
}
-static gboolean qemu_chr_be_generic_open_bh(gpointer opaque)
-{
-CharDriverState *s = opaque;
-qemu_chr_be_event(s, CHR_EVENT_OPENED);
-s-idle_tag = 0;
-
Re: [Qemu-devel] [PATCH 2/2] MAINTAINERS: split Monitor (QMP/HMP) entry
On Mon, Jun 03, 2013 at 03:54:48PM -0400, Luiz Capitulino wrote: On Mon, 03 Jun 2013 21:13:30 +0200 Paolo Bonzini pbonz...@redhat.com wrote: Il 03/06/2013 19:17, Luiz Capitulino ha scritto: This entry doesn't reflect reality for a few years now. This commit splits it into Human Monitor (HMP), QAPI and QMP. Markus is dropped as a maintainer. This is what we have been for the last few years. Also, it's going to help me to offload some of this work to someone else in the near future. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- MAINTAINERS | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 432185a..45f2e68 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -685,11 +685,12 @@ M: Anthony Liguori aligu...@us.ibm.com S: Supported F: vl.c -Monitor (QMP/HMP) +Human Monitor (HMP) M: Luiz Capitulino lcapitul...@redhat.com -M: Markus Armbruster arm...@redhat.com S: Supported F: monitor.c +F: hmp.c +F: hmp-commands.hx Network device layer M: Anthony Liguori aligu...@us.ibm.com @@ -706,6 +707,11 @@ F: nbd.* F: qemu-nbd.c T: git git://github.com/bonzini/qemu.git nbd-next +QAPI +M: Luiz Capitulino lcapitul...@redhat.com Mike Roth too, perhaps? I'd like to, but last time I asked he sort of refused :) Michael, can I add you? Haha, well, I think the discussion back then was whether I should send QAPI pulls, and I was of the mindset that we should keep sending those through the QMP tree since most QAPI changes were in support of QMP. According to the MAINTAINERS documentation though, M: is meant to be a set of names that developers can consult when they have a question about a particular subset and also to provide a set of names to be CC'd when submitting a patch to obtain appropriate review., so if that's the case I'd be happy to add my name and help in that regard.
Re: [Qemu-devel] [PATCH] qemu-char: don't issue CHR_EVENT_OPEN in a BH
On Thu, May 30, 2013 at 11:55:56AM -0500, Anthony Liguori wrote:
Michael Roth mdr...@linux.vnet.ibm.com writes:
When CHR_EVENT_OPEN was initially added, it was CHR_EVENT_RESET, and
it was issued as a bottom-half:
86e94dea5b740dad65446c857f6959eae43e0ba6
AFAICT the only reason this was ever done in a BH was because it was
initially used to to issue a CHR_EVENT_RESET when we initialized the
monitor,
It was specifically added so that we could redisplay the monitor
banner. Because the event was emitted in open(), if we tried to
redisplay the monitor banner in the callback, the device would be in a
partially initialized state and badness would ensue. The BH was there
to ensure the CharDriverState was fully initialized before firing the
callback.
and we would in some cases modify the chr_write handler for
a new chardev backend *after* the site where we issued the reset
(see: 86e94d:qemu_chr_open_stdio())
So we executed the reset in a BH to ensure the chardev was fully
initialized before we executed the CHR_EVENT_RESET handler (which
generally involved printing out a greeting/prompt for the monitor).
At some point this event was renamed to CHR_EVENT_OPEN, and we've
maintained the use of this BH ever since.
However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule
the BH via g_idle_add(), which is causing events to sometimes be
delivered after we've already begun processing data from backends,
leading to:
known bugs:
QMP:
session negotation resets with OPEN event, in some cases this
is causing new sessions to get sporadically reset
potential bugs:
hw/usb/redirect.c:
can_read handler checks for dev-parser != NULL, which may be
true if CLOSED BH has not been executed yet. In the past, OPENED
quiesced outstanding CLOSED events prior to us reading client
data. If it's delayed, our check may allow reads to occur even
though we haven't processed the OPENED event yet, and when we
do finally get the OPENED event, our state may get reset.
qtest.c:
can begin session before OPENED event is processed, leading to
a spurious reset of the system and irq_levels
gdbstub.c:
may start a gdb session prior to the machine being paused
To fix these, let's just drop the BH.
Since the initial reasoning for using it still applies to an extent,
work around that be deferring the delivery of CHR_EVENT_OPENED until
after the chardevs have been fully initialized by setting a
'be_open_on_init' flag that gets checked toward the end of
qmp_chardev_add(). This defers delivery long enough that we can
be assured a CharDriverState is fully initialized before
CHR_EVENT_OPENED is sent.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
backends/baum.c |2 +-
include/sysemu/char.h |2 +-
qemu-char.c | 29 ++---
ui/console.c |2 +-
ui/gtk.c |2 +-
5 files changed, 14 insertions(+), 23 deletions(-)
diff --git a/backends/baum.c b/backends/baum.c
index 4cba79f..8384ef2 100644
snip
@@ -3803,6 +3791,9 @@ ChardevReturn *qmp_chardev_add(const char *id,
ChardevBackend *backend,
chr-label = g_strdup(id);
chr-avail_connections =
(backend-kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX : 1;
+if (chr-be_open_on_init) {
+qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+}
Why does this need to be called conditionally? Could we drop
be_open_on_init and just call this unconditionally here?
We have a couple instances where we don't immediately set the backend to
an open state on init. The main one is socket backends, where where we
don't send the OPENED event until a client connects.
Regards,
Anthony Liguori
Re: [Qemu-devel] [PATCH] qemu-char: don't issue CHR_EVENT_OPEN in a BH
On Thu, May 30, 2013 at 02:35:37PM -0500, Anthony Liguori wrote:
mdroth mdr...@linux.vnet.ibm.com writes:
On Thu, May 30, 2013 at 11:55:56AM -0500, Anthony Liguori wrote:
Michael Roth mdr...@linux.vnet.ibm.com writes:
When CHR_EVENT_OPEN was initially added, it was CHR_EVENT_RESET, and
it was issued as a bottom-half:
86e94dea5b740dad65446c857f6959eae43e0ba6
AFAICT the only reason this was ever done in a BH was because it was
initially used to to issue a CHR_EVENT_RESET when we initialized the
monitor,
It was specifically added so that we could redisplay the monitor
banner. Because the event was emitted in open(), if we tried to
redisplay the monitor banner in the callback, the device would be in a
partially initialized state and badness would ensue. The BH was there
to ensure the CharDriverState was fully initialized before firing the
callback.
and we would in some cases modify the chr_write handler for
a new chardev backend *after* the site where we issued the reset
(see: 86e94d:qemu_chr_open_stdio())
So we executed the reset in a BH to ensure the chardev was fully
initialized before we executed the CHR_EVENT_RESET handler (which
generally involved printing out a greeting/prompt for the monitor).
At some point this event was renamed to CHR_EVENT_OPEN, and we've
maintained the use of this BH ever since.
However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule
the BH via g_idle_add(), which is causing events to sometimes be
delivered after we've already begun processing data from backends,
leading to:
known bugs:
QMP:
session negotation resets with OPEN event, in some cases this
is causing new sessions to get sporadically reset
potential bugs:
hw/usb/redirect.c:
can_read handler checks for dev-parser != NULL, which may be
true if CLOSED BH has not been executed yet. In the past, OPENED
quiesced outstanding CLOSED events prior to us reading client
data. If it's delayed, our check may allow reads to occur even
though we haven't processed the OPENED event yet, and when we
do finally get the OPENED event, our state may get reset.
qtest.c:
can begin session before OPENED event is processed, leading to
a spurious reset of the system and irq_levels
gdbstub.c:
may start a gdb session prior to the machine being paused
To fix these, let's just drop the BH.
Since the initial reasoning for using it still applies to an extent,
work around that be deferring the delivery of CHR_EVENT_OPENED until
after the chardevs have been fully initialized by setting a
'be_open_on_init' flag that gets checked toward the end of
qmp_chardev_add(). This defers delivery long enough that we can
be assured a CharDriverState is fully initialized before
CHR_EVENT_OPENED is sent.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
backends/baum.c |2 +-
include/sysemu/char.h |2 +-
qemu-char.c | 29 ++---
ui/console.c |2 +-
ui/gtk.c |2 +-
5 files changed, 14 insertions(+), 23 deletions(-)
diff --git a/backends/baum.c b/backends/baum.c
index 4cba79f..8384ef2 100644
snip
@@ -3803,6 +3791,9 @@ ChardevReturn *qmp_chardev_add(const char *id,
ChardevBackend *backend,
chr-label = g_strdup(id);
chr-avail_connections =
(backend-kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX : 1;
+if (chr-be_open_on_init) {
+qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+}
Why does this need to be called conditionally? Could we drop
be_open_on_init and just call this unconditionally here?
We have a couple instances where we don't immediately set the backend to
an open state on init.
Would it make more sense to mark those since they are less common?
It's less code, but emitting an event on device init seems like it
should be more of an opt-in type of thing, IMO. I'm fine with either
approach though.
Regards,
Anthony Liguori
The main one is socket backends, where where we
don't send the OPENED event until a client connects.
Regards,
Anthony Liguori
Re: [Qemu-devel] [PATCH] qemu-char: don't issue CHR_EVENT_OPEN in a BH
On Thu, May 30, 2013 at 03:24:14PM -0500, Anthony Liguori wrote:
mdroth mdr...@linux.vnet.ibm.com writes:
On Thu, May 30, 2013 at 02:35:37PM -0500, Anthony Liguori wrote:
mdroth mdr...@linux.vnet.ibm.com writes:
On Thu, May 30, 2013 at 11:55:56AM -0500, Anthony Liguori wrote:
Michael Roth mdr...@linux.vnet.ibm.com writes:
When CHR_EVENT_OPEN was initially added, it was CHR_EVENT_RESET, and
it was issued as a bottom-half:
86e94dea5b740dad65446c857f6959eae43e0ba6
AFAICT the only reason this was ever done in a BH was because it was
initially used to to issue a CHR_EVENT_RESET when we initialized the
monitor,
It was specifically added so that we could redisplay the monitor
banner. Because the event was emitted in open(), if we tried to
redisplay the monitor banner in the callback, the device would be in a
partially initialized state and badness would ensue. The BH was there
to ensure the CharDriverState was fully initialized before firing the
callback.
and we would in some cases modify the chr_write handler for
a new chardev backend *after* the site where we issued the reset
(see: 86e94d:qemu_chr_open_stdio())
So we executed the reset in a BH to ensure the chardev was fully
initialized before we executed the CHR_EVENT_RESET handler (which
generally involved printing out a greeting/prompt for the monitor).
At some point this event was renamed to CHR_EVENT_OPEN, and we've
maintained the use of this BH ever since.
However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule
the BH via g_idle_add(), which is causing events to sometimes be
delivered after we've already begun processing data from backends,
leading to:
known bugs:
QMP:
session negotation resets with OPEN event, in some cases this
is causing new sessions to get sporadically reset
potential bugs:
hw/usb/redirect.c:
can_read handler checks for dev-parser != NULL, which may be
true if CLOSED BH has not been executed yet. In the past, OPENED
quiesced outstanding CLOSED events prior to us reading client
data. If it's delayed, our check may allow reads to occur even
though we haven't processed the OPENED event yet, and when we
do finally get the OPENED event, our state may get reset.
qtest.c:
can begin session before OPENED event is processed, leading to
a spurious reset of the system and irq_levels
gdbstub.c:
may start a gdb session prior to the machine being paused
To fix these, let's just drop the BH.
Since the initial reasoning for using it still applies to an extent,
work around that be deferring the delivery of CHR_EVENT_OPENED until
after the chardevs have been fully initialized by setting a
'be_open_on_init' flag that gets checked toward the end of
qmp_chardev_add(). This defers delivery long enough that we can
be assured a CharDriverState is fully initialized before
CHR_EVENT_OPENED is sent.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
backends/baum.c |2 +-
include/sysemu/char.h |2 +-
qemu-char.c | 29 ++---
ui/console.c |2 +-
ui/gtk.c |2 +-
5 files changed, 14 insertions(+), 23 deletions(-)
diff --git a/backends/baum.c b/backends/baum.c
index 4cba79f..8384ef2 100644
snip
@@ -3803,6 +3791,9 @@ ChardevReturn *qmp_chardev_add(const char *id,
ChardevBackend *backend,
chr-label = g_strdup(id);
chr-avail_connections =
(backend-kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX :
1;
+if (chr-be_open_on_init) {
+qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+}
Why does this need to be called conditionally? Could we drop
be_open_on_init and just call this unconditionally here?
We have a couple instances where we don't immediately set the backend to
an open state on init.
Would it make more sense to mark those since they are less common?
It's less code, but emitting an event on device init seems like it
should be more of an opt-in type of thing, IMO. I'm fine with either
approach though.
I'd prefer the other way around. There's less harm in generating an
extra event than not generating one.
Ok, I'll send a v2 shortly
Regards,
Anthony Liguori
Regards,
Anthony Liguori
The main one is socket backends, where where we
don't send the OPENED event until a client connects.
Regards,
Anthony Liguori
Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits
On Wed, May 29, 2013 at 01:32:52PM -0400, Luiz Capitulino wrote:
On Sun, 26 May 2013 22:20:58 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
With the introduction of native list types, we now have types such as
int64List where the 'value' field is not a pointer, but the actual
64-bit value.
On 32-bit architectures, this can lead to situations where 'next' field
offset in GenericList does not correspond to the 'next' field in the
types that we cast to GenericList when using the visit_next_list()
interface, causing issues when we attempt to traverse linked list
structures of these types.
To fix this, pad the 'value' field of GenericList and other
schema-defined/native *List types out to 64-bits.
This is less memory-efficient for 32-bit architectures, but allows us to
continue to rely on list-handling interfaces that target GenericList to
simply visitor implementations.
In the future we can improve efficiency by defaulting to using native C
array backends to handle list of non-pointer types, which would be more
memory efficient in itself and allow us to roll back this change.
I'm also concerned with the small complexity this change is adding.
How hard would it be to do the proper solution with arrays instead?
It's not *too* bad, we'd need patches 9-11 from here:
http://lists.gnu.org/archive/html/qemu-devel/2012-10/threads.html#05755
Along with code generation bits, and then all the unit test stuff.
I think we should be able to get it in for 1.6, but I'd rather not leave
32-bit busted in the meantime.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
include/qapi/visitor.h |5 -
scripts/qapi-types.py | 10 --
tests/test-qmp-output-visitor.c |5 -
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1fef18c..28c21d8 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,7 +18,10 @@
typedef struct GenericList
{
-void *value;
+union {
+void *value;
+uint64_t padding;
+};
struct GenericList *next;
} GenericList;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index fd42d71..ddcfed9 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -22,7 +22,10 @@ def generate_fwd_struct(name, members,
builtin_type=False):
typedef struct %(name)sList
{
-%(type)s value;
+union {
+%(type)s value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
@@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
typedef struct %(name)sList
{
-%(name)s *value;
+union {
+%(name)s *value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
diff --git a/tests/test-qmp-output-visitor.c
b/tests/test-qmp-output-visitor.c
index 0942a41..b2fa9a7 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -295,7 +295,10 @@ static void
test_visitor_out_struct_errors(TestOutputVisitorData *data,
typedef struct TestStructList
{
-TestStruct *value;
+union {
+TestStruct *value;
+uint64_t padding;
+};
struct TestStructList *next;
} TestStructList;
Re: [Qemu-devel] [PATCH v2] monitor: work around delayed CHR_EVENT_OPENED events
On Wed, May 29, 2013 at 01:27:33PM -0400, Luiz Capitulino wrote:
On Mon, 27 May 2013 12:59:25 -0500
mdroth mdr...@linux.vnet.ibm.com wrote:
On Mon, May 27, 2013 at 11:16:01AM -0500, Anthony Liguori wrote:
Luiz Capitulino lcapitul...@redhat.com writes:
On Sun, 26 May 2013 10:33:39 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
In the past, CHR_EVENT_OPENED events were emitted via a pre-expired
QEMUTimer. Due to timers being processing at the tail end of each main
loop iteration, this generally meant that such events would be emitted
within the same main loop iteration, prior any client data being read
by tcp/unix socket server backends.
With 9f939df955a4152aad69a19a77e0898631bb2c18, this was switched to
a bottom-half that is registered via g_idle_add(). This makes it
likely that the event won't be sent until a subsequent iteration, and
also adds the possibility that such events will race with the
processing of client data.
In cases where we rely on the CHR_EVENT_OPENED event being delivered
prior to any client data being read, this may cause unexpected
behavior.
In the case of a QMP monitor session using a socket backend, the
delayed
processing of the CHR_EVENT_OPENED event can lead to a situation where
a previous session, where 'qmp_capabilities' has already processed, can
cause the rejection of 'qmp_capabilities' for a subsequent session,
since we reset capabilities in response to CHR_EVENT_OPENED, which may
not yet have been delivered. This can be reproduced with the following
command, generally within 50 or so iterations:
mdroth@loki:~$ cat cmds
{'execute':'qmp_capabilities'}
{'execute':'query-cpus'}
mdroth@loki:~$ while true; do if socat stdio
unix-connect:/tmp/qmp.sock
cmds | grep CommandNotFound /dev/null; then echo failed; break;
else
echo ok; fi; done;
ok
ok
failed
mdroth@loki:~$
As a work-around, we reset capabilities as part of the CHR_EVENT_CLOSED
hook, which gets called as part of the GIOChannel cb associated with
the
client rather than a bottom-half, and is thus guaranteed to be
delivered
prior to accepting any subsequent client sessions.
This does not address the more general problem of whether or not there
are chardev users that rely on CHR_EVENT_OPENED being delivered prior
to
client data, and whether or not we should consider moving
CHR_EVENT_OPENED
in-band with connection establishment as a general solution, but
fixes
QMP for the time being.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Thanks for debugging this Michael. I'm going to apply your patch to the
qmp
branch because we can't let this broken (esp. in -stable) but this is
papering
over the real bug...
Do we really need OPENED to happen in a bottom half? Shouldn't the open
event handlers register the callback instead if they need it?
I think that's the more general fix, but wasn't sure if there were
historical reasons why we didn't do this in the first place.
Looking at it more closely it does seem like we need a general fix
sooner rather than later though, and I don't see any reason why we can't
move BH registration into the actual OPENED hooks as you suggest:
hw/usb/ccid-card-passthru.c
- currently affected? no
debug hook only
- needs OPENED BH? no
hw/usb/redirect.c
- currently affected? yes
can_read handler checks for dev-parser != NULL, which may be
true if CLOSED BH has been executed yet. In the past, OPENED
quiesced outstanding CLOSED events prior to us reading client data.
- need OPENED BH? possibly
seems to only be needed by CLOSED events, which can be issued by
USBDevice, so we do teardown/usb_detach in BH. For OPENED, this
may not apply. if we do issue a BH, we'd need to modify can_read
handler to avoid race.
hw/usb/dev-serial.c
- currently affected? no
can_read checks for dev.attached != NULL. set to NULL
synchronously in CLOSED, will not accept reads until OPENED gets
called and sets dev.attached
- need opened BH? no
hw/char/sclpconsole.c
- currently affected? no
guarded by can_read handler
- need opened BH? no
hw/char/ipoctal232.c
- currently affected? no
debug hook only
- need opened BH? no
hw/char/virtio-console.c
- currently affected? no
OPENED/CLOSED map to vq events handled asynchronously. can_read
checks for guest_connected state rather than anything set by OPENED
- need opened BH? no
qtest.c
- currently affected? yes
can_read handler doesn't check for qtest_opened == true, can read
data before OPENED event
Re: [Qemu-devel] [PATCH v2] monitor: work around delayed CHR_EVENT_OPENED events
On Wed, May 29, 2013 at 01:27:33PM -0400, Luiz Capitulino wrote:
On Mon, 27 May 2013 12:59:25 -0500
mdroth mdr...@linux.vnet.ibm.com wrote:
On Mon, May 27, 2013 at 11:16:01AM -0500, Anthony Liguori wrote:
Luiz Capitulino lcapitul...@redhat.com writes:
On Sun, 26 May 2013 10:33:39 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
In the past, CHR_EVENT_OPENED events were emitted via a pre-expired
QEMUTimer. Due to timers being processing at the tail end of each main
loop iteration, this generally meant that such events would be emitted
within the same main loop iteration, prior any client data being read
by tcp/unix socket server backends.
With 9f939df955a4152aad69a19a77e0898631bb2c18, this was switched to
a bottom-half that is registered via g_idle_add(). This makes it
likely that the event won't be sent until a subsequent iteration, and
also adds the possibility that such events will race with the
processing of client data.
In cases where we rely on the CHR_EVENT_OPENED event being delivered
prior to any client data being read, this may cause unexpected
behavior.
In the case of a QMP monitor session using a socket backend, the
delayed
processing of the CHR_EVENT_OPENED event can lead to a situation where
a previous session, where 'qmp_capabilities' has already processed, can
cause the rejection of 'qmp_capabilities' for a subsequent session,
since we reset capabilities in response to CHR_EVENT_OPENED, which may
not yet have been delivered. This can be reproduced with the following
command, generally within 50 or so iterations:
mdroth@loki:~$ cat cmds
{'execute':'qmp_capabilities'}
{'execute':'query-cpus'}
mdroth@loki:~$ while true; do if socat stdio
unix-connect:/tmp/qmp.sock
cmds | grep CommandNotFound /dev/null; then echo failed; break;
else
echo ok; fi; done;
ok
ok
failed
mdroth@loki:~$
As a work-around, we reset capabilities as part of the CHR_EVENT_CLOSED
hook, which gets called as part of the GIOChannel cb associated with
the
client rather than a bottom-half, and is thus guaranteed to be
delivered
prior to accepting any subsequent client sessions.
This does not address the more general problem of whether or not there
are chardev users that rely on CHR_EVENT_OPENED being delivered prior
to
client data, and whether or not we should consider moving
CHR_EVENT_OPENED
in-band with connection establishment as a general solution, but
fixes
QMP for the time being.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Thanks for debugging this Michael. I'm going to apply your patch to the
qmp
branch because we can't let this broken (esp. in -stable) but this is
papering
over the real bug...
Do we really need OPENED to happen in a bottom half? Shouldn't the open
event handlers register the callback instead if they need it?
I think that's the more general fix, but wasn't sure if there were
historical reasons why we didn't do this in the first place.
Looking at it more closely it does seem like we need a general fix
sooner rather than later though, and I don't see any reason why we can't
move BH registration into the actual OPENED hooks as you suggest:
hw/usb/ccid-card-passthru.c
- currently affected? no
debug hook only
- needs OPENED BH? no
hw/usb/redirect.c
- currently affected? yes
can_read handler checks for dev-parser != NULL, which may be
true if CLOSED BH has been executed yet. In the past, OPENED
quiesced outstanding CLOSED events prior to us reading client data.
- need OPENED BH? possibly
seems to only be needed by CLOSED events, which can be issued by
USBDevice, so we do teardown/usb_detach in BH. For OPENED, this
may not apply. if we do issue a BH, we'd need to modify can_read
handler to avoid race.
hw/usb/dev-serial.c
- currently affected? no
can_read checks for dev.attached != NULL. set to NULL
synchronously in CLOSED, will not accept reads until OPENED gets
called and sets dev.attached
- need opened BH? no
hw/char/sclpconsole.c
- currently affected? no
guarded by can_read handler
- need opened BH? no
hw/char/ipoctal232.c
- currently affected? no
debug hook only
- need opened BH? no
hw/char/virtio-console.c
- currently affected? no
OPENED/CLOSED map to vq events handled asynchronously. can_read
checks for guest_connected state rather than anything set by OPENED
- need opened BH? no
qtest.c
- currently affected? yes
can_read handler doesn't check for qtest_opened == true, can read
data before OPENED event
Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits
On Mon, May 27, 2013 at 06:38:35AM +0200, Stefan Weil wrote:
Am 27.05.2013 05:20, schrieb Michael Roth:
With the introduction of native list types, we now have types such as
int64List where the 'value' field is not a pointer, but the actual
64-bit value.
On 32-bit architectures, this can lead to situations where 'next' field
offset in GenericList does not correspond to the 'next' field in the
types that we cast to GenericList when using the visit_next_list()
interface, causing issues when we attempt to traverse linked list
structures of these types.
To fix this, pad the 'value' field of GenericList and other
schema-defined/native *List types out to 64-bits.
This is less memory-efficient for 32-bit architectures, but allows us to
continue to rely on list-handling interfaces that target GenericList to
simply visitor implementations.
In the future we can improve efficiency by defaulting to using native C
array backends to handle list of non-pointer types, which would be more
memory efficient in itself and allow us to roll back this change.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
include/qapi/visitor.h |5 -
scripts/qapi-types.py | 10 --
tests/test-qmp-output-visitor.c |5 -
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1fef18c..28c21d8 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,7 +18,10 @@
typedef struct GenericList
{
-void *value;
+union {
+void *value;
+uint64_t padding;
+};
struct GenericList *next;
} GenericList;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index fd42d71..ddcfed9 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -22,7 +22,10 @@ def generate_fwd_struct(name, members,
builtin_type=False):
typedef struct %(name)sList
{
-%(type)s value;
+union {
+%(type)s value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
@@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
typedef struct %(name)sList
{
-%(name)s *value;
+union {
+%(name)s *value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
diff --git a/tests/test-qmp-output-visitor.c
b/tests/test-qmp-output-visitor.c
index 0942a41..b2fa9a7 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -295,7 +295,10 @@ static void
test_visitor_out_struct_errors(TestOutputVisitorData *data,
typedef struct TestStructList
{
-TestStruct *value;
+union {
+TestStruct *value;
+uint64_t padding;
+};
struct TestStructList *next;
} TestStructList;
Looks good. Would reordering of value, next work, too
(without memory overhead for 32 bit systems)?
typedef struct GenericList
{
struct GenericList *next;
void *value;
} GenericList;
typedef struct %(name)sList
{
struct %(name)sList *next;
%(type)s value;
} %(name)sList;
Hmm, that should fix the issue as far as casting goes, but there's also
the issue of allocating memory:
...
It looks like memory allocation (g_malloc0) for GenericList
was also wrong in the old code (this is fixed with your patch).
Yup, input visitors are expected to allocate memory for storage of the
lists, and currently do so based on sizeof(GenericList), so we'd still
need to address that problem if we took the above approach. It wouldn't
take much: we'd probably modify visit_start_list() to accept an
additional argument for how large a list container we need it to
allocate.
But this is kind of a 1-off thing specifically for non-pointer list
types, and the ones in-tree, (u)int{8,16,32,64}/bool/double, should be
the only ones we ever need, so i'd like to avoid complicating the qapi
interface/visitor implementations to support them, especially since I
plan on switching them to using an C array backend in the future instead
of linked lists, which should address the memory efficiency issues.
Re: [Qemu-devel] [PATCH v2] monitor: work around delayed CHR_EVENT_OPENED events
On Mon, May 27, 2013 at 11:16:01AM -0500, Anthony Liguori wrote:
Luiz Capitulino lcapitul...@redhat.com writes:
On Sun, 26 May 2013 10:33:39 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
In the past, CHR_EVENT_OPENED events were emitted via a pre-expired
QEMUTimer. Due to timers being processing at the tail end of each main
loop iteration, this generally meant that such events would be emitted
within the same main loop iteration, prior any client data being read
by tcp/unix socket server backends.
With 9f939df955a4152aad69a19a77e0898631bb2c18, this was switched to
a bottom-half that is registered via g_idle_add(). This makes it
likely that the event won't be sent until a subsequent iteration, and
also adds the possibility that such events will race with the
processing of client data.
In cases where we rely on the CHR_EVENT_OPENED event being delivered
prior to any client data being read, this may cause unexpected behavior.
In the case of a QMP monitor session using a socket backend, the delayed
processing of the CHR_EVENT_OPENED event can lead to a situation where
a previous session, where 'qmp_capabilities' has already processed, can
cause the rejection of 'qmp_capabilities' for a subsequent session,
since we reset capabilities in response to CHR_EVENT_OPENED, which may
not yet have been delivered. This can be reproduced with the following
command, generally within 50 or so iterations:
mdroth@loki:~$ cat cmds
{'execute':'qmp_capabilities'}
{'execute':'query-cpus'}
mdroth@loki:~$ while true; do if socat stdio unix-connect:/tmp/qmp.sock
cmds | grep CommandNotFound /dev/null; then echo failed; break; else
echo ok; fi; done;
ok
ok
failed
mdroth@loki:~$
As a work-around, we reset capabilities as part of the CHR_EVENT_CLOSED
hook, which gets called as part of the GIOChannel cb associated with the
client rather than a bottom-half, and is thus guaranteed to be delivered
prior to accepting any subsequent client sessions.
This does not address the more general problem of whether or not there
are chardev users that rely on CHR_EVENT_OPENED being delivered prior to
client data, and whether or not we should consider moving CHR_EVENT_OPENED
in-band with connection establishment as a general solution, but fixes
QMP for the time being.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Thanks for debugging this Michael. I'm going to apply your patch to the qmp
branch because we can't let this broken (esp. in -stable) but this is
papering
over the real bug...
Do we really need OPENED to happen in a bottom half? Shouldn't the open
event handlers register the callback instead if they need it?
I think that's the more general fix, but wasn't sure if there were
historical reasons why we didn't do this in the first place.
Looking at it more closely it does seem like we need a general fix
sooner rather than later though, and I don't see any reason why we can't
move BH registration into the actual OPENED hooks as you suggest:
hw/usb/ccid-card-passthru.c
- currently affected? no
debug hook only
- needs OPENED BH? no
hw/usb/redirect.c
- currently affected? yes
can_read handler checks for dev-parser != NULL, which may be
true if CLOSED BH has been executed yet. In the past, OPENED
quiesced outstanding CLOSED events prior to us reading client data.
- need OPENED BH? possibly
seems to only be needed by CLOSED events, which can be issued by
USBDevice, so we do teardown/usb_detach in BH. For OPENED, this
may not apply. if we do issue a BH, we'd need to modify can_read
handler to avoid race.
hw/usb/dev-serial.c
- currently affected? no
can_read checks for dev.attached != NULL. set to NULL
synchronously in CLOSED, will not accept reads until OPENED gets
called and sets dev.attached
- need opened BH? no
hw/char/sclpconsole.c
- currently affected? no
guarded by can_read handler
- need opened BH? no
hw/char/ipoctal232.c
- currently affected? no
debug hook only
- need opened BH? no
hw/char/virtio-console.c
- currently affected? no
OPENED/CLOSED map to vq events handled asynchronously. can_read
checks for guest_connected state rather than anything set by OPENED
- need opened BH? no
qtest.c
- currently affected? yes
can_read handler doesn't check for qtest_opened == true, can read
data before OPENED event is processed
- need opened BH? no
monitor.c
- current affected? yes
negotiated session state can temporarilly leak into a new session
- need opened BH? no
gdbstub.c
- currently affected? yes
can fail to pause machine prior to starting gdb session
- need opened BH? no
So we have a number of cases that can be fixed by avoiding the use of
the generic BH, and only 1 possible cause where we might need a
device
Re: [Qemu-devel] make check breakage on 32 bit hosts
On Sun, May 26, 2013 at 12:00:57PM +, Blue Swirl wrote: I get this on i386 chroot for make check: GTESTER tests/test-qmp-output-visitor ** ERROR:/src/qemu/tests/test-qmp-output-visitor.c:595:check_native_list: assertion failed: (tmp) GTester: last random seed: R02S559792e7c8d0762d9a2ee153fba8896c ** Tests case looks correct, problem seems to be that we cast list node to GenericList, which expects the 'value' field to be a pointer type. With native lists types these are in some cases non-pointer types, like intList, where 'value' is an int64_t. On 32-bit archs this cast leads to use uses incorrect offset when trying to traverse the list. Don't see a way to fix this outside of making the code generators do a bit of massaging for these cases rather than a cast. Looking at it now, but might not be able to send a patch till later.
Re: [Qemu-devel] [Qemu-stable] qmp commands get rejected
On Sun, May 26, 2013 at 05:13:36PM +0200, Stefan Priebe wrote:
Am 26.05.2013 03:23, schrieb mdroth:
On Sat, May 25, 2013 at 01:09:50PM +0200, Stefan Priebe wrote:
Am 25.05.2013 00:32, schrieb mdroth:
On Sat, May 25, 2013 at 12:12:22AM +0200, Stefan Priebe wrote:
Am 25.05.2013 00:09, schrieb mdroth:
I would try to create a small example script.
I use qmp-shell and other little scripts very often.
Am this be due to the fact that I don't wait for the welcome banner
right now?
If you're not reading from the socket, then you'll get the banner back
when
you read your first response. But qom-set shouldn't fail because of
that.
I can workaround it by adding this patch:
diff --git a/monitor.c b/monitor.c
index 62aaebe..9997520 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4239,7 +4239,8 @@ static int monitor_can_read(void *opaque)
static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
{
int is_cap = compare_cmd(cmd_name, qmp_capabilities);
-return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
+//return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
+return ((is_cap 0) ? 0 : (qmp_cmd_mode(mon) ? is_cap : !is_cap));
}
I think this is unrelated to your original issue. If you issue
'qmp_capabilities' command more than once you will get CommandNotFound,
and that behavior seems to be present even with v1.3.0. This patch seems
to be masking the problem you're having (which seems to be state from
previous monitor sessions/connections leaking into subsequent ones).
That sounds reasonable. I'm using proxmox / PVE which does a lot of
qmp queries in the background. So i might see situations where X
connections in parallel do qmp queries.
It's possible the GSource-based mechanism for handling I/O for chardev
backends is causing a difference in behavior. Still not sure exactly
what's going on though.
Can i revert some patches to test?
I think somewhere prior to this one should be enough to test:
2ea5a7af7bfa576a5936400ccca4144caca9640b
YES! I used 2ea5a7af7bfa576a5936400ccca4144caca9640b~1 for my tests
and this works absoluty fine.
Turns out the real culprit was a few commits later:
9f939df955a4152aad69a19a77e0898631bb2c18
I've sent a workaround this fixes things for QMP, but we may need a more
general fix. Please give it a shot and see if it fixes your issues:
http://article.gmane.org/gmane.comp.emulators.qemu/213106
no i got again:
The command qom-set has not been found JSON Reply: {id:
21677:2, error: {class: CommandNotFound, desc: The
command qom-set has not been found}} JSON Query:
{execute:qom-set,id:21677:2,arguments:{value:2,path:machine/peripheral/balloon0,property:guest-stats-polling-interval}}
at /usr/share/perl5/PVE/QMPClient.pm line 101.
Darn, I noticed another potential race after I sent the patch. Hadn't
been able to trigger it on my end, but it might be what you're hitting.
Just sent a v2, can you give that a shot?
http://article.gmane.org/gmane.comp.emulators.qemu/213147
Stefan
Re: [Qemu-devel] make check breakage on 32 bit hosts
On Sun, May 26, 2013 at 10:26:48AM -0500, mdroth wrote: On Sun, May 26, 2013 at 12:00:57PM +, Blue Swirl wrote: I get this on i386 chroot for make check: GTESTER tests/test-qmp-output-visitor ** ERROR:/src/qemu/tests/test-qmp-output-visitor.c:595:check_native_list: assertion failed: (tmp) GTester: last random seed: R02S559792e7c8d0762d9a2ee153fba8896c ** Tests case looks correct, problem seems to be that we cast list node to GenericList, which expects the 'value' field to be a pointer type. With native lists types these are in some cases non-pointer types, like intList, where 'value' is an int64_t. On 32-bit archs this cast leads to use uses incorrect offset when trying to traverse the list. Don't see a way to fix this outside of making the code generators do a bit of massaging for these cases rather than a cast. Looking at it now, but might not be able to send a patch till later. Patch sent: http://article.gmane.org/gmane.comp.emulators.qemu/213202 Teaching code generators to be smarter didn't work out so well since visitor implementations allocate storage of list types based on GenericList, so ended up relying on padding to 64-bit instead.
Re: [Qemu-devel] [Qemu-stable] qmp commands get rejected
On Sat, May 25, 2013 at 01:09:50PM +0200, Stefan Priebe wrote:
Am 25.05.2013 00:32, schrieb mdroth:
On Sat, May 25, 2013 at 12:12:22AM +0200, Stefan Priebe wrote:
Am 25.05.2013 00:09, schrieb mdroth:
I would try to create a small example script.
I use qmp-shell and other little scripts very often.
Am this be due to the fact that I don't wait for the welcome banner
right now?
If you're not reading from the socket, then you'll get the banner back
when
you read your first response. But qom-set shouldn't fail because of
that.
I can workaround it by adding this patch:
diff --git a/monitor.c b/monitor.c
index 62aaebe..9997520 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4239,7 +4239,8 @@ static int monitor_can_read(void *opaque)
static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
{
int is_cap = compare_cmd(cmd_name, qmp_capabilities);
-return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
+//return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
+return ((is_cap 0) ? 0 : (qmp_cmd_mode(mon) ? is_cap : !is_cap));
}
I think this is unrelated to your original issue. If you issue
'qmp_capabilities' command more than once you will get CommandNotFound,
and that behavior seems to be present even with v1.3.0. This patch seems
to be masking the problem you're having (which seems to be state from
previous monitor sessions/connections leaking into subsequent ones).
That sounds reasonable. I'm using proxmox / PVE which does a lot of
qmp queries in the background. So i might see situations where X
connections in parallel do qmp queries.
It's possible the GSource-based mechanism for handling I/O for chardev
backends is causing a difference in behavior. Still not sure exactly
what's going on though.
Can i revert some patches to test?
I think somewhere prior to this one should be enough to test:
2ea5a7af7bfa576a5936400ccca4144caca9640b
YES! I used 2ea5a7af7bfa576a5936400ccca4144caca9640b~1 for my tests
and this works absoluty fine.
Turns out the real culprit was a few commits later:
9f939df955a4152aad69a19a77e0898631bb2c18
I've sent a workaround this fixes things for QMP, but we may need a more
general fix. Please give it a shot and see if it fixes your issues:
http://article.gmane.org/gmane.comp.emulators.qemu/213106
Stefan
[Qemu-devel] [ANNOUNCE] QEMU 1.4.2 Stable released
Hi everyone, I am pleased to announce that the QEMU v1.4.2 stable release is now available at: http://wiki.qemu.org/download/qemu-1.4.2.tar.bz2 The official stable-1.4 repository has also been updated to v1.4.2: http://git.qemu.org/?p=qemu-stable-1.4.git;a=summary This release contains 25 build/bug fixes, and includes an important security fix for the QEMU guest agent. This is the last planned release for the 1.4 series as we'll be moving on to supporting the recently released 1.5 series. QEMU v1.5.1 is planned for 2013-06-26. Thanks to everyone involved! 89400a8: update VERSION for 1.4.2 (Michael Roth) e85b521: ppc: do not register IABR SPR twice for 603e (Hervé Poussineau) f890185: hw/9pfs: use O_NOFOLLOW for mapped readlink operation (Aneesh Kumar K.V) 745f6c0: hw/9pfs: Fix segfault with 9p2000.u (Aneesh Kumar K.V) 0182df5: rbd: add an asynchronous flush (Josh Durgin) 7f28f0f: qemu-iotests: add tests for rebasing zero clusters (Paolo Bonzini) 45bbe1f: virtio-balloon: fix integer overflow in BALLOON_CHANGE QMP event (Luiz Capitulino) 06efdc4: qemu-timer: move timeBeginPeriod/timeEndPeriod to os-win32 (Paolo Bonzini) 0c70b5a: configure: Don't fall back to gthread coroutine backend (Brad Smith) b90fd15: usb-redir: Fix crash on migration with no client connected (Hans de Goede) 7322cb1: docs: Fix generating qemu-doc.html with texinfo 5 (Cole Robinson) 1d7723f: qga: unlink just created guest-file if fchmod() or fdopen() fails on it (Laszlo Ersek) 67b460a: qga: distinguish binary modes in guest_file_open_modes map (Laszlo Ersek) 84247bb: translate-all.c: Remove cpu_unlink_tb() (Peter Maydell) 2ebcc59: Handle CPU interrupts by inline checking of a flag (Peter Maydell) 69001b3: cpu-exec: wrap tcg_qemu_tb_exec() in a fn to restore the PC (Peter Maydell) 3accab7: tcg: Document tcg_qemu_tb_exec() and provide constants for low bit uses (Peter Maydell) 6025953: qga: set umask 0077 when daemonizing (CVE-2013-2007) (Laszlo Ersek) 93399d0: tcg/optimize: fix setcond2 optimization (Aurelien Jarno) 074dd56: target-mips: Fix accumulator arguments to gen_helper_dmult(u) (Richard Sandiford) d10d251: configure: Pick up libseccomp include path (Andreas Färber) 5613bda: virtio-ccw: Check indicators location. (Cornelia Huck) c5675a9: tap: properly initialize vhostfds (Jason Wang) e355efd: rng random backend: check for -EAGAIN errors on read (Amit Shah) 4d7f455: qdev: Fix QOM unrealize behavior (Andreas Färber) 0486c27: nbd: unlock mutex in nbd_co_send_request() error path (Stefan Hajnoczi)
Re: [Qemu-devel] [Qemu-stable] qmp commands get rejected
On Fri, May 24, 2013 at 11:37:46PM +0200, Stefan Priebe wrote:
Am 24.05.2013 17:21, schrieb Luiz Capitulino:
On Fri, 24 May 2013 16:36:26 +0200
Stefan Priebe - Profihost AG s.pri...@profihost.ag wrote:
Am 24.05.2013 um 16:02 schrieb Luiz Capitulino lcapitul...@redhat.com:
On Fri, 24 May 2013 15:57:59 +0200
Stefan Priebe - Profihost AG s.pri...@profihost.ag wrote:
Am 24.05.2013 um 15:23 schrieb Luiz Capitulino
lcapitul...@redhat.com:
On Fri, 24 May 2013 07:50:33 +0200
Stefan Priebe s.pri...@profihost.ag wrote:
Hello list,
since upgrading from qemu 1.4.1 to 1.5.0 i've problems with qmp
commands.
With Qemu 1.5 i've the following socket communication:
'{execute:qmp_capabilities,id:12125:1,arguments:{}}'
'{return: {}, id: 12125:1}'
'{execute:qom-set,id:12125:2,arguments:{value:2,path:machine/peripheral/balloon0,property:guest-stats-polling-interval}}'
'{QMP: {version: {qemu: {micro: 0, minor: 5, major: 1},
package: }, capabilities: []}}'
'{id: 12125:2, error: {class: CommandNotFound, desc:
The
command qom-set has not been found}}'
It seems that the command mode (qmp_capabilities) gets resets by the
welcome banner?
It looks like you got disconnected before qom-set was issued.
No its the same socket connection. No disconnect had happened.
Can you share more details on how those commands are being issued?
They're send through socket with a perl script. What do you need?
That perl script maybe? I can't reproduce the problem.
I would try to create a small example script.
I use qmp-shell and other little scripts very often.
Am this be due to the fact that I don't wait for the welcome banner
right now?
If you're not reading from the socket, then you'll get the banner back
when
you read your first response. But qom-set shouldn't fail because of that.
I can workaround it by adding this patch:
diff --git a/monitor.c b/monitor.c
index 62aaebe..9997520 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4239,7 +4239,8 @@ static int monitor_can_read(void *opaque)
static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
{
int is_cap = compare_cmd(cmd_name, qmp_capabilities);
-return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
+//return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
+return ((is_cap 0) ? 0 : (qmp_cmd_mode(mon) ? is_cap : !is_cap));
}
I think this is unrelated to your original issue. If you issue
'qmp_capabilities' command more than once you will get CommandNotFound,
and that behavior seems to be present even with v1.3.0. This patch seems
to be masking the problem you're having (which seems to be state from
previous monitor sessions/connections leaking into subsequent ones).
It's possible the GSource-based mechanism for handling I/O for chardev
backends is causing a difference in behavior. Still not sure exactly
what's going on though.
/*
Stefan
Re: [Qemu-devel] [Qemu-stable] qmp commands get rejected
On Sat, May 25, 2013 at 12:12:22AM +0200, Stefan Priebe wrote:
Am 25.05.2013 00:09, schrieb mdroth:
I would try to create a small example script.
I use qmp-shell and other little scripts very often.
Am this be due to the fact that I don't wait for the welcome banner
right now?
If you're not reading from the socket, then you'll get the banner back
when
you read your first response. But qom-set shouldn't fail because of that.
I can workaround it by adding this patch:
diff --git a/monitor.c b/monitor.c
index 62aaebe..9997520 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4239,7 +4239,8 @@ static int monitor_can_read(void *opaque)
static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
{
int is_cap = compare_cmd(cmd_name, qmp_capabilities);
-return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
+//return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
+return ((is_cap 0) ? 0 : (qmp_cmd_mode(mon) ? is_cap : !is_cap));
}
I think this is unrelated to your original issue. If you issue
'qmp_capabilities' command more than once you will get CommandNotFound,
and that behavior seems to be present even with v1.3.0. This patch seems
to be masking the problem you're having (which seems to be state from
previous monitor sessions/connections leaking into subsequent ones).
That sounds reasonable. I'm using proxmox / PVE which does a lot of
qmp queries in the background. So i might see situations where X
connections in parallel do qmp queries.
It's possible the GSource-based mechanism for handling I/O for chardev
backends is causing a difference in behavior. Still not sure exactly
what's going on though.
Can i revert some patches to test?
I think somewhere prior to this one should be enough to test:
2ea5a7af7bfa576a5936400ccca4144caca9640b
Stefan
Re: [Qemu-devel] (Another) 1.4.1 - 1.5.0 migration failure
On Tue, May 21, 2013 at 12:33:38PM +0100, Nicholas Thomas wrote: Hi all, Migrating from: /opt/qemu-1.4.1/bin/qemu-system-x86_64 -M pc -watchdog i6300esb -watchdog-action reset [...] to: /opt/qemu-1.5.0/bin/qemu-system-x86_64 -M pc-i440fx-1.4 -watchdog i6300esb -watchdog-action reset [...] I get: qemu: warning: error while loading state for instance 0x0 of device ':00:06.0/i6300esb_wdt' load of migration failed Perhaps a similar issue to the network device one? I'm afraid I'm not sure how to debug these... Doesn't look like it, here's a dump of the SaveStateEntry's (i just added some debug statements in vmstate_register_with_alias_id): verison: 1.4.1, se-idstr: timer, se-instance_id: 0 verison: 1.4.1, se-idstr: cpu_common, se-instance_id: 0 verison: 1.4.1, se-idstr: kvm-tpr-opt, se-instance_id: 0 verison: 1.4.1, se-idstr: apic, se-instance_id: 0 verison: 1.4.1, se-idstr: kvmclock, se-instance_id: 0 verison: 1.4.1, se-idstr: fw_cfg, se-instance_id: 0 verison: 1.4.1, se-idstr: PCIBUS, se-instance_id: 0 verison: 1.4.1, se-idstr: :00:00.0/I440FX, se-instance_id: 0 verison: 1.4.1, se-idstr: :00:01.0/PIIX3, se-instance_id: 0 verison: 1.4.1, se-idstr: i8259, se-instance_id: 0 verison: 1.4.1, se-idstr: i8259, se-instance_id: 1 verison: 1.4.1, se-idstr: ioapic, se-instance_id: 0 verison: 1.4.1, se-idstr: :00:02.0/vga, se-instance_id: 0 verison: 1.4.1, se-idstr: hpet, se-instance_id: 0 verison: 1.4.1, se-idstr: mc146818rtc, se-instance_id: 0 verison: 1.4.1, se-idstr: i8254, se-instance_id: 0 verison: 1.4.1, se-idstr: serial, se-instance_id: 0 verison: 1.4.1, se-idstr: ps2kbd, se-instance_id: 0 verison: 1.4.1, se-idstr: ps2mouse, se-instance_id: 0 verison: 1.4.1, se-idstr: pckbd, se-instance_id: 0 verison: 1.4.1, se-idstr: vmmouse, se-instance_id: 0 verison: 1.4.1, se-idstr: port92, se-instance_id: 0 verison: 1.4.1, se-idstr: dma, se-instance_id: 0 verison: 1.4.1, se-idstr: dma, se-instance_id: 1 verison: 1.4.1, se-idstr: fdc, se-instance_id: 0 verison: 1.4.1, se-idstr: :00:01.1/ide, se-instance_id: 0 verison: 1.4.1, se-idstr: i2c_bus, se-instance_id: 0 verison: 1.4.1, se-idstr: :00:01.3/piix4_pm, se-instance_id: 0 verison: 1.4.1, se-idstr: :00:06.0/i6300esb_wdt, se-instance_id: 0 version: 1.5.0, se-idstr: timer, se-instance_id: 0 version: 1.5.0, se-idstr: cpu_common, se-instance_id: 0 version: 1.5.0, se-idstr: cpu, se-instance_id: 0 version: 1.5.0, se-idstr: kvm-tpr-opt, se-instance_id: 0 version: 1.5.0, se-idstr: apic, se-instance_id: 0 version: 1.5.0, se-idstr: kvmclock, se-instance_id: 0 version: 1.5.0, se-idstr: fw_cfg, se-instance_id: 0 version: 1.5.0, se-idstr: PCIBUS, se-instance_id: 0 version: 1.5.0, se-idstr: :00:00.0/I440FX, se-instance_id: 0 version: 1.5.0, se-idstr: :00:01.0/PIIX3, se-instance_id: 0 version: 1.5.0, se-idstr: i8259, se-instance_id: 0 version: 1.5.0, se-idstr: i8259, se-instance_id: 1 version: 1.5.0, se-idstr: ioapic, se-instance_id: 0 version: 1.5.0, se-idstr: :00:02.0/vga, se-instance_id: 0 version: 1.5.0, se-idstr: hpet, se-instance_id: 0 version: 1.5.0, se-idstr: mc146818rtc, se-instance_id: 0 version: 1.5.0, se-idstr: i8254, se-instance_id: 0 version: 1.5.0, se-idstr: serial, se-instance_id: 0 version: 1.5.0, se-idstr: ps2kbd, se-instance_id: 0 version: 1.5.0, se-idstr: ps2mouse, se-instance_id: 0 version: 1.5.0, se-idstr: pckbd, se-instance_id: 0 version: 1.5.0, se-idstr: vmmouse, se-instance_id: 0 version: 1.5.0, se-idstr: port92, se-instance_id: 0 version: 1.5.0, se-idstr: dma, se-instance_id: 0 version: 1.5.0, se-idstr: dma, se-instance_id: 1 version: 1.5.0, se-idstr: fdc, se-instance_id: 0 version: 1.5.0, se-idstr: :00:01.1/ide, se-instance_id: 0 version: 1.5.0, se-idstr: i2c_bus, se-instance_id: 0 version: 1.5.0, se-idstr: :00:01.3/piix4_pm, se-instance_id: 0 version: 1.5.0, se-idstr: :00:06.0/i6300esb_wdt, se-instance_id: 0 Seems like the actual load is failing, but the VMSD hasn't changed so i'm not sure what's going on. /Nick
Re: [Qemu-devel] (Another) 1.4.1 - 1.5.0 migration failure
On Tue, May 21, 2013 at 11:55:56AM -0500, mdroth wrote:
On Tue, May 21, 2013 at 12:33:38PM +0100, Nicholas Thomas wrote:
Hi all,
Migrating from:
/opt/qemu-1.4.1/bin/qemu-system-x86_64 -M pc -watchdog i6300esb
-watchdog-action reset [...]
to:
/opt/qemu-1.5.0/bin/qemu-system-x86_64 -M pc-i440fx-1.4 -watchdog
i6300esb -watchdog-action reset [...]
I get:
qemu: warning: error while loading state for instance 0x0 of device
':00:06.0/i6300esb_wdt'
load of migration failed
Perhaps a similar issue to the network device one? I'm afraid I'm not
sure how to debug these...
Doesn't look like it, here's a dump of the SaveStateEntry's (i just
added some debug statements in vmstate_register_with_alias_id):
verison: 1.4.1, se-idstr: timer, se-instance_id: 0
verison: 1.4.1, se-idstr: cpu_common, se-instance_id: 0
verison: 1.4.1, se-idstr: kvm-tpr-opt, se-instance_id: 0
verison: 1.4.1, se-idstr: apic, se-instance_id: 0
verison: 1.4.1, se-idstr: kvmclock, se-instance_id: 0
verison: 1.4.1, se-idstr: fw_cfg, se-instance_id: 0
verison: 1.4.1, se-idstr: PCIBUS, se-instance_id: 0
verison: 1.4.1, se-idstr: :00:00.0/I440FX, se-instance_id: 0
verison: 1.4.1, se-idstr: :00:01.0/PIIX3, se-instance_id: 0
verison: 1.4.1, se-idstr: i8259, se-instance_id: 0
verison: 1.4.1, se-idstr: i8259, se-instance_id: 1
verison: 1.4.1, se-idstr: ioapic, se-instance_id: 0
verison: 1.4.1, se-idstr: :00:02.0/vga, se-instance_id: 0
verison: 1.4.1, se-idstr: hpet, se-instance_id: 0
verison: 1.4.1, se-idstr: mc146818rtc, se-instance_id: 0
verison: 1.4.1, se-idstr: i8254, se-instance_id: 0
verison: 1.4.1, se-idstr: serial, se-instance_id: 0
verison: 1.4.1, se-idstr: ps2kbd, se-instance_id: 0
verison: 1.4.1, se-idstr: ps2mouse, se-instance_id: 0
verison: 1.4.1, se-idstr: pckbd, se-instance_id: 0
verison: 1.4.1, se-idstr: vmmouse, se-instance_id: 0
verison: 1.4.1, se-idstr: port92, se-instance_id: 0
verison: 1.4.1, se-idstr: dma, se-instance_id: 0
verison: 1.4.1, se-idstr: dma, se-instance_id: 1
verison: 1.4.1, se-idstr: fdc, se-instance_id: 0
verison: 1.4.1, se-idstr: :00:01.1/ide, se-instance_id: 0
verison: 1.4.1, se-idstr: i2c_bus, se-instance_id: 0
verison: 1.4.1, se-idstr: :00:01.3/piix4_pm, se-instance_id: 0
verison: 1.4.1, se-idstr: :00:06.0/i6300esb_wdt, se-instance_id: 0
version: 1.5.0, se-idstr: timer, se-instance_id: 0
version: 1.5.0, se-idstr: cpu_common, se-instance_id: 0
version: 1.5.0, se-idstr: cpu, se-instance_id: 0
version: 1.5.0, se-idstr: kvm-tpr-opt, se-instance_id: 0
version: 1.5.0, se-idstr: apic, se-instance_id: 0
version: 1.5.0, se-idstr: kvmclock, se-instance_id: 0
version: 1.5.0, se-idstr: fw_cfg, se-instance_id: 0
version: 1.5.0, se-idstr: PCIBUS, se-instance_id: 0
version: 1.5.0, se-idstr: :00:00.0/I440FX, se-instance_id: 0
version: 1.5.0, se-idstr: :00:01.0/PIIX3, se-instance_id: 0
version: 1.5.0, se-idstr: i8259, se-instance_id: 0
version: 1.5.0, se-idstr: i8259, se-instance_id: 1
version: 1.5.0, se-idstr: ioapic, se-instance_id: 0
version: 1.5.0, se-idstr: :00:02.0/vga, se-instance_id: 0
version: 1.5.0, se-idstr: hpet, se-instance_id: 0
version: 1.5.0, se-idstr: mc146818rtc, se-instance_id: 0
version: 1.5.0, se-idstr: i8254, se-instance_id: 0
version: 1.5.0, se-idstr: serial, se-instance_id: 0
version: 1.5.0, se-idstr: ps2kbd, se-instance_id: 0
version: 1.5.0, se-idstr: ps2mouse, se-instance_id: 0
version: 1.5.0, se-idstr: pckbd, se-instance_id: 0
version: 1.5.0, se-idstr: vmmouse, se-instance_id: 0
version: 1.5.0, se-idstr: port92, se-instance_id: 0
version: 1.5.0, se-idstr: dma, se-instance_id: 0
version: 1.5.0, se-idstr: dma, se-instance_id: 1
version: 1.5.0, se-idstr: fdc, se-instance_id: 0
version: 1.5.0, se-idstr: :00:01.1/ide, se-instance_id: 0
version: 1.5.0, se-idstr: i2c_bus, se-instance_id: 0
version: 1.5.0, se-idstr: :00:01.3/piix4_pm, se-instance_id: 0
version: 1.5.0, se-idstr: :00:06.0/i6300esb_wdt, se-instance_id: 0
Seems like the actual load is failing, but the VMSD hasn't changed so
i'm not sure what's going on.
Ehhh...
static const VMStateDescription vmstate_i6300esb = {
.name = i6300esb_wdt,
.version_id = sizeof(I6300State),
.minimum_version_id = sizeof(I6300State),
.minimum_version_id_old = sizeof(I6300State),
.fields = (VMStateField []) {
VMSTATE_PCI_DEVICE(dev, I6300State),
VMSTATE_INT32(reboot_enabled, I6300State),
VMSTATE_INT32(clock_scale, I6300State),
VMSTATE_INT32(int_type, I6300State),
VMSTATE_INT32(free_run, I6300State),
VMSTATE_INT32(locked, I6300State),
VMSTATE_INT32(enabled, I6300State),
VMSTATE_TIMER(timer, I6300State),
VMSTATE_UINT32(timer1_preload, I6300State),
VMSTATE_UINT32(timer2_preload, I6300State),
VMSTATE_INT32(stage, I6300State),
VMSTATE_INT32(unlock_state, I6300State),
VMSTATE_INT32
Re: [Qemu-devel] (Another) 1.4.1 - 1.5.0 migration failure
On Tue, May 21, 2013 at 06:50:23PM +0100, Peter Maydell wrote:
On 21 May 2013 18:26, mdroth mdr...@linux.vnet.ibm.com wrote:
static const VMStateDescription vmstate_i6300esb = {
.name = i6300esb_wdt,
.version_id = sizeof(I6300State),
.minimum_version_id = sizeof(I6300State),
.minimum_version_id_old = sizeof(I6300State),
apparently minimum version ID is set the size of the device struct?
Almost certain that's the problem.
Haha, yeah, that's totally busted. Interestingly it's been
like that since 2009. I think to fix this we probably need to:
* set version_id to something fixed but larger than
the worst sizeof() has ever been for this device.
Since we have plenty of space in an int we might as
well set it to 1.
* set minimum_version_id and minimum_version_id_old to 1
[this is safe, I think, since the fields haven't ever
changed, but needs testing]
* add a big fat comment about why the weird version ID
This then brings it into line with everything else, and
the standard rules about when to bump vmstate version
and marking up new fields with first-version-present and
so on all apply as usual.
Makes sense, but apparently version IDs for incoming device state are
not allowed to exceed the destination's version, so we can't bump it
beyond the value in 1.5 without breaking migration from 1.5+ - 1.5
So I think our only option for version ID is to lock in the 1.5 value,
which seems to be 1936 (hopefully that's consistent across builds...).
But reseting minimum version ID to 1 does seem to fix 1.4 - 1.5+
thanks
-- PMM
Re: [Qemu-devel] (Another) 1.4.1 - 1.5.0 migration failure
On Tue, May 21, 2013 at 10:16:48PM +0100, Peter Maydell wrote: On 21 May 2013 21:43, mdroth mdr...@linux.vnet.ibm.com wrote: Makes sense, but apparently version IDs for incoming device state are not allowed to exceed the destination's version, so we can't bump it beyond the value in 1.5 without breaking migration from 1.5+ - 1.5 We care about backwards migration? That sounds like a pain. As a best effort at least, hence subsections and whatnot, but not always. This case for instance... So I think our only option for version ID is to lock in the 1.5 value, which seems to be 1936 (hopefully that's consistent across builds...). Yeah, I'm not convinced that's going to be consistent across builds, compilers, 64 vs 32 bit, etc etc etc. That's why I suggested a really high number. Yah, it's more important to ensure old-new. Playing guessing games about struct sizes to try to maintain new-old is likely to conflict with that, so I guess we don't have much choice here. I'll send a patch out shortly. thanks -- PMM
Re: [Qemu-devel] [PATCH 0/6] local state directory fixes for win32 qga
On Sat, May 18, 2013 at 06:31:47AM +0200, Laszlo Ersek wrote: Qouting patch 2/6: Since commit 39097daf (qemu-ga: use key-value store to avoid recycling fd handles after restart) we've relied on the state directory for the fd handles' key-value store. Even though we don't support the guest-file-* commands on win32 yet, the key-value store is written, and it's the first use of the state directory on win32. We should have a sensible default for its location. Motivated by RHBZ#962669. I've perpetrated this in the second half of a Friday-Sunday all-nighter, so be gentle. Thanks. :) I still need to do some testing to make sure our w32 incantations are doing what we expect, but I reviewed your series and it looks good to me. Also, thanks to 4/7 and the fact that the keystore isn't currently used on w32, we should be able to switch existing users to the new default state directory without any problems. Series: Reviewed-by: Michael Roth mdr...@linux.vnet.ibm.com Laszlo Ersek (6): osdep: add qemu_get_local_state_pathname() qga: determine default state dir and pidfile dynamically configure: don't save any fixed local_statedir for win32 qga: create state directory on win32 qga: remove undefined behavior in ga_install_service() qga: save state directory in ga_install_service() configure| 12 +++--- include/qemu/osdep.h | 11 + qga/service-win32.h |3 +- qga/main.c | 57 +++-- qga/service-win32.c | 25 ++ util/oslib-posix.c |9 util/oslib-win32.c | 22 +++ 7 files changed, 118 insertions(+), 21 deletions(-)
Re: [Qemu-devel] Patch Round-up for stable 1.4.2, freeze on Monday
On Fri, May 17, 2013 at 10:46:15AM -0500, Doug Goldstein wrote: On Tue, May 14, 2013 at 4:52 PM, Michael Roth mdr...@linux.vnet.ibm.comwrote: Hi everyone, The following new patches are queued for QEMU stable v1.4.2: https://github.com/mdroth/qemu/commits/stable-1.4-staging The release is planned for 05-24-2013: http://wiki.qemu.org/Planning/1.4 Please CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 05-20-2013 for new patches. Testing/feedback is greatly appreciated. Thanks! Michael, I have one patch in my 1.4 stable queue. From: Paolo Bonzini pbonz...@redhat.com Date: Wed, 13 Mar 2013 14:58:13 + (+0100) Subject: qemu-iotests: add tests for rebasing zero clusters X-Git-Url: http://git.qemu.org/?p=qemu.git;a=commitdiff_plain;h=acbf30ec601b1f817febc4500025b7c4181312c4 qemu-iotests: add tests for rebasing zero clusters If zero clusters are erroneously treated as unallocated, qemu-img rebase will copy the backing file's contents onto the cluster. The bug existed also in image streaming, but since the root cause was in qcow2's is_allocated implementation it is enough to test it with qemu-img. Signed-off-by: Paolo Bonzini pbonz...@redhat.com Signed-off-by: Stefan Hajnoczi stefa...@redhat.com --- It only adds a test for something that was fixed in 1.4.1 (maybe was fixed by the final 1.4.0 release I can't recall). Thanks, pushed this to staging along with what should be all outstanding patches noted so far. -- Doug Goldstein
Re: [Qemu-devel] [PATCH v3 00/11] qapi: add support for lists of native types
On Thu, May 16, 2013 at 12:38:30PM +0200, Laszlo Ersek wrote: On 05/15/13 21:13, mdroth wrote: On Wed, May 15, 2013 at 02:05:58PM -0400, Luiz Capitulino wrote: On Wed, 15 May 2013 12:42:24 -0500 mdroth mdr...@linux.vnet.ibm.com wrote: The only way I've managed to reproduce this is by having a stale qapi-types.h hanging around in $SRC_DIR while i'm building in a different $BUILD_DIR. Can you confirm that's not what's happening here? Yes, it was :( I had an old qapi-types.h in $SRC_DIR, but was building in a different $BUILD_DIR. I am really sorry for having wasted your time on this. I've applied this series to qmp-next branch, but I have no idea how I ended up with a qapi-types.h file in $SRC_DIR. I have an alias to build qemu that I use for several months now... No problem, only thought to check that scenario because it happens to me all the time :) Side question: what's a common use case for a separate $BUILD_DIR? I just use git clean -fdx in $SRC_DIR instead of make clean. The need to reconfigure (and to regenerate my tags file from scratch) is a small price to pay for the peace of mind. I also do it for peace of mind, but I'm not particularly disciplined about logging all my changes/new files into git as i go, so it's nice to be able to do a clean build even when my $SRC_DIR is littered with abandoned code/files/etc by simply wiping out the build dir. I also have a number of situations where the resulting builds need to be kept around for a while. For example I have build directories for v1.1-v1.5 that I keep around for migration testing, other builds for general usage, and a multitude of temp builds i might create to compare upstream against a test/stable/development build. I could use `make install` with an install prefix for this but for me it's quicker to just use the binaries within the build directory. Wasn't aware of `git clean` though, always just tried to make due with `git reset --hard` + rm but that can get tedious at times. Thanks Laszlo
Re: [Qemu-devel] [Qemu-stable] Patch Round-up for stable 1.4.2, freeze on Monday
On Fri, May 17, 2013 at 01:43:28PM -0700, Josh Durgin wrote: On 05/17/2013 12:08 PM, mdroth wrote: On Fri, May 17, 2013 at 10:46:15AM -0500, Doug Goldstein wrote: On Tue, May 14, 2013 at 4:52 PM, Michael Roth mdr...@linux.vnet.ibm.comwrote: Hi everyone, The following new patches are queued for QEMU stable v1.4.2: https://github.com/mdroth/qemu/commits/stable-1.4-staging The release is planned for 05-24-2013: http://wiki.qemu.org/Planning/1.4 Please CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 05-20-2013 for new patches. Testing/feedback is greatly appreciated. Thanks! Michael, I have one patch in my 1.4 stable queue. From: Paolo Bonzini pbonz...@redhat.com Date: Wed, 13 Mar 2013 14:58:13 + (+0100) Subject: qemu-iotests: add tests for rebasing zero clusters X-Git-Url: http://git.qemu.org/?p=qemu.git;a=commitdiff_plain;h=acbf30ec601b1f817febc4500025b7c4181312c4 qemu-iotests: add tests for rebasing zero clusters If zero clusters are erroneously treated as unallocated, qemu-img rebase will copy the backing file's contents onto the cluster. The bug existed also in image streaming, but since the root cause was in qcow2's is_allocated implementation it is enough to test it with qemu-img. Signed-off-by: Paolo Bonzini pbonz...@redhat.com Signed-off-by: Stefan Hajnoczi stefa...@redhat.com --- It only adds a test for something that was fixed in 1.4.1 (maybe was fixed by the final 1.4.0 release I can't recall). Thanks, pushed this to staging along with what should be all outstanding patches noted so far. Could you add the patch: commit dc7588c1eb3008bda53dde1d6b890cd299758155 Author: Josh Durgin josh.dur...@inktank.com Date: Fri Mar 29 13:03:23 2013 -0700 rbd: add an asynchronous flush The existing bdrv_co_flush_to_disk implementation uses rbd_flush(), which is sychronous and causes the main qemu thread to block until it is complete. This results in unresponsiveness and extra latency for the guest. Fix this by using an asynchronous version of flush. This was added to librbd with a special #define to indicate its presence, since it will be backported to stable versions. Thus, there is no need to check the version of librbd. Implement this as bdrv_aio_flush, since it matches other aio functions in the rbd block driver, and leave out bdrv_co_flush_to_disk when the asynchronous version is available. Reported-by: Oliver Francke oli...@filoo.de Signed-off-by: Josh Durgin josh.dur...@inktank.com Signed-off-by: Stefan Hajnoczi stefa...@redhat.com I sent a cherry-pick of it to qemu-stable a couple days ago, although it applies to the stable-1.4 branch cleanly. It fixes a significant interactivity and performance problem when rbd is used with caching enabled. Hmm, not sure how that slipped by me. Just applied it to staging tree: https://github.com/mdroth/qemu/commits/stable-1.4-staging Thanks, Josh
Re: [Qemu-devel] [ANNOUNCE] QEMU 1.5.0-rc2 is now available
On Wed, May 15, 2013 at 06:53:47PM -0500, Anthony Liguori wrote: Hi, On behalf of the QEMU Team, I'd like to announce the availability of the third release candidate for the QEMU 1.5 release. This release is meant for testing purposes and should not be used in a production environment. http://wiki.qemu.org/download/qemu-1.5.0-rc2.tar.bz2 You can help improve the quality of the QEMU 1.5 release by testing this release and reporting bugs on Launchpad: Sorry to chime in on this so late in the cycle, but I just noticed what seems to be a pretty serious problem with migration to/from 1.4. This is the failure for 1.4 - 1.5-rc2 (qemu) migrate unix:/tmp/migrate.sock Unknown savevm section or instance ':00:03.0/virtio-net' 0 Configuration: source: v14/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -L v14-bios -M pc-i440fx-1.4 -m 512M -kernel boot/vmlinuz-x86_64 -initrd boot/test-initramfs-x86_64.img.gz -vga std -append seed=1234 -drive file=disk1.img,if=virtio -drive file=disk2.img,if=virtio -net nic,model=virtio -net user -monitor unix:/tmp/vm-hmp.sock,server,nowait -qmp unix:/tmp/vm-qmp.sock,server,nowait -vnc :100 target: v15rc2/x86_64-softmmu/qemu-system-x86_64 -enable-kvm -L temp-bios -M pc-i440fx-1.4 -m 512M -kernel boot/vmlinuz-x86_64 -initrd boot/test-initramfs-x86_64.img.gz -vga std -append seed=1234 -drive file=disk1.img,if=virtio -drive file=disk2.img,if=virtio -net nic,model=virtio -net user -incoming unix:/tmp/migrate.sock -monitor unix:/tmp/vm-hmp-incoming.sock,server,nowait -qmp unix:/tmp/vm-qmp-incoming.sock,server,nowait -vnc :101 QEMU 1.4.0 monitor - type 'help' for more information This seems to have been introduced with the virtio refactoring: commit e37da3945fa2fde161e1b217f937fc318c4b7639 Author: KONRAD Frederic fred.kon...@greensocs.com Date: Thu Apr 11 16:29:58 2013 +0200 virtio-net-pci: switch to the new API. Here the virtio-net-pci is modified for the new API. The device virtio-net-pci extends virtio-pci. It creates and connects a virtio-net-device during the init. The properties are not changed. Signed-off-by: KONRAD Frederic fred.kon...@greensocs.com Tested-by: Cornelia Huck cornelia.h...@de.ibm.com Message-id: 1365690602-22729-4-git-send-email-fred.kon...@greensocs.com Signed-off-by: Anthony Liguori aligu...@us.ibm.com And if we roll that back, we have similar failures for virtio-blk, and most likely the other virtio devices touched by the refactoring. The issue seems to be a change the way section id strings are generated in vmstate_register(). In v1.4.x we had: se-instance_id: 0, se-idstr: :00:03.0/virtio-net In v1.5.0-rc2 we have: se-instance_id: 0, se-idstr: virtio-net This seems to be due to the fact that these devices now sit on a TYPE_VIRTIO_BUS that has no implementation of TYPE_BUS's get_dev_path() interface, which is what savevm uses to calculate the id prefix for se-idstr. Prior to the refactoring, the device sat on a TYPE_PCI_BUS which used pcibus_get_dev_path() to calculate this. I'm not sure what the best fix is for this. I looking at implementing get_dev_path() for TYPE_VIRTIO_BUS, but to maintain migration compatibility we'd end up baking in PCI-specific stuff which from what I gather is exactly what we were trying to avoid there. I think adding a compat string property to TYPE_VIRTIO_DEVICE and having that get set somewhere like virtio_bus_plug_device() is a better approach, but vmstate_register() gets call during TYPE_VIRTIO_DEVICE init which I think happens before then. Still looking at it but if someone more familiar with this code has some ideas or wants to whip up a patch please jump right in. Regards, Anthony Liguori
Re: [Qemu-devel] [ANNOUNCE] QEMU 1.5.0-rc2 is now available
On Thu, May 16, 2013 at 06:07:12PM +0200, Paolo Bonzini wrote:
Il 16/05/2013 17:54, KONRAD Frédéric ha scritto:
I think this can do the job, any better idea?
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d5257ed..e033b53 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -43,7 +43,6 @@
#endif
static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
-static char *pcibus_get_dev_path(DeviceState *dev);
static char *pcibus_get_fw_dev_path(DeviceState *dev);
static int pcibus_reset(BusState *qbus);
@@ -2129,7 +2128,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev)
return g_strdup(path);
}
-static char *pcibus_get_dev_path(DeviceState *dev)
+char *pcibus_get_dev_path(DeviceState *dev)
{
PCIDevice *d = container_of(dev, PCIDevice, qdev);
PCIDevice *t;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 70d2c6b..0241223 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1514,11 +1514,19 @@ static void virtio_pci_bus_new(VirtioBusState
*bus, VirtIOPCIProxy *dev)
qbus-allow_hotplug = 1;
}
+static char *virtio_pci_bus_get_dev_path(DeviceState *dev)
+{
+BusState *bus = qdev_get_parent_bus(dev);
+DeviceState *proxy = DEVICE(bus-parent);
+return g_strdup(pcibus_get_dev_path(proxy));
You do not need to export pcibus_get_dev_path. This should just return
qdev_get_dev_path(proxy) and should be in TYPE_VIRTIO_BUS, not in the
PCI-specific subclass.
(The g_strdup is not needed, either).
I've been testing the patch below and it seems to do the trick. If it seems
reasonable I'll submit it after a bit more testing.
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index aab72ff..91b7bad 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -154,12 +154,35 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus,
uint8_t *config)
}
}
+static char *virtio_bus_get_dev_path(DeviceState *dev)
+{
+DeviceState *parent_dev = DEVICE(OBJECT(dev)-parent);
+BusClass *parent_bus_klass =
BUS_GET_CLASS(qdev_get_parent_bus(parent_dev));
+
+/* pass this call up to the device's parent's bus to get at
+ * guest-visible hardware topology
+ */
+if (parent_bus_klass-get_dev_path) {
+return parent_bus_klass-get_dev_path(parent_dev);
+}
+
+return NULL;
+}
+
+static void virtio_bus_class_init(ObjectClass *klass, void *data)
+{
+BusClass *k = BUS_CLASS(klass);
+
+k-get_dev_path = virtio_bus_get_dev_path;
+}
+
static const TypeInfo virtio_bus_info = {
.name = TYPE_VIRTIO_BUS,
.parent = TYPE_BUS,
.instance_size = sizeof(VirtioBusState),
.abstract = true,
.class_size = sizeof(VirtioBusClass),
+.class_init = virtio_bus_class_init,
};
Paolo
+}
+
static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
{
BusClass *bus_class = BUS_CLASS(klass);
VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
bus_class-max_dev = 1;
+bus_class-get_dev_path = virtio_pci_bus_get_dev_path;
k-notify = virtio_pci_notify;
k-save_config = virtio_pci_save_config;
k-load_config = virtio_pci_load_config;
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 8d075ab..fb5723c 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -708,6 +708,8 @@ static inline void pci_dma_sglist_init(QEMUSGList
*qsg, PCIDevice *dev,
extern const VMStateDescription vmstate_pci_device;
+char *pcibus_get_dev_path(DeviceState *dev);
+
#define VMSTATE_PCI_DEVICE(_field, _state) { \
.name = (stringify(_field)), \
.size = sizeof(PCIDevice), \
Re: [Qemu-devel] [ANNOUNCE] QEMU 1.5.0-rc2 is now available
On Thu, May 16, 2013 at 06:34:09PM +0200, KONRAD Frédéric wrote:
On 16/05/2013 18:07, Paolo Bonzini wrote:
Il 16/05/2013 17:54, KONRAD Frédéric ha scritto:
I think this can do the job, any better idea?
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d5257ed..e033b53 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -43,7 +43,6 @@
#endif
static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
-static char *pcibus_get_dev_path(DeviceState *dev);
static char *pcibus_get_fw_dev_path(DeviceState *dev);
static int pcibus_reset(BusState *qbus);
@@ -2129,7 +2128,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev)
return g_strdup(path);
}
-static char *pcibus_get_dev_path(DeviceState *dev)
+char *pcibus_get_dev_path(DeviceState *dev)
{
PCIDevice *d = container_of(dev, PCIDevice, qdev);
PCIDevice *t;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 70d2c6b..0241223 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1514,11 +1514,19 @@ static void virtio_pci_bus_new(VirtioBusState
*bus, VirtIOPCIProxy *dev)
qbus-allow_hotplug = 1;
}
+static char *virtio_pci_bus_get_dev_path(DeviceState *dev)
+{
+BusState *bus = qdev_get_parent_bus(dev);
+DeviceState *proxy = DEVICE(bus-parent);
+return g_strdup(pcibus_get_dev_path(proxy));
You do not need to export pcibus_get_dev_path. This should just return
qdev_get_dev_path(proxy) and should be in TYPE_VIRTIO_BUS, not in the
PCI-specific subclass.
(The g_strdup is not needed, either).
Paolo
True, I avoided it because of CCW and S390, but as they don't have
there get_dev_path, it seems
not to change anything for them.
I think that's better and I get :00:04.0/virtio-net for idstr.
Sorry, my email seems to be malfunctioning this morning and I didn't see
this before sending mine. Not sure which one is better but I'll be happy
to test whatever we decide on.
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index aab72ff..ea2e11a 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -154,12 +154,26 @@ void virtio_bus_set_vdev_config(VirtioBusState
*bus, uint8_t *config)
}
}
+static char *virtio_bus_get_dev_path(DeviceState *dev)
+{
+BusState *bus = qdev_get_parent_bus(dev);
+DeviceState *proxy = DEVICE(bus-parent);
+return qdev_get_dev_path(proxy);
+}
+
+static void virtio_bus_class_init(ObjectClass *klass, void *data)
+{
+BusClass *bus_class = BUS_CLASS(klass);
+bus_class-get_dev_path = virtio_bus_get_dev_path;
+}
+
static const TypeInfo virtio_bus_info = {
.name = TYPE_VIRTIO_BUS,
.parent = TYPE_BUS,
.instance_size = sizeof(VirtioBusState),
.abstract = true,
.class_size = sizeof(VirtioBusClass),
+.class_init = virtio_bus_class_init
};
static void virtio_register_types(void)
--
1.7.11.7
Re: [Qemu-devel] Patch Round-up for stable 1.4.2, freeze on Monday
On Wed, May 15, 2013 at 05:48:14PM -0400, Cole Robinson wrote: On 05/14/2013 05:52 PM, Michael Roth wrote: Hi everyone, The following new patches are queued for QEMU stable v1.4.2: https://github.com/mdroth/qemu/commits/stable-1.4-staging The release is planned for 05-24-2013: http://wiki.qemu.org/Planning/1.4 Please CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 05-20-2013 for new patches. I just added this patch to Fedora to fix a crash that gnome-boxes is hitting: commit 3713e1485e6eace7d48b9c790602cfd92c616e5f Author: Hans de Goede hdego...@redhat.com Date: Fri Mar 15 11:52:37 2013 +0100 usb-redir: Fix crash on migration with no client connected Also, building documentation is still broken with texinfo 5, I got the details wrong about this for the 1.4.1 series. Master got the fix through commit 5d6768e3b8908a60f0a3016b7fa24194f6b47c80 Author: MORITA Kazutaka morita.kazut...@lab.ntt.co.jp Date: Fri Feb 22 12:39:51 2013 +0900 sheepdog: accept URIs But that's not appropriate for stable. I'll send a patch. Thanks, just pushed both of these: https://github.com/mdroth/qemu/commits/stable-1.4-staging - Cole
Re: [Qemu-devel] [PATCH for-1.5] virtio: add virtio_bus_get_dev_path.'
On Thu, May 16, 2013 at 07:06:07PM +0200, fred.kon...@greensocs.com wrote:
From: KONRAD Frederic fred.kon...@greensocs.com
This adds virtio_bus_get_dev_path to fix migration id string which is wrong
since the virtio refactoring.
Signed-off-by: KONRAD Frederic fred.kon...@greensocs.com
Reviewed-by: Paolo Bonzini pbonz...@redhat.com
Cc: mdroth mdr...@linux.vnet.ibm.com
Re-tested the failure scenario with this patch applied and this does
seem to resolve the issue. For reference, the configuration was v1.4.0
- 1.5.0-rc2+fix using the following options:
x86_64-softmmu/qemu-system-x86_64 -enable-kvm -L temp-bios -M
pc-i440fx-1.4 -m 512M -kernel boot/vmlinuz-x86_64 -initrd
boot/test-initramfs-x86_64.img.gz -vga std -append seed=1234 -drive
file=disk1.img,if=virtio -drive file=disk2.img,if=virtio -device
virtio-net-pci,netdev=net0 -netdev user,id=net0
Tested-by: Michael Roth mdr...@linux.vnet.ibm.com
---
hw/virtio/virtio-bus.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index aab72ff..ea2e11a 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -154,12 +154,26 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus,
uint8_t *config)
}
}
+static char *virtio_bus_get_dev_path(DeviceState *dev)
+{
+BusState *bus = qdev_get_parent_bus(dev);
+DeviceState *proxy = DEVICE(bus-parent);
+return qdev_get_dev_path(proxy);
+}
+
+static void virtio_bus_class_init(ObjectClass *klass, void *data)
+{
+BusClass *bus_class = BUS_CLASS(klass);
+bus_class-get_dev_path = virtio_bus_get_dev_path;
+}
+
static const TypeInfo virtio_bus_info = {
.name = TYPE_VIRTIO_BUS,
.parent = TYPE_BUS,
.instance_size = sizeof(VirtioBusState),
.abstract = true,
.class_size = sizeof(VirtioBusClass),
+.class_init = virtio_bus_class_init
};
static void virtio_register_types(void)
--
1.7.11.7
Re: [Qemu-devel] [PATCH v3 00/11] qapi: add support for lists of native types
On Wed, May 15, 2013 at 09:17:46AM -0400, Luiz Capitulino wrote:
On Fri, 10 May 2013 17:45:59 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
These patches apply on top of qemu.git master, and can also be obtained
from:
git://github.com/mdroth/qemu.git qapi-native-lists
Sending this now since a number of series have popped up in the past that
wanted this, and Amos has some pending patches (query-mac-tables) that rely
on this as well.
These patches add support for specifying lists of native qapi types
(int/bool/str/number/int8/uint8/etc) like so:
{ 'type': 'foo',
'data': { 'bar': ['int'] }}
for a 'bar' field that is a list of type 'int',
{ 'type': 'foo2',
'data': { 'bar2': ['str'] }}
for a 'bar2' field that is a list of type 'str', and so on.
This uses linked list types for the native C representations, just as we do
for complex schema-defined types. In the future we may add schema
annotations
of some sort to specify a more natural/efficient array type for the C
representations, but this should serve the majority of uses-cases for now.
I'm getting a build breakage when building all targets:
Hmm, tested all target builds and didn't/don't see any issues. Is this a
clean build? Can you confirm whether or not int8List/etc are declared in
qapi-types.h in your current build dir?
If they're not in there the file is stale and not being regenerated for some
reason. Which would be weird since qapi-visit.h seems to be getting
regenerated.
If they *are* there might be some kind of race in the QAPI build system,
because I think we rely on qapi-types.h/qapi-visit.h coming from
GENERATED_HEADERS, but otherwise being treated as parallelizable. Really,
qapi-types.h should be a dependancy of all qapi-visit.h users, and I don't
see that reflected in the Makefile unless I'm missing something.
In file included from
/home/lcapitulino/work/src/upstream/qmp-unstable/target-i386/cpu.c:34:0:
../qapi-visit.h:30:38: error: unknown type name ‘int8List’
../qapi-visit.h:31:39: error: unknown type name ‘int16List’
../qapi-visit.h:32:39: error: unknown type name ‘int32List’
../qapi-visit.h:33:39: error: unknown type name ‘int64List’
../qapi-visit.h:34:39: error: unknown type name ‘uint8List’
../qapi-visit.h:35:40: error: unknown type name ‘uint16List’
../qapi-visit.h:36:40: error: unknown type name ‘uint32List’
../qapi-visit.h:37:40: error: unknown type name ‘uint64List’
make[1]: *** [target-i386/cpu.o] Error 1
make: *** [subdir-i386-softmmu] Error 2
make: *** Waiting for unfinished jobs
In file included from
/home/lcapitulino/work/src/upstream/qmp-unstable/target-i386/cpu.c:34:0:
../qapi-visit.h:30:38: error: unknown type name ‘int8List’
../qapi-visit.h:31:39: error: unknown type name ‘int16List’
../qapi-visit.h:32:39: error: unknown type name ‘int32List’
../qapi-visit.h:33:39: error: unknown type name ‘int64List’
../qapi-visit.h:34:39: error: unknown type name ‘uint8List’
../qapi-visit.h:35:40: error: unknown type name ‘uint16List’
../qapi-visit.h:36:40: error: unknown type name ‘uint32List’
../qapi-visit.h:37:40: error: unknown type name ‘uint64List’
make[1]: *** [target-i386/cpu.o] Error 1
make: *** [subdir-x86_64-softmmu] Error 2
Re: [Qemu-devel] [PATCH for-1.5?] ide-test: Fix endianness problems
On Wed, May 15, 2013 at 10:37:43AM -0500, Anthony Liguori wrote: Paolo Bonzini pbonz...@redhat.com writes: Il 15/05/2013 15:24, Kevin Wolf ha scritto: Am 15.05.2013 um 15:15 hat Paolo Bonzini geschrieben: Il 15/05/2013 15:00, Kevin Wolf ha scritto: The test case passes on big endian hosts now (tested on ppc64) Signed-off-by: Kevin Wolf kw...@redhat.com For 1.5? Do we need an extra -rc? An extra -rc for a test case fix on big endian hosts is probably too much... But if it can still be applied for the release, sure, why not. Well, we have Mac OS X not building, Peter's patch fixes it and will be in -rc2 BSDs not supporting the GTK+ We don't have a formal support statement, but I don't think it's controversial to say that BSD hosts are a secondary platform from a host point of view. A build issue in an optional feature on a secondary platform is not something I'd consider a release blocker. release, and SLIRP broken on Windows. This is a tough one since it's the default networking backend. That said, it likely has been broken for a while (months) and no one noticed. That makes me think that fixing in stable (particularly if we scheduled a stable release for two weeks after 1.5.0) is reasonable. This is a bit tight. Freeze for 1.4.2 is the same day as 1.5.0, so we'd be testing 2 stable releases at the same time. So if we do this, I think it we should restrict it to fixing the specific issues we're considering as potential 1.5.0 release blockers so we can focus on those in testing rather than opening it up for general fixes. Regards, Anthony Liguori At least the first two have patches on the list, the last is bisected but no patch yet. Paolo
Re: [Qemu-devel] Patch Round-up for stable 1.4.2, freeze on Monday
On Wed, May 15, 2013 at 10:09:32AM -0400, Brad Smith wrote: On Tue, May 14, 2013 at 04:52:57PM -0500, Michael Roth wrote: Hi everyone, The following new patches are queued for QEMU stable v1.4.2: https://github.com/mdroth/qemu/commits/stable-1.4-staging The release is planned for 05-24-2013: http://wiki.qemu.org/Planning/1.4 Please CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 05-20-2013 for new patches. Testing/feedback is greatly appreciated. This patch is missing from the 1.4 branch.. http://lists.nongnu.org/archive/html/qemu-stable/2013-04/msg00069.html Sorry, didn't think to re-check for patches that came in during 1.4.1 freeze. I'm also missing: qemu-timer: move timeBeginPeriod/timeEndPeriod to os-win32 but there are some conflicts I need to look at. I have you patch queued locally, but can you respond to that thread with your SoB before I push it? -- This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean.
Re: [Qemu-devel] [PATCH v3 00/11] qapi: add support for lists of native types
On Wed, May 15, 2013 at 11:04:27AM -0400, Luiz Capitulino wrote:
On Wed, 15 May 2013 09:32:37 -0500
mdroth mdr...@linux.vnet.ibm.com wrote:
On Wed, May 15, 2013 at 09:17:46AM -0400, Luiz Capitulino wrote:
On Fri, 10 May 2013 17:45:59 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
These patches apply on top of qemu.git master, and can also be obtained
from:
git://github.com/mdroth/qemu.git qapi-native-lists
Sending this now since a number of series have popped up in the past
that
wanted this, and Amos has some pending patches (query-mac-tables) that
rely
on this as well.
These patches add support for specifying lists of native qapi types
(int/bool/str/number/int8/uint8/etc) like so:
{ 'type': 'foo',
'data': { 'bar': ['int'] }}
for a 'bar' field that is a list of type 'int',
{ 'type': 'foo2',
'data': { 'bar2': ['str'] }}
for a 'bar2' field that is a list of type 'str', and so on.
This uses linked list types for the native C representations, just as
we do
for complex schema-defined types. In the future we may add schema
annotations
of some sort to specify a more natural/efficient array type for the C
representations, but this should serve the majority of uses-cases for
now.
I'm getting a build breakage when building all targets:
Hmm, tested all target builds and didn't/don't see any issues. Is this a
clean build? Can you confirm whether or not int8List/etc are declared in
qapi-types.h in your current build dir?
Yes, it's a clean build and yes, the int?List declarations are being
generated.
Tested a little bit more and this also happens with make -j1 and when
building only one target (x86_64 in my case).
The only way I've managed to reproduce this is by having a stale
qapi-types.h hanging around in $SRC_DIR while i'm building in a
different $BUILD_DIR. Can you confirm that's not what's happening here?
This is a real headscratcher otherwise. `make clean` wipes out
qapi-types.h, yet the file is present at the time of the build failure,
and appears in tact. So all I can imagine occuring here is the file
having only been partially generated at the time it was read, but I'm
not sure how that could happen with -j1
I don't remember this happening with v1 btw, but I also don't remember if I
fully built it.
Re: [Qemu-devel] [PATCH v3 00/11] qapi: add support for lists of native types
On Wed, May 15, 2013 at 02:05:58PM -0400, Luiz Capitulino wrote:
On Wed, 15 May 2013 12:42:24 -0500
mdroth mdr...@linux.vnet.ibm.com wrote:
On Wed, May 15, 2013 at 11:04:27AM -0400, Luiz Capitulino wrote:
On Wed, 15 May 2013 09:32:37 -0500
mdroth mdr...@linux.vnet.ibm.com wrote:
On Wed, May 15, 2013 at 09:17:46AM -0400, Luiz Capitulino wrote:
On Fri, 10 May 2013 17:45:59 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
These patches apply on top of qemu.git master, and can also be
obtained from:
git://github.com/mdroth/qemu.git qapi-native-lists
Sending this now since a number of series have popped up in the
past that
wanted this, and Amos has some pending patches (query-mac-tables)
that rely
on this as well.
These patches add support for specifying lists of native qapi types
(int/bool/str/number/int8/uint8/etc) like so:
{ 'type': 'foo',
'data': { 'bar': ['int'] }}
for a 'bar' field that is a list of type 'int',
{ 'type': 'foo2',
'data': { 'bar2': ['str'] }}
for a 'bar2' field that is a list of type 'str', and so on.
This uses linked list types for the native C representations, just
as we do
for complex schema-defined types. In the future we may add schema
annotations
of some sort to specify a more natural/efficient array type for the
C
representations, but this should serve the majority of uses-cases
for now.
I'm getting a build breakage when building all targets:
Hmm, tested all target builds and didn't/don't see any issues. Is this a
clean build? Can you confirm whether or not int8List/etc are declared in
qapi-types.h in your current build dir?
Yes, it's a clean build and yes, the int?List declarations are being
generated.
Tested a little bit more and this also happens with make -j1 and when
building only one target (x86_64 in my case).
The only way I've managed to reproduce this is by having a stale
qapi-types.h hanging around in $SRC_DIR while i'm building in a
different $BUILD_DIR. Can you confirm that's not what's happening here?
Yes, it was :( I had an old qapi-types.h in $SRC_DIR, but was building
in a different $BUILD_DIR. I am really sorry for having wasted your time
on this.
I've applied this series to qmp-next branch, but I have no idea how I ended
up with a qapi-types.h file in $SRC_DIR. I have an alias to build qemu that
I use for several months now...
No problem, only thought to check that scenario because it happens to me
all the time :)
Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda
On Sun, May 12, 2013 at 06:47:05PM +0200, Andreas Färber wrote: Am 10.05.2013 22:09, schrieb mdroth: On Fri, May 10, 2013 at 09:53:27PM +0200, Laszlo Ersek wrote: On 05/10/13 21:30, mdroth wrote: On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote: I should have paid more attention to portability and error path cleanup in the CVE-2013-2007 fix. (We continue to assume, like the rest of qemu code, that qemu_set_cloexec() never fails internally. This should be a reasonable assumption when the input fd is valid.) Laszlo Ersek (2): qga: distinguish binary modes in guest_file_open_modes map qga: unlink just created guest-file if fchmod() or fdopen() fails on it Thanks, applied to qga branch: https://github.com/mdroth/qemu/commits/qga Thanks! Can you reword the second commit to include Eric's R-b? http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg01179.html Sure, missed that one. Should be fixed in tree now. Shouldn't at least the unlinking be backported to stable as well? Yes, these are basically updates to the CVE fix, so I think they should all be applied to stable. I'll send PULL today so hopefully we can get them into 1.5 prior to patch freeze for 1.4.2. Otherwise I'll backport from the qga tree. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support
On Fri, May 10, 2013 at 11:04:22AM +0800, Amos Kong wrote: On Thu, May 09, 2013 at 09:20:53PM -0500, Michael Roth wrote: Teach type generators about native types so they can generate the appropriate linked list types. Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com --- scripts/qapi-types.py | 43 --- scripts/qapi.py | 21 + 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 9e19920..96cb26d 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -16,7 +16,18 @@ import os import getopt import errno snip diff --git a/scripts/qapi.py b/scripts/qapi.py index afc5f32..0ac8c2b 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -11,6 +11,10 @@ from ordereddict import OrderedDict +builtin_types = [ +'str', 'int', 'number', 'bool' Will we add size, int8, uint32, ... in future when they are needed? Well, that was the plan, but I hadn't recalled that support for these was already been added to the code generators as part of the Netdev opts-visitor stuff, so I'll go ahead and do a re-spin with those added. +] + -- Amos.
Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
On Fri, May 10, 2013 at 02:47:18PM +0200, Laszlo Ersek wrote:
On 05/10/13 14:22, Eric Blake wrote:
On 05/09/2013 08:20 PM, Michael Roth wrote:
Currently our JSON parser assumes that numbers lacking a mantissa are
integers and attempts to store them as QInt/int64 values. This breaks in
the case where the number overflows/underflows int64 values (which is
still valid JSON)
Fix this by detecting such cases and using a QFloat to store the value
instead.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
qobject/json-parser.c | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
This changes the error message handed back to QMP clients, and possibly
exposes problems in other qemu code that receives the result of json
parses. Previously, for an 'int' argument, if you passed in a too-large
number, you got an error that the argument was too large for int. Now,
the number is accepted as a double; are we guaranteed that in a context
that expects a qint, when that code is now handed a qfloat (a case which
was previously impossible because qint_from_int protected it), that the
code will still behave correctly?
I tried to consider this while reviewing... Maybe I was wrong.
The pre-patch code for JSON_INTEGER:
obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
doesn't check for errors at all. (I assume that JSON_INTEGER is selected
by the parser, token_get_type(), based on syntax purely.)
I thought when the pre-patch version encounters an int-looking decimal
string that's actually too big in magnitude for an int, you'd simply end
up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
value, errno is lost, and qint_from_int() sees nothing wrong.
With the patch, you end up with a float instead of an int-typed
LLONG_MIN/LLONG_MAX, and also no error.
This is correct, but it does make code acting on the resulting QObject
capable of checking for this scenario and acting accordingly in whatever
manner is appropriate. I think that's appropriate, since there's no error
as far as the JSON spec goes, but further up the stack we may have stricter
requirements on what the valid ranges are for various fields.
For instance, in the case of QmpInputVisitor, these cases will
trigger an error case that was previously being bypassed when
out-of-range values were provided in place of 'int' due to the parser
silently capping the max/min values:
in qmp_input_type_int():
QObject *qobj = qmp_input_get_object(qiv, name);
if (!qobj || qobject_type(qobj) != QTYPE_QINT) {
error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : null,
integer);
return;
}
We'd now trigger this error for such inputs (since we'd have QTYPE_QFLOAT),
which is stricter checking than before, but as intended since we map int to
int64_t and expect values in that range (though we could stand to be clearer
about this in the QAPI documentation)
At any rate, libvirt already checks that all numbers that fall outside
the range of int64_t are never passed over qmp when passing an int
argument (and yes, this is annoying, in that large 64-bit unsigned
numbers have to be passed as negative numbers, rather than exceeding
INT64_MAX), so libvirt should not be triggering this newly exposed code
path. But even if libvirt doesn't plan on triggering it, I'd still feel
better if your commit message documented evidence of testing what
happens in this case. For example, compare what
{execute:add-fd,arguments:{fdset-id:}}
does before and after this patch.
That would be likely interesting to test, yes.
That would trigger the error mentioned above, as opposed to silently changing
the value to LLONG_MAX. I'll see about covering this case somewhere in
tests/test-qmp-input-visitor.
Laszlo
Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
On Fri, May 10, 2013 at 08:08:05AM -0600, Eric Blake wrote:
On 05/10/2013 06:47 AM, Laszlo Ersek wrote:
The pre-patch code for JSON_INTEGER:
obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));
doesn't check for errors at all. (I assume that JSON_INTEGER is selected
by the parser, token_get_type(), based on syntax purely.)
I thought when the pre-patch version encounters an int-looking decimal
string that's actually too big in magnitude for an int, you'd simply end
up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
value, errno is lost, and qint_from_int() sees nothing wrong.
Oh, right. _That's_ why libvirt had to add checks that it wasn't
passing 0x8000ULL as a positive number - because the qemu
parser was silently clamping it to 0x7fffLL, which is not
what libvirt wanted. So the code was NOT erroring out with an overflow
message, but was acting on the wrong integer.
With the patch, you end up with a float instead of an int-typed
LLONG_MIN/LLONG_MAX, and also no error.
Ah, but here we have a difference - beforehand, the code was passing a
valid (albeit wrong value) qint, so the rest of the qemu code was
oblivious to the fact that the QMP message contained an overflow. But
now the code is passing a qdouble, and the rest of the qemu code may be
unprepared to handle it when expecting a qint.
Yup, new error cases can be triggered, but in the case of
QmpInputVisitor this is handled appropriately (will add a test case to
confirm), and none of our other input visitors act on QObjects, and this
ambiguity isn't present for output visitors.
We also have monitor events that call qobject_from_json() to marshall
event payloads, but these are essentially open-coded QmpInputVisitors
where the JSON values come from native C types. The only case where I
can see this triggering the change is if they did something like:
obj = qobject_from_jsonf({'myInt': %f}, whole_valued_float);
which would be evil, and thankfully such cases don't appear to exist:
mdroth@loki:~/w/qemu.git$ ack-grep qobject_from_json | grep %f
tests/check-qjson.c:987:obj = qobject_from_jsonf(%f, valuef);
mdroth@loki:~/w/qemu.git$
(the 'valuef' above is not whole-valued, and the output is expected to
be a QFloat)
I'm not aware of any other cases to consider, but I might've missed
something.
At any rate, libvirt already checks that all numbers that fall outside
the range of int64_t are never passed over qmp when passing an int
argument (and yes, this is annoying, in that large 64-bit unsigned
numbers have to be passed as negative numbers, rather than exceeding
INT64_MAX), so libvirt should not be triggering this newly exposed code
path. But even if libvirt doesn't plan on triggering it, I'd still feel
better if your commit message documented evidence of testing what
happens in this case. For example, compare what
{execute:add-fd,arguments:{fdset-id:}}
does before and after this patch.
That would be likely interesting to test, yes.
add-fd may not be the best candidate (it expects an fd to be passed at
the same time, and does its own checking that it does not get a negative
number); but I'm sure there's plenty of other candidates (add-cpu is
another possibility that comes quickly to mind) - basically, pick a
command that takes an explicit 'int' argument, and overflow that
argument to see what happens when the command now has to deal with a
qdouble.
Command params will end up getting marshalled in QObject prior to being
passed into commands:
mi = qmp_input_visitor_new_strict(QOBJECT(args));
v = qmp_input_get_visitor(mi);
visit_start_optional(v, has_fdset_id, fdset-id, errp);
if (has_fdset_id) {
visit_type_int(v, fdset_id, fdset-id, errp);
}
visit_end_optional(v, errp);
visit_start_optional(v, has_opaque, opaque, errp);
if (has_opaque) {
visit_type_str(v, opaque, opaque, errp);
}
visit_end_optional(v, errp);
qmp_input_visitor_cleanup(mi);
if (error_is_set(errp)) {
goto out;
}
retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, errp);
so i think a check in tests-qmp-input-visitor that verifies that values that
exceed LLONG_MAX/LLONG_MIN will get added into the QObject as QFloats
and trigger a type error when being passed to visit_type_int() should
cover the cases in question.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support
On Fri, May 10, 2013 at 10:07:45AM -0400, Luiz Capitulino wrote:
On Thu, 9 May 2013 21:20:53 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
Teach type generators about native types so they can generate the
appropriate linked list types.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
scripts/qapi-types.py | 43 ---
scripts/qapi.py | 21 +
2 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9e19920..96cb26d 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -16,7 +16,18 @@ import os
import getopt
import errno
-def generate_fwd_struct(name, members):
+def generate_fwd_struct(name, members, builtin_type=False):
+if builtin_type:
+return mcgen('''
+typedef struct %(name)sList
+{
+%(type)s value;
+struct %(name)sList *next;
+} %(name)sList;
+''',
Sorry for the utterly minor comment, but as you're going to respin please
add a newline before ''' so that we get the declarations properly separated
when generated.
+ type=c_type(name),
+ name=name)
+
return mcgen('''
typedef struct %(name)s %(name)s;
@@ -164,6 +175,7 @@ void qapi_free_%(type)s(%(c_type)s obj);
def generate_type_cleanup(name):
ret = mcgen('''
+
void qapi_free_%(type)s(%(c_type)s obj)
{
QapiDeallocVisitor *md;
@@ -184,8 +196,9 @@ void qapi_free_%(type)s(%(c_type)s obj)
try:
-opts, args = getopt.gnu_getopt(sys.argv[1:], chp:o:,
- [source, header, prefix=,
output-dir=])
+opts, args = getopt.gnu_getopt(sys.argv[1:], chbp:o:,
+ [source, header, builtins,
+prefix=, output-dir=])
except getopt.GetoptError, err:
print str(err)
sys.exit(1)
@@ -197,6 +210,7 @@ h_file = 'qapi-types.h'
do_c = False
do_h = False
+do_builtins = False
for o, a in opts:
if o in (-p, --prefix):
@@ -207,6 +221,8 @@ for o, a in opts:
do_c = True
elif o in (-h, --header):
do_h = True
+elif o in (-b, --builtins):
+do_builtins = True
if not do_c and not do_h:
do_c = True
@@ -282,6 +298,11 @@ fdecl.write(mcgen('''
exprs = parse_schema(sys.stdin)
exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
+fdecl.write(guardstart(QAPI_TYPES_BUILTIN_STRUCT_DECL))
+for typename in builtin_types:
+fdecl.write(generate_fwd_struct(typename, None, builtin_type=True))
+fdecl.write(guardend(QAPI_TYPES_BUILTIN_STRUCT_DECL))
+
for expr in exprs:
ret = \n
if expr.has_key('type'):
@@ -298,6 +319,22 @@ for expr in exprs:
continue
fdecl.write(ret)
+# to avoid header dependency hell, we always generate declarations
+# for built-in types in our header files and simply guard them
+fdecl.write(guardstart(QAPI_TYPES_BUILTIN_CLEANUP_DECL))
+for typename in builtin_types:
+fdecl.write(generate_type_cleanup_decl(typename + List))
+fdecl.write(guardend(QAPI_TYPES_BUILTIN_CLEANUP_DECL))
I'm not sure I got why you're doing this. Is it because you're going to
generate them in more .h files? This is a bit ugly :(
The issue is that things like the types generated from
qapi-schema-test.json or qga/qapi-schema.json may end up referencing
intList/strList/visit_type_intList/etc, which we'll also have declared for
use in the main qapi-schema.json. qapi-schema.json of course can't
depend on tests or qga, so we generate the definitions for the builtin types
when running the code generators on qapi-schema.json.
For everyone else, tests/qga/etc, to be able to use/link against those
definitions we need to include the declarations by either #include'ing
qapi-types.h/qapi-visit.h etc, or by always declaring them and simply
adding a guard.
In this case I've taken the latter approach since hard-coding a
reference in the code generators to header files created by separate calls
to the code generators seemed less modular. qapi-schema-test.json should
be capable of generating self-contained code if need be (and the only
reason we don't make it self-contained is that test-cases rely on
libqemuutil to build, which actually does have a dependency on
qapi-schema.json due to qemu-sockets.
+
+# ...this doesn't work for cases where we link in multiple objects that
+# have the functions defined, so we use -b option to provide control
+# over these cases
+if do_builtins:
+fdef.write(guardstart(QAPI_TYPES_BUILTIN_CLEANUP_DEF))
+for typename in builtin_types:
+fdef.write(generate_type_cleanup(typename + List))
+fdef.write(guardend(QAPI_TYPES_BUILTIN_CLEANUP_DEF))
+
for expr in exprs:
ret = \n
if expr.has_key('type'):
Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values
On Fri, May 10, 2013 at 11:17:17AM -0400, Luiz Capitulino wrote:
On Thu, 9 May 2013 21:20:58 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
Currently our JSON parser assumes that numbers lacking a mantissa are
integers and attempts to store them as QInt/int64 values. This breaks in
the case where the number overflows/underflows int64 values (which is
still valid JSON)
Anthony wanted to fix this by moving to another wire format :)
But, how this patch related to this series?
In v1 Laszlo pointed out that the QFloat tests I added in
test-visitor-serialization were actually non-functional, and those tests were
based on pre-existing code. So I added a patch to fix the pre-existing
code as a pre-cursor to the new unit tests based on it. But fixing that code
exposed the json-parser bug, so I added that fix as a precursor to the
precursor :)
Same with mem leak fixes, etc, to ensure the tests were functional and
leak-free within this series.
Fix this by detecting such cases and using a QFloat to store the value
instead.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
qobject/json-parser.c | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 05279c1..4d14e71 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
case JSON_STRING:
obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
break;
-case JSON_INTEGER:
-obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL,
10)));
-break;
+case JSON_INTEGER: {
+/* A possibility exists that this is a whole-valued float where the
+ * mantissa was left out due to being 0 (.0). It's not a big deal
to
+ * treat these as ints in the parser, so long as users of the
+ * resulting QObject know to expect a QInt in place of a QFloat in
+ * cases like these.
+ *
+ * However, in some cases these values will overflow/underflow a
+ * QInt/int64 container, thus we should assume these are to be
handled
+ * as QFloats/doubles rather than silently changing their values.
+ *
+ * strtoll() indicates these instances by setting errno to ERANGE
+ */
+int64_t value;
+
+errno = 0; /* strtoll doesn't set errno on success */
+value = strtoll(token_get_value(token), NULL, 10);
+if (errno != ERANGE) {
+obj = QOBJECT(qint_from_int(value));
+break;
+}
+/* fall through to JSON_FLOAT */
+}
case JSON_FLOAT:
/* FIXME dependent on locale */
obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token),
NULL)));
Re: [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code
On Fri, May 10, 2013 at 10:10:03AM -0400, Luiz Capitulino wrote: On Thu, 9 May 2013 21:20:56 -0500 Michael Roth mdr...@linux.vnet.ibm.com wrote: Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs qapi-types.c/qapi-visit.c Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com --- Makefile |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 7dc0204..9695c9d 100644 --- a/Makefile +++ b/Makefile @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y) # Build libraries libqemustub.a: $(stub-obj-y) -libqemuutil.a: $(util-obj-y) +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o Don't we want this in for 1.5? I don't think it's causing any issues currently since it's not causing undefined reference errors upstream. users of libqemuutil that make use of qemu-sockets seem to be pulling qapi-types/qapi-visit in through other dependencies. I only noticed it because I was attempting to generate the native list code via tests/Makefile and running into redefinition conflicts with qapi-types.o/qapi-visit.o, then noticed the qemu-sockets.o issue when I attempted to remove the qapi-types/qapi-visit dependency from tests/test-visitor-serialization Now that we're generating the native list code from top-level Makefile, it actually doesn't seem to be needed by this series anymore, so maybe I'll pull it out for now. I think a better fix would be to have qapi/Makefile.obj add these to $util-obj-y directly anyway. ## @@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) qapi-types.c qapi-types.h :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o . $, GEN $@) + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o . -b $, GEN $@) qapi-visit.c qapi-visit.h :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py) - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o . $, GEN $@) + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o . -b $, GEN $@) qmp-commands.h qmp-marshal.c :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o . $, GEN $@)
Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda
On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote: I should have paid more attention to portability and error path cleanup in the CVE-2013-2007 fix. (We continue to assume, like the rest of qemu code, that qemu_set_cloexec() never fails internally. This should be a reasonable assumption when the input fd is valid.) Laszlo Ersek (2): qga: distinguish binary modes in guest_file_open_modes map qga: unlink just created guest-file if fchmod() or fdopen() fails on it Thanks, applied to qga branch: https://github.com/mdroth/qemu/commits/qga qga/commands-posix.c | 25 +++-- 1 files changed, 19 insertions(+), 6 deletions(-)
Re: [Qemu-devel] [PATCH v2 0/2] qga umask fix addenda
On Fri, May 10, 2013 at 09:53:27PM +0200, Laszlo Ersek wrote: On 05/10/13 21:30, mdroth wrote: On Wed, May 08, 2013 at 05:31:34PM +0200, Laszlo Ersek wrote: I should have paid more attention to portability and error path cleanup in the CVE-2013-2007 fix. (We continue to assume, like the rest of qemu code, that qemu_set_cloexec() never fails internally. This should be a reasonable assumption when the input fd is valid.) Laszlo Ersek (2): qga: distinguish binary modes in guest_file_open_modes map qga: unlink just created guest-file if fchmod() or fdopen() fails on it Thanks, applied to qga branch: https://github.com/mdroth/qemu/commits/qga Thanks! Can you reword the second commit to include Eric's R-b? http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg01179.html Sure, missed that one. Should be fixed in tree now. Thanks! Laszlo
Re: [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code
On Fri, May 10, 2013 at 11:32:48AM -0500, mdroth wrote: On Fri, May 10, 2013 at 10:10:03AM -0400, Luiz Capitulino wrote: On Thu, 9 May 2013 21:20:56 -0500 Michael Roth mdr...@linux.vnet.ibm.com wrote: Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs qapi-types.c/qapi-visit.c Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com --- Makefile |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 7dc0204..9695c9d 100644 --- a/Makefile +++ b/Makefile @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y) # Build libraries libqemustub.a: $(stub-obj-y) -libqemuutil.a: $(util-obj-y) +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o Don't we want this in for 1.5? I don't think it's causing any issues currently since it's not causing undefined reference errors upstream. users of libqemuutil that make use of qemu-sockets seem to be pulling qapi-types/qapi-visit in through other dependencies. I only noticed it because I was attempting to generate the native list code via tests/Makefile and running into redefinition conflicts with qapi-types.o/qapi-visit.o, then noticed the qemu-sockets.o issue when I attempted to remove the qapi-types/qapi-visit dependency from tests/test-visitor-serialization Now that we're generating the native list code from top-level Makefile, it actually doesn't seem to be needed by this series anymore, so maybe I'll pull it out for now. I think a better fix would be to have qapi/Makefile.obj add these to $util-obj-y directly anyway. ^ this wasn't quite right, we do have a new dependency on qapi-types/qapi-visit in the visitor tests for native list types, so I've left this in place. ## @@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) qapi-types.c qapi-types.h :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o . $, GEN $@) + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o . -b $, GEN $@) qapi-visit.c qapi-visit.h :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py) - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o . $, GEN $@) + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o . -b $, GEN $@) qmp-commands.h qmp-marshal.c :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o . $, GEN $@)
Re: [Qemu-devel] [PATCH 6/8] qapi: add native list coverage for visitor serialization tests
On Thu, May 09, 2013 at 02:31:03PM +0200, Laszlo Ersek wrote:
On 05/09/13 01:33, Michael Roth wrote:
+case PTYPE_NUMBER: {
+numberList *ptr;
+char *double1, *double2;
+if (cur_head) {
+ptr = cur_head;
+cur_head = ptr-next;
+} else {
+cur_head = ptr = pl_copy.value.numbers;
+}
+/* we serialize with %f for our reference visitors, so rather
than
+ * fuzzy * floating math to test equality, just compare the
+ * formatted values
+ */
I think this comment block has been copied from elsewhere in this file,
indented more deeply and re-filled. There's an asterisk in the comment
body now.
Yes :) Will fix it on the next pass.
+double1 =
g_malloc0(calc_float_string_storage(pt-value.number));
+double2 = g_malloc0(calc_float_string_storage(ptr-value));
+g_assert_cmpstr(double1, ==, double2);
Are you comparing empty strings? Space is allocated and zeroed, but I
can't see where the values are actually formatted.
Well, that's embarassing...
(Same holds for the original instance of this code, test_primitives().)
but at least I can blame it on the fact that I copied it from here
(we'll just ignore who the original author was :)
Will add a patch on the next pass to fix the original instance
beforehand.
Thanks for the catch!
+g_free(double1);
+g_free(double2);
+break;
+}
Thanks,
Laszlo
Re: [Qemu-devel] [PATCH 0/8] qapi: add support for lists of native types
On Thu, May 09, 2013 at 03:31:08PM +0200, Laszlo Ersek wrote:
On 05/09/13 01:33, Michael Roth wrote:
These patches apply on top of qemu.git master, and can also be obtained
from:
git://github.com/mdroth/qemu.git qapi-native-lists
Sending this now since a number of series have popped up in the past that
wanted this, and Amos has some pending patches (query-mac-tables) that rely
on this as well.
These patches add support for specifying lists of native qapi types
(int/bool/str/number) like so:
{ 'type': 'foo',
'data': { 'bar': ['int'] }}
for a 'bar' field that is a list of type 'int',
{ 'type': 'foo2',
'data': { 'bar2': ['str'] }}
for a 'bar2' field that is a list of type 'str', and so on.
This uses linked list types for the native C representations, just as we do
for complex schema-defined types. In the future we may add schema
annotations
of some sort to specify a more natural/efficient array type for the C
representations, but this should serve the majority of uses-cases for now.
Makefile |6 +-
qapi-schema-test.json |8 ++
scripts/qapi-types.py | 44 ++-
scripts/qapi-visit.py | 36 -
scripts/qapi.py| 21 +++
tests/test-qmp-input-visitor.c | 181 +
tests/test-qmp-output-visitor.c| 172
tests/test-visitor-serialization.c | 256
+---
8 files changed, 692 insertions(+), 32 deletions(-)
Two notes:
- the remark I made for 6/8 (comparing empty strings),
- for 7/8 and 8/8: the format specification %3.4f is not very useful.
3 is the field width, 4 is the precision, and the latter means for
%f the number of digits printed after the radix character. It's not
useful to specify a smaller field width (which covers the entire output
string) than precision here.
Yup, that's a mistake. The field width isn't useful at all for what I
was intending actually (to constrain the whole portion of float so that
the fractional portion wouldn't get truncated by snprintf and make the
test less likely to catch re-encoding issues). I think just using %.4f
should suffice since we have plenty of extra buffer space for the values
we're working with (max being 32 / 3) so I'll do that for the next pass.
Other than these nothing pokes me in the eye.
Laszlo
Re: [Qemu-devel] [PATCH 1/2] qga: distinguish binary modes in guest_file_open_modes map
On Tue, May 07, 2013 at 11:27:03AM -0600, Eric Blake wrote: On 05/07/2013 10:56 AM, Laszlo Ersek wrote: In Windows guests this may make a difference. Suggested-by: Eric Blake ebl...@redhat.com Signed-off-by: Laszlo Ersek ler...@redhat.com --- qga/commands-posix.c | 22 -- 1 files changed, 16 insertions(+), 6 deletions(-) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 04c6951..2eec712 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c Oh, and only NOW do I notice that this is in a file named commands-posix.c that doesn't get compiled into the Windows build of qga (there, we only build commands-win32.c, and THAT file always fails this command, because no one has ported guest-file-open there yet). But I guess there is still the argument that some weirdnix system exists that isn't quite POSIX compliant and does have a distinct binary mode (maybe someone plans on compiling qga for Cygwin instead of native windows, since at least that would be able to open files when the commands-win32.c variant doesn't?), so I still think the patch is worth keeping. FWIW, I have some rough patches for w32 implementations of guest-file-* commands queued up that I'll be cleaning up for 1.6, so it's not so much theoretical as just a tad early :) -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH] qga: set umask 0077 when daemonizing (CVE-2013-2007)
On Tue, May 07, 2013 at 09:55:06AM -0600, Eric Blake wrote:
On 05/07/2013 05:47 AM, Anthony Liguori wrote:
From: Laszlo Ersek ler...@redhat.com
The qemu guest agent creates a bunch of files with insecure permissions
when started in daemon mode. For example:
-rw-rw-rw- 1 root root /var/log/qemu-ga.log
-rw-rw-rw- 1 root root /var/run/qga.state
-rw-rw-rw- 1 root root /var/log/qga-fsfreeze-hook.log
In addition, at least all files created with the guest-file-open QMP
command, and all files created with shell output redirection (or
otherwise) by utilities invoked by the fsfreeze hook script are affected.
For now mask all file mode bits for group and others in
become_daemon().
Temporarily, for compatibility reasons, stick with the 0666 file-mode in
case of files newly created by the guest-file-open QMP call. Do so
without changing the umask temporarily.
This points out that:
1. the documentation for guest-file-open is insufficient to describe our
limitations (for example, although C11 requires support for the wx
flag, this patch forbids that flag)
Got a pointer? I can fix up the docs if need be, but fopen() docs that
qemu-ga points at seem to indicate 0666 will be used for new files.
That's no longer the case?
2. guest-file-open is rather puny; we may need a newer interface that
allows the user finer-grained control over the actual mode bits set on
Yes, shame it wasn't there at the start. a guest-file-open-full or
something with support for specifying creation mode will have to do it.
the file that they are creating (and maybe something similar to openat()
that would let the user open/create a file relative to an existing
handle to a directory, rather than always having to specify an absolute
path).
But I agree that _this_ patch fixes the CVE, and that any further
changes to resolve the above two issues are not essential to include in 1.5.
+/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
+static const struct {
+ccpc *forms;
+int oflag_base;
+} guest_file_open_modes[] = {
+{ (ccpc[]){ r, rb, NULL }, O_RDONLY
},
You are mapping things according to their POSIX definition (POSIX, as an
additional requirement above and beyond C99, states that presence or
absence of 'b' is a no-op because there is no difference between binary
and text mode). But C99 permits a distinct binary mode, and qga is
compiled for Windows where binary mode is indeed different.
I think that you probably should split this map into:
{ (ccpc[]){ r, NULL }, O_RDONLY },
{ (ccpc[]){ rb,NULL }, O_RDONLY | O_BINARY },
and so forth (assuming that O_BINARY is properly #defined to 0 on
POSIX-y systems that don't need it), so that you don't regress the qga
server in a Windows guest where the management client is explicitly
passing or omitting 'b' for the intentional difference between text and
binary files. [1]
+
+if ((oflag O_CREAT) fchmod(fd, DEFAULT_NEW_FILE_MODE) ==
-1) {
+error_setg_errno(local_err, errno, failed to set
permission
+ 0%03o on new file '%s' (mode: '%s'),
+ (unsigned)DEFAULT_NEW_FILE_MODE, path,
mode);
On this particular error path, we've left behind an empty mode
file. Is it worth trying to unlink() the dead file?
+} else {
+FILE *f;
+
+f = fdopen(fd, mode);
[1] I don't know if Windows is okay using fdopen() to create a FILE in
binary mode if you didn't match O_BINARY on the fd underlying the FILE.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
On Mon, May 06, 2013 at 09:54:14AM +0200, Paolo Bonzini wrote: Il 03/05/2013 18:03, Michael Roth ha scritto: These patches apply on top of qemu.git master, and can also be obtained from: git://github.com/mdroth/qemu.git qcontext OVERVIEW This series introduces a set of QOM classes/interfaces for event registration/handling: QContext and QSource, which are based closely on their GMainContext/GSource GLib counterparts. QContexts can be created via the command-line via -object, and can also be intructed (via -object params/properties) to automatically start a thread/event-loop to handle QSources we attach to them. This is an awesome idea. However, it seems a bit overengineered. Why do we need QSource at all? In my opinion, we should first change dataplane to use AioContext as a GSource, and benchmark it thoroughly. If it is fast enough, we can I think it would be great to just stick with GSources. I didn't want to rely too heavily on GLib for the RFC since there seems to be some reservations about relying too heavily on GLib for our OneTrueEventLoop interface (mainly, lack of PI mutexes in the context of real-time device threads, or other performance considerations that might pop up and cause us to rethink our use of glib). However, knowing that we *could* do something like porting to QSources and using a different QContext implementation if the need ever became evident is enough for me, and I'm happy to drop QSources until we actually need them. The GSource-QSource conversions would be mostly mechanical. GSource, and benchmark it thoroughly. If it is fast enough, we can just introduce a glib-based QContext and be done with it. Hopefully that is the case... Sounds good to me. I'll look into that more, and talk to some of our performance folks who were involved with the virtio-blk dataplane testing. Paolo
Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
On Mon, May 06, 2013 at 07:25:24AM -0500, Anthony Liguori wrote: Paolo Bonzini pbonz...@redhat.com writes: Il 03/05/2013 18:03, Michael Roth ha scritto: These patches apply on top of qemu.git master, and can also be obtained from: git://github.com/mdroth/qemu.git qcontext OVERVIEW This series introduces a set of QOM classes/interfaces for event registration/handling: QContext and QSource, which are based closely on their GMainContext/GSource GLib counterparts. QContexts can be created via the command-line via -object, and can also be intructed (via -object params/properties) to automatically start a thread/event-loop to handle QSources we attach to them. This is an awesome idea. Ack. However, it seems a bit overengineered. Ack. Why do we need QSource at all? In my opinion, we should first change dataplane to use AioContext as a GSource, and benchmark it thoroughly. If it is fast enough, we can just introduce a glib-based QContext and be done with it. Hopefully that is the case... Why even bother with QContext then? The QContext/GlibQContext object in general, or the QContext base class? In the case of the former, I think a wrapper around GLib that we can instantiate from the command-line line and query properties like TIDs from is necessary for robust control over event loops and CPU resources. We get this essentially for free with QOM, so I think it makes sense to use it. In the case of the latter I'm not too sure. Without the QSource abstraction there isn't much reason not to use the native GLib interfaces on the underlying GSources/GMainContexts directly. In which case GlibQContext would only need to be a container of sorts with some minor additions like spawning an event thread for itself. If we ever did need to switch it out in favor of a non-GLib implementation, it should be a mostly mechanical conversion of GSource-QSource and adding some wrappers around g_main_context_prepare/check/etc. Also along that line, if we're taking the approach of not adding infrastructure/cruft until we actually have a plan to use it, it probably makes sense to make QContext a concrete class implemented via GLib, and we can move the GLib stuff to a sub-class later if we ever end up with another QContext implementation. Does this seem reasonable? Regards, Anthony Liguori Paolo
Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
On Mon, May 06, 2013 at 11:26:06AM +0800, liu ping fan wrote: On Sat, May 4, 2013 at 12:03 AM, Michael Roth mdr...@linux.vnet.ibm.com wrote: These patches apply on top of qemu.git master, and can also be obtained from: git://github.com/mdroth/qemu.git qcontext OVERVIEW This series introduces a set of QOM classes/interfaces for event registration/handling: QContext and QSource, which are based closely on their GMainContext/GSource GLib counterparts. QContexts can be created via the command-line via -object, and can also be intructed (via -object params/properties) to automatically start a thread/event-loop to handle QSources we attach to them. The reference implementation of QContext is GlibQContext, which uses GMainContext/GSource interfaces underneath the covers, thus we can also attach GSource (and as a result, AioContexts) to it. As part of this series we also port the QEMU main loop to using QContext and drop virtio-blk's dataplane thread in favor of a GlibQContext thread, which virtio-blk dataplane attaches to via it's AioContext's GSource. With these patches in place we can do virtio-blk dataplane assignment like so: qemu ... \ -object glib-qcontext,id=ctx0,threaded=yes -drive file=file.raw,id=drive0,aio=native,cache=none \ -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0 And also gain the ability to assign a virtio-blk dataplane to the default QContext driven by the QEMU main iothread: qemu ... \ -drive file=file.raw,id=drive0,aio=native,cache=none \ -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main The latter likely isn't particularly desirable, and the series is in rough shape in general, but the goal of this RFC to demonstrate the approach and get some feedback on how we might handle thread assignments for things like virtio-blk/virtio-net dataplane, and multi-threaded device models general. Input on this would be greatly appreciated. BACKGROUND There has been an outgoing discussion on qemu-devel about what event loop interface to consolidate around for virtio-blk dataplane, threaded virtio-net, and offloading device workloads to seperate threads in general. Currently the main candidates seem to be GLib GSources and AioContext, with virtio-blk/virtio-net dataplane being the use-cases under consideration. virtio-blk: In the case of virtio-blk dataplane, we need to drive virtqueue and AIO events. Since AioContext is used extensively throughout the block layer to drive AIO, it makes sense to re-use it here as we look toward pushing dataplane functionality deeper into the block layer to benefit from things like image format support/snapshots/etc. virtio-net: In the case of Ping Fan's virtio-net dataplane patches (http://thread.gmane.org/gmane.comp.emulators.qemu/196111/focus=196111), we need to drive virtqueue and NetClient peer events (such as TAP devices, or possibly things like slirp in the future). Currently NetClient events rely on IOHandler interfaces such as qemu_set_fd_handler(). These interfaces are global ones that rely on a single IOHandler list serviced by QEMU's main loop. An effort is currently underway to port these to GSources so that can be more easilly attached to other event loops (as opposed to the hooks used for the virtio-net dataplane series). Theoretically, much of the latter (such as TAP devices) can also be done around AioContext with some minor changes, but other NetClient backends such as slirp lend themselves to more open-ended event loop interfaces like GSources. Other devices might also find themselves needing something more open-ended (a somewhat silly but present example being virtio-serial + GSource-driven chardev) QContext: Since the direction for the forseeable future will likely continue to be GSources for some things, AioContext for others, a way to reconcile these approaches would be the age-old approach of adding a layer of abstration on top of the 2 so that we can handle device/backend thread assignments using a general mechanism. Building around this abstration also leaves open the ability to deal with things like locking considerations for real-time support in the future. A reasonable start to modeling abstraction layer this would be the open-ended GMainContext/GSource approach that QEMU relies on already, which is what the QContext/QSource interfaces do (with some minor differences/additions such as QSources storing and opaque instead of the GSource-subclassing approach for GLib). I think, custom-ed function for readable or not , ex, tap_can_send() should be passed into QSource, but I failed to find such things in [PATCH 3/9] QSource: QEMU event source object. Do I miss it? We can actually do that via the prepare function. It gets called prior to polling
Re: [Qemu-devel] [PATCH 2/9] qom: add object_property_add_unnamed_child
On Mon, May 06, 2013 at 09:44:13AM +0200, Paolo Bonzini wrote:
Il 03/05/2013 18:03, Michael Roth ha scritto:
This interface allows us to add a child property without specifying a
name. Instead, a unique name is created and passed back after adding
the property.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
include/qom/object.h | 16
qom/object.c | 25 +
2 files changed, 41 insertions(+)
diff --git a/include/qom/object.h b/include/qom/object.h
index 86f1e2e..ca0fce8 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1041,6 +1041,22 @@ void object_property_add_child(Object *obj, const
char *name,
Object *child, struct Error **errp);
/**
+ * object_property_add_unnamed_child:
+ *
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @child: the child object
+ * @errp: if an error occurs, a pointer to an area to store the area
+ *
+ * Same as object_property_add_child, but will allocate a unique name to
+ * identify the child property.
+ *
+ * Returns: The name assigned to the child property, or NULL on failure.
+ */
+char *object_property_add_unnamed_child(Object *obj, Object *child,
+struct Error **errp);
+
+/**
* object_property_add_link:
* @obj: the object to add a property to
* @name: the name of the property
diff --git a/qom/object.c b/qom/object.c
index c932f64..229a9a7 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -926,6 +926,31 @@ static void object_finalize_child_property(Object
*obj, const char *name,
object_unref(child);
}
+char *object_property_add_unnamed_child(Object *obj, Object *child, Error
**errp)
+{
+int idx = 0;
+bool next_idx_found = false;
+char name[64];
+ObjectProperty *prop;
+
+while (!next_idx_found) {
+sprintf(name, unnamed[%d], idx);
+QTAILQ_FOREACH(prop, obj-properties, node) {
+if (strcmp(name, prop-name) == 0) {
+idx++;
+break;
+}
+}
+if (!prop) {
+next_idx_found = true;
+}
+}
+
+object_property_add_child(obj, name, child, errp);
+
+return error_is_set(errp) ? NULL : g_strdup(name);
+}
This is O(n^3) for adding N children. O(n^2) would be not-that-great
but fine; can you take the occasion to convert the properties list to a
hashtable?
Sure, I'll look into it.
Paolo
+
void object_property_add_child(Object *obj, const char *name,
Object *child, Error **errp)
{
Re: [Qemu-devel] [PATCH 1/9] qom: add qom_init_completion
On Mon, May 06, 2013 at 09:45:22AM +0200, Paolo Bonzini wrote:
Il 03/05/2013 18:03, Michael Roth ha scritto:
This is similar in concept to realize, though semantics are a
bit more open-ended:
And object might in some cases need a number of properties to be
specified before it can be used/started/etc. This can't always
be done via an open-ended new() function, the main example being objects
that around created via the command-line by -object.
To support these cases we allow a function, -instance_init_completion,
to be registered that will be called by the -object constructor, or can
be called at the end of new() constructors and such.
This seems a lot like a realize property that cannot be set back to false...
I seem to recall some other conditions like properties not being
modifiable after being realized? In this case though I think event loops
should be startable/stoppable via properties (post-migration, for
instance, or maybe testing/debugging) with simple qom-set commands.
Not too sure honestly, mainly I just recalled realize() being pushed
down into DeviceState for specific reasons, and didn't want to confuse
this with being the same thing (even though it does seem very similar).
I'm not sure what the best approach is here.
Paolo
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
include/qom/object.h | 19 +++
qom/object.c | 21 +
vl.c |2 ++
3 files changed, 42 insertions(+)
diff --git a/include/qom/object.h b/include/qom/object.h
index d0f99c5..86f1e2e 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -394,6 +394,11 @@ struct Object
* @instance_init: This function is called to initialize an object. The
parent
* class will have already been initialized so the type is only
responsible
* for initializing its own members.
+ * @instance_init_completion: This function is used mainly cases where an
+ * object has been instantiated via the command-line, and is called once
all
+ * properties specified via command-line have been set for the object.
This
+ * is not called automatically, but manually via @object_init_completion
once
+ * the processing of said properties is completed.
* @instance_finalize: This function is called during object destruction.
This
* is called before the parent @instance_finalize function has been
called.
* An object should only free the members that are unique to its type in
this
@@ -429,6 +434,7 @@ struct TypeInfo
size_t instance_size;
void (*instance_init)(Object *obj);
+void (*instance_init_completion)(Object *obj);
void (*instance_finalize)(Object *obj);
bool abstract;
@@ -562,6 +568,19 @@ struct InterfaceClass
Object *object_new(const char *typename);
/**
+ * object_init_completion:
+ * @obj: The object to complete initialization of
+ *
+ * In cases where an object is instantiated from a command-line with a
number
+ * of properties specified as parameters (generally via -object), or for
cases
+ * where a new()/helper function is used to pass/set some minimal number of
+ * properties that are required prior to completion of object
initialization,
+ * this function can be called to mark when that occurs to complete object
+ * initialization.
+ */
+void object_init_completion(Object *obj);
+
+/**
* object_new_with_type:
* @type: The type of the object to instantiate.
*
diff --git a/qom/object.c b/qom/object.c
index 75e6aac..c932f64 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -50,6 +50,7 @@ struct TypeImpl
void *class_data;
void (*instance_init)(Object *obj);
+void (*instance_init_completion)(Object *obj);
void (*instance_finalize)(Object *obj);
bool abstract;
@@ -110,6 +111,7 @@ static TypeImpl *type_register_internal(const TypeInfo
*info)
ti-class_data = info-class_data;
ti-instance_init = info-instance_init;
+ti-instance_init_completion = info-instance_init_completion;
ti-instance_finalize = info-instance_finalize;
ti-abstract = info-abstract;
@@ -422,6 +424,25 @@ Object *object_new(const char *typename)
return object_new_with_type(ti);
}
+
+static void object_init_completion_with_type(Object *obj, TypeImpl *ti)
+{
+if (type_has_parent(ti)) {
+object_init_completion_with_type(obj, type_get_parent(ti));
+}
+
+if (ti-instance_init_completion) {
+ti-instance_init_completion(obj);
+}
+}
+
+void object_init_completion(Object *obj)
+{
+TypeImpl *ti = type_get_by_name(object_get_class(obj)-type-name);
+
+object_init_completion_with_type(obj, ti);
+}
+
Object *object_dynamic_cast(Object *obj, const char *typename)
{
if (obj
Re: [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource
On Mon, May 06, 2013 at 09:53:12AM +0200, Paolo Bonzini wrote:
Il 03/05/2013 18:03, Michael Roth ha scritto:
This introduces a GlibQContext wrapper around the main GMainContext
event loop, and associates iohandlers with it via a QSource (which
GlibQContext creates a GSource from so that it can be driven via
GLib. A subsequent patch will drive the GlibQContext directly)
We also add QContext-aware functionality to iohandler interfaces
so that they can be bound to other QContext event loops, and add
non-global set_fd_handler() interfaces to facilitate this. This is made
possible by simply searching a given QContext for a QSource by the name
of iohandler so that we can attach event handlers to the associated
IOHandlerState.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
This patch is why I think that this is a bit overengineered. The main
loop is always glib-based, there should be no need to go through the
QSource abstraction.
BTW, this is broken for Win32. The right thing to do here is to first
convert iohandler to a GSource in such a way that it works for both
POSIX and Win32, and then (if needed) we can later convert GSource to
QSource.
Yup, forgot to note that Win32 was broken and on my TODO. I'll work on
that and stick to using GSources for now.
Paolo
---
include/qemu/main-loop.h | 31 +-
iohandler.c | 238
--
main-loop.c | 21 +++-
3 files changed, 213 insertions(+), 77 deletions(-)
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 6f0200a..dbadf9f 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -26,6 +26,7 @@
#define QEMU_MAIN_LOOP_H 1
#include block/aio.h
+#include qcontext/qcontext.h
#define SIG_IPI SIGUSR1
@@ -168,9 +169,24 @@ void qemu_del_wait_object(HANDLE handle,
WaitObjectFunc *func, void *opaque);
/* async I/O support */
+#define QSOURCE_IOHANDLER iohandler
+
typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
typedef int IOCanReadHandler(void *opaque);
+QContext *qemu_get_qcontext(void);
+/**
+ * iohandler_attach: Attach a QSource to a QContext
+ *
+ * This enables the use of IOHandler interfaces such as
+ * set_fd_handler() on the given QContext. IOHandler lists will be
+ * tracked/handled/dispatched based on a named QSource that is added to
+ * the QContext
+ *
+ * @ctx: A QContext to add an IOHandler QSource to
+ */
+void iohandler_attach(QContext *ctx);
+
/**
* qemu_set_fd_handler2: Register a file descriptor with the main loop
*
@@ -217,6 +233,13 @@ int qemu_set_fd_handler2(int fd,
IOHandler *fd_write,
void *opaque);
+int set_fd_handler2(QContext *ctx,
+int fd,
+IOCanReadHandler *fd_read_poll,
+IOHandler *fd_read,
+IOHandler *fd_write,
+void *opaque);
+
/**
* qemu_set_fd_handler: Register a file descriptor with the main loop
*
@@ -250,6 +273,12 @@ int qemu_set_fd_handler(int fd,
IOHandler *fd_write,
void *opaque);
+int set_fd_handler(QContext *ctx,
+ int fd,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque);
+
#ifdef CONFIG_POSIX
/**
* qemu_add_child_watch: Register a child process for reaping.
@@ -302,8 +331,6 @@ void qemu_mutex_unlock_iothread(void);
/* internal interfaces */
void qemu_fd_register(int fd);
-void qemu_iohandler_fill(GArray *pollfds);
-void qemu_iohandler_poll(GArray *pollfds, int rc);
QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque);
void qemu_bh_schedule_idle(QEMUBH *bh);
diff --git a/iohandler.c b/iohandler.c
index ae2ef8f..8625272 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -41,38 +41,170 @@ typedef struct IOHandlerRecord {
int fd;
int pollfds_idx;
bool deleted;
+GPollFD pfd;
+bool pfd_added;
} IOHandlerRecord;
-static QLIST_HEAD(, IOHandlerRecord) io_handlers =
-QLIST_HEAD_INITIALIZER(io_handlers);
+typedef struct IOHandlerState {
+QLIST_HEAD(, IOHandlerRecord) io_handlers;
+} IOHandlerState;
+static bool iohandler_prepare(QSource *qsource, int *timeout)
+{
+QSourceClass *qsourcek = QSOURCE_GET_CLASS(qsource);
+IOHandlerState *s = qsourcek-get_user_data(qsource);
+IOHandlerRecord *ioh;
-/* XXX: fd_read_poll should be suppressed, but an API change is
- necessary in the character devices to suppress fd_can_read(). */
-int qemu_set_fd_handler2(int fd,
- IOCanReadHandler *fd_read_poll,
- IOHandler *fd_read,
-
Re: [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in place of custom thread
On Mon, May 06, 2013 at 09:54:03AM +0200, Paolo Bonzini wrote:
Il 03/05/2013 18:03, Michael Roth ha scritto:
virtio-blk dataplane currently creates/manages it's own thread to
offload work to a separate event loop.
This patch insteads allows us to specify a QContext-based event loop by
adding a context property for virtio-blk we can use like so:
qemu ... \
-object glib-qcontext,id=ctx0,threaded=yes
-drive file=file.raw,id=drive0,aio=native,cache=none \
-device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0
virtio-blk dataplane then simply attachs/detaches it's AioContext to the
ctx0 event loop on start/stop.
This also makes available the option to drive a virtio-blk dataplane via
the default main loop:
qemu ... \
-drive file=file.raw,id=drive0,aio=native,cache=none \
-device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main
This doesn't do much in and of itself, but helps to demonstrate how we
might model a general mechanism to offload device workloads to separate
threads.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
hw/block/dataplane/virtio-blk.c | 46
---
include/hw/virtio/virtio-blk.h |7 --
2 files changed, 19 insertions(+), 34 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c
b/hw/block/dataplane/virtio-blk.c
index 0356665..08ea10f 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -24,6 +24,8 @@
#include virtio-blk.h
#include block/aio.h
#include hw/virtio/virtio-bus.h
+#include qcontext/qcontext.h
+#include qcontext/glib-qcontext.h
enum {
SEG_MAX = 126, /* maximum number of I/O segments */
@@ -60,6 +62,7 @@ struct VirtIOBlockDataPlane {
* use it).
*/
AioContext *ctx;
+QContext *qctx;
EventNotifier io_notifier; /* Linux AIO completion */
EventNotifier host_notifier;/* doorbell */
@@ -375,26 +378,6 @@ static void handle_io(EventNotifier *e)
}
}
-static void *data_plane_thread(void *opaque)
-{
-VirtIOBlockDataPlane *s = opaque;
-
-do {
-aio_poll(s-ctx, true);
-} while (!s-stopping || s-num_reqs 0);
-return NULL;
-}
-
-static void start_data_plane_bh(void *opaque)
-{
-VirtIOBlockDataPlane *s = opaque;
-
-qemu_bh_delete(s-start_bh);
-s-start_bh = NULL;
-qemu_thread_create(s-thread, data_plane_thread,
- s, QEMU_THREAD_JOINABLE);
-}
-
bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
VirtIOBlockDataPlane **dataplane)
{
@@ -460,6 +443,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane
*s)
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
VirtQueue *vq;
int i;
+Error *err = NULL;
if (s-started) {
return;
@@ -502,9 +486,16 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane
*s)
/* Kick right away to begin processing requests already in vring */
event_notifier_set(virtio_queue_get_host_notifier(vq));
-/* Spawn thread in BH so it inherits iothread cpusets */
-s-start_bh = qemu_bh_new(start_data_plane_bh, s);
-qemu_bh_schedule(s-start_bh);
+/* use QEMU main loop/context by default */
+if (!s-blk-context) {
+s-blk-context = g_strdup(main);
+}
Or rather create a device-specific context by default?
Yup, definitely.
I think I did it this way to to give an idea of how a normal threaded device
might look (i.e. reworked or written from the start to always be capable of
being driven by a separate event loop)
But x-data-plane=on should imply a new context be used so we don't break
things, and I'll do it that way on the next pass.
Paolo
+s-qctx = qcontext_find_by_name(s-blk-context, err);
+if (err) {
+fprintf(stderr, virtio-blk failed to start: %s,
error_get_pretty(err));
+exit(1);
+}
+aio_context_attach(s-ctx, s-qctx);
}
void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
@@ -517,15 +508,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane
*s)
s-stopping = true;
trace_virtio_blk_data_plane_stop(s);
-/* Stop thread or cancel pending thread creation BH */
-if (s-start_bh) {
-qemu_bh_delete(s-start_bh);
-s-start_bh = NULL;
-} else {
-aio_notify(s-ctx);
-qemu_thread_join(s-thread);
-}
-
aio_set_event_notifier(s-ctx, s-io_notifier, NULL, NULL);
ioq_cleanup(s-ioqueue);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index fc71853..c5514a4 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -110,6 +110,7 @@ struct
Re: [Qemu-devel] [PATCH v2] qapi: add struct strList and visit_type_strList()
On Thu, Apr 25, 2013 at 12:08:05AM +0800, Amos Kong wrote:
Currently we can only use ['String'] to add string to a list,
it contains some additional JSON structure.
multicast: [
{
str: 01:80:c2:00:00:21
},
{
str: 00:00:00:00:00:00
}
]
This patch adds struct strList and visit function, we can use ['str'] in
schema file.
multicast: [
01:00:5e:00:00:01,
33:33:ff:12:34:57
]
V2: remove ugly #ifndef, add forward declaration in qapi-types.h,
and define the struct in include/qapi/visitor.h (Paolo)
Signed-off-by: Amos Kong ak...@redhat.com
Sorry for the delay in getting back to you, I started to respond last
week and ended up hacking something up that take a different approach.
Namely: we don't hardcode visit_type_strList(), but instead teach the
qapi code generators about native types we currently allow in schemas
('str', 'int', 'number', 'bool') and have them handle code generation
for these like they would for any user-defined type.
This approach works too, but I think this solution is more complete.
I'm still working out some bits with the unit tests but I've pushed my
WIP here for reference:
https://github.com/mdroth/qemu/commits/qapi-native-lists
Please give that a shot.
---
include/qapi/visitor.h |7 +++
qapi/qapi-visit-core.c | 23 +++
scripts/qapi-types.py |1 +
scripts/qapi.py|4 ++--
4 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1fef18c..743ff92 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -22,6 +22,11 @@ typedef struct GenericList
struct GenericList *next;
} GenericList;
+typedef struct strList {
+char *value;
+struct strList *next;
+} strList;
+
typedef struct Visitor Visitor;
void visit_start_handle(Visitor *v, void **obj, const char *kind,
@@ -50,6 +55,8 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char
*name, Error **errp);
void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error
**errp);
void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
+void visit_type_strList(Visitor *m, strList ** obj, const char *name,
+Error **errp);
void visit_type_number(Visitor *v, double *obj, const char *name, Error
**errp);
#endif
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 401ee6e..dc54cc8 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -257,6 +257,29 @@ void visit_type_str(Visitor *v, char **obj, const char
*name, Error **errp)
}
}
+void visit_type_strList(Visitor *m, strList ** obj, const char *name,
+Error **errp)
+{
+GenericList *i, **prev = (GenericList **)obj;
+Error *err = NULL;
+
+if (!error_is_set(errp)) {
+visit_start_list(m, name, err);
+if (!err) {
+for (; (i = visit_next_list(m, prev, err)) != NULL; prev = i) {
+strList *native_i = (strList *)i;
+visit_type_str(m, native_i-value, NULL, err);
+}
+error_propagate(errp, err);
+err = NULL;
+
+/* Always call end_list if start_list succeeded. */
+visit_end_list(m, err);
+}
+error_propagate(errp, err);
+}
+}
+
void visit_type_number(Visitor *v, double *obj, const char *name, Error
**errp)
{
if (!error_is_set(errp)) {
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9e19920..e449a14 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -276,6 +276,7 @@ fdecl.write(mcgen('''
#include stdbool.h
#include stdint.h
+struct strList;
''',
guard=guardname(h_file)))
diff --git a/scripts/qapi.py b/scripts/qapi.py
index afc5f32..14f9f4d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -166,11 +166,11 @@ def c_fun(name, protect=True):
return c_var(name, protect).replace('.', '_')
def c_list_type(name):
-return '%sList' % name
+return 'struct %sList' % name
def type_name(name):
if type(name) == list:
-return c_list_type(name[0])
+return '%sList' % name[0]
return name
enum_types = []
--
1.7.1
Re: [Qemu-devel] [qapi] Cannot use list of strings
On Tue, Apr 16, 2013 at 10:49:19AM +0200, Stefan Hajnoczi wrote:
On Mon, Apr 15, 2013 at 10:04:24PM +0200, Lluís Vilanova wrote:
Tried using a list of strings as an argument to a command, but the generated
code references the 'strList' type, which does not exist.
Is a specialized version for ['str'] missing, or should I define my own
type
with a single field of 'str' type?
I would say just give that a shot. Stick:
typedef struct strList
{
char *value;
struct strList *next;
} strList;
in include/qapi/visitor.h and see what happens.
It's not immediately obvious to me why that wouldn't work, except maybe
that sometimes the code generators will special case on CamelCase types,
but that shouldn't be too hard to work around if it's an issue.
If that works, the same approach can probably be taken for all native qapi
types: str, bool, [u]int[(8|16|32|64)]
akong just hit this too.
I think it's a question for aliguori, luiz, or mdroth.
Stefan
[Qemu-devel] [ANNOUNCE] QEMU 1.4.1 Stable released
Hi everyone, I am pleased to announce that the QEMU v1.4.1 stable release is now available at: http://wiki.qemu.org/download/qemu-1.4.1.tar.bz2 The official stable-1.4 repository has also been updated to v1.4.1: http://git.qemu.org/?p=qemu-stable-1.4.git;a=summary This release includes 57 build/bug fixes, including an important security fix for guests using qemu-nbd images. QEMU v1.4.2 is planned for 2013-05-24. Thanks! 57105f7: update VERSION for 1.4.1 (Michael Roth) 6e88653: Add -f FMT / --format FMT arg to qemu-nbd (Daniel P. Berrange) 6d0b135: target-mips: Fix accumulator selection for MIPS16 and microMIPS (Richard Sandiford) d89f9ba: Allow clock_gettime() monotonic clock to be utilized on more OS's (Brad Smith) 46f9071: target-i386: Check for host features before filter_features_for_kvm() (Eduardo Habkost) f85e082: help: add docs for missing 'queues' option of tap (Jason Wang) da78a1b: compiler: fix warning with GCC 4.8.0 (Paolo Bonzini) 2b92aa3: block: complete all IOs before resizing a device (Peter Lieven) e4cce2d: Revert block: complete all IOs before .bdrv_truncate (Peter Lieven) d15b1aa: qxl: better vga init in enter_vga_mode (Gerd Hoffmann) 65fe29e: doc: Fix texinfo @table markup in qemu-options.hx (Markus Armbruster) 888e036: acpi: initialize s4_val used in s4 shutdown (Bruce Rogers) d019dd9: target-mips: fix rndrashift_short_acc and code for EXTR_ instructions (Petar Jovanovic) dac077f: target-mips: fix DSP overflow macro and affected routines (Petar Jovanovic) b09a673: target-mips: fix for sign-issue in MULQ_W helper (Petar Jovanovic) 79a4dd4: target-mips: fix for incorrect multiplication with MULQ_S.PH (Petar Jovanovic) 57e929c: usb-tablet: Don't claim wakeup capability for USB-2 version (Hans de Goede) 27c7135: chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors (Stefan Hajnoczi) 283b7de: qemu-socket: set passed fd non-blocking in socket_connect() (Stefan Hajnoczi) a1cb89f: net: ensure socket backend uses non-blocking fds (Stefan Hajnoczi) 68f9df5: oslib-posix: rename socket_set_nonblock() to qemu_set_nonblock() (Stefan Hajnoczi) 0135796: update seabios to 1.7.2.1 (Gerd Hoffmann) 799a34a: linux-user/syscall.c: Don't warn about unimplemented get_robust_list (Peter Maydell) 8378910: linux-user: make bogus negative iovec lengths fail EINVAL (Peter Maydell) 7a238b9: linux-user: fix futex strace of FUTEX_CLOCK_REALTIME (John Rigby) 02493ee: linux-user/syscall.c: handle FUTEX_WAIT_BITSET in do_futex (John Rigby) 7d47b24: qcow2: flush refcount cache correctly in qcow2_write_snapshots() (Stefan Hajnoczi) 02ea844: qcow2: flush refcount cache correctly in alloc_refcount_block() (Stefan Hajnoczi) 0fcf00b: page_cache: fix memory leak (Peter Lieven) 5610ef5: Fix page_cache leak in cache_resize (Orit Wasserman) 7a687ae: virtio-blk: fix unplug + virsh reboot (Christian Borntraeger) b91aee5: ide/macio: Fix macio DMA initialisation. (Mark Cave-Ayland) e09b99b: target-ppc: Fix CPU_POWERPC_MPC8547E (Andreas Färber) 611c7f2: pseries: Add cleanup hook for PAPR virtual LAN device (David Gibson) 4e4566c: configure: Require at least spice-protocol-0.12.3 (Michal Privoznik) 43e0061: qemu-bridge-helper: force usage of a very high MAC address for the bridge (Paolo Bonzini) 3c3de7c: virtio-ccw: Queue sanity check for notify hypercall. (Cornelia Huck) b0da310: tcg: Fix occasional TCG broken problem when ldst optimization enabled (Yeongkyoon Lee) d26efd2: qga/main.c: Don't use g_key_file_get/set_int64 (Peter Crosthwaite) f305d50: qemu-ga: use key-value store to avoid recycling fd handles after restart (Michael Roth) d3652a1: qcow2: make is_allocated return true for zero clusters (Paolo Bonzini) 5194350: pseries: Add compatible property to root of device tree (David Gibson) 4d1cdb9: Allow virtio-net features for legacy s390 virtio bus (Christian Borntraeger) c3b81e0: rtc-test: Fix test failures with recent glib (Cole Robinson) 99b1f39: scsi-disk: do not complete canceled UNMAP requests (Paolo Bonzini) f23ab03: scsi: do not call scsi_read_data/scsi_write_data for a canceled request (Paolo Bonzini) 0c918dd: iscsi: look for pkg-config file too (Paolo Bonzini) a8b090e: scsi-disk: handle io_canceled uniformly and correctly (Paolo Bonzini) 4a38944: qemu-ga: make guest-sync-delimited available during fsfreeze (Michael Roth) b7ff1a7: qmp: netdev_add is like -netdev, not -net, fix documentation (Markus Armbruster) d49fed4: vga: fix byteswapping. (Gerd Hoffmann) cebb8eb: help: add docs for multiqueue tap options (Jason Wang) 3b39a11: net: reduce the unnecessary memory allocation of multiqueue (Jason Wang) ec9f828: qemu-char.c: fix waiting for telnet connection message (Igor Mitsyanko) 332e934: tap: forbid creating multiqueue tap when hub is used (Jason Wang) e6b795f: block: complete all IOs before .bdrv_truncate (Peter Lieven) 51968b8: coroutine: trim down nesting level in perf_nesting test (Paolo Bonzini) 80d8b5d: target-ppc: Fix G2leGP3 PVR (Andreas Färber)
Re: [Qemu-devel] Patch Round-up for stable 1.4.1, freeze next Tuesday
On Tue, Apr 02, 2013 at 04:45:05PM -0500, Michael Roth wrote: Hi everyone, The following new patches are queued for QEMU stable v1.4.1: https://github.com/mdroth/qemu/commits/stable-1.4-staging The release is planned for 04-15-2013: http://wiki.qemu.org/Planning/1.4 Please CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 04-09-2013 for new patches. The 1.4 staging tree should now include all patches suggested so far, except for the 2 where I noted otherwise: https://github.com/mdroth/qemu/commits/stable-1.4-staging Testing/feedback is greatly appreciated. Thanks! Andreas Färber (2): target-ppc: Fix G2leGP3 PVR target-ppc: Fix CPU_POWERPC_MPC8547E Christian Borntraeger (2): Allow virtio-net features for legacy s390 virtio bus virtio-blk: fix unplug + virsh reboot Cole Robinson (1): rtc-test: Fix test failures with recent glib Cornelia Huck (1): virtio-ccw: Queue sanity check for notify hypercall. David Gibson (2): pseries: Add compatible property to root of device tree pseries: Add cleanup hook for PAPR virtual LAN device Gerd Hoffmann (2): vga: fix byteswapping. update seabios to 1.7.2.1 Igor Mitsyanko (1): qemu-char.c: fix waiting for telnet connection message Jason Wang (3): tap: forbid creating multiqueue tap when hub is used net: reduce the unnecessary memory allocation of multiqueue help: add docs for multiqueue tap options John Rigby (2): linux-user/syscall.c: handle FUTEX_WAIT_BITSET in do_futex linux-user: fix futex strace of FUTEX_CLOCK_REALTIME Mark Cave-Ayland (1): ide/macio: Fix macio DMA initialisation. Markus Armbruster (1): qmp: netdev_add is like -netdev, not -net, fix documentation Michael Roth (2): qemu-ga: make guest-sync-delimited available during fsfreeze qemu-ga: use key-value store to avoid recycling fd handles after restart Michal Privoznik (1): configure: Require at least spice-protocol-0.12.3 Orit Wasserman (1): Fix page_cache leak in cache_resize Paolo Bonzini (7): coroutine: trim down nesting level in perf_nesting test scsi-disk: handle io_canceled uniformly and correctly iscsi: look for pkg-config file too scsi: do not call scsi_read_data/scsi_write_data for a canceled request scsi-disk: do not complete canceled UNMAP requests qcow2: make is_allocated return true for zero clusters qemu-bridge-helper: force usage of a very high MAC address for the bridge Peter Crosthwaite (1): qga/main.c: Don't use g_key_file_get/set_int64 Peter Lieven (2): block: complete all IOs before .bdrv_truncate page_cache: fix memory leak Peter Maydell (2): linux-user: make bogus negative iovec lengths fail EINVAL linux-user/syscall.c: Don't warn about unimplemented get_robust_list Stefan Hajnoczi (2): qcow2: flush refcount cache correctly in alloc_refcount_block() qcow2: flush refcount cache correctly in qcow2_write_snapshots() Yeongkyoon Lee (1): tcg: Fix occasional TCG broken problem when ldst optimization enabled block.c |4 + block/qcow2-cluster.c |3 + block/qcow2-refcount.c | 10 ++- block/qcow2-snapshot.c |5 +- block/qcow2.c |6 +- configure | 10 ++- hw/macio.c |2 +- hw/qxl-render.c |3 +- hw/s390x/s390-virtio-bus.c |1 + hw/s390x/s390-virtio-ccw.c |3 + hw/scsi-bus.c |4 + hw/scsi-disk.c | 36 +++-- hw/spapr.c |1 + hw/spapr_llan.c |8 ++ hw/vga.c| 18 ++--- hw/virtio-blk.c |4 +- hw/virtio-net.c |6 +- hw/vmware_vga.c |2 +- hw/xenfb.c |3 +- include/net/net.h |2 +- include/ui/console.h|3 +- linux-user/strace.c |6 ++ linux-user/syscall.c| 20 - net/net.c | 19 +++-- net/tap.c |7 ++ page_cache.c|4 + pc-bios/bios.bin| Bin 131072 - 131072 bytes qapi-schema.json|6 ++ qemu-bridge-helper.c| 18 + qemu-char.c |2 +- qemu-options.hx |4 +- qga/commands-posix.c| 25 -- qga/guest-agent-core.h |1 + qga/main.c | 185 +++ qmp-commands.hx |2 +- roms/seabios|2 +- target-ppc/translate_init.c |4 +- tests/rtc-test.c|4 +- tests/test-coroutine.c |2 +- trace-events|1 + translate-all.c |4
Re: [Qemu-devel] Patch Round-up for stable 1.4.1, freeze next Tuesday
On Wed, Apr 03, 2013 at 11:51:31PM +0200, Aurelien Jarno wrote: On Tue, Apr 02, 2013 at 04:45:05PM -0500, Michael Roth wrote: Hi everyone, The following new patches are queued for QEMU stable v1.4.1: https://github.com/mdroth/qemu/commits/stable-1.4-staging The release is planned for 04-15-2013: http://wiki.qemu.org/Planning/1.4 Please CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 04-09-2013 for new patches. For the MIPS emulation, you might want to also include: 9c19eb1e205b29018f6f61c5f43db6abbe7dc0e5 target-mips: fix for incorrect multiplication with MULQ_S.PH a345481baa2b2fb3d54f8c9ddb58dfcaf75786df target-mips: fix for sign-issue in MULQ_W helper 20c334a797bf46a4ee59a6e42be6d5e7c3cda585 target-mips: fix DSP overflow macro and affected routines 26135ead80fa1fd13e95c162dacfd06f2ba82981 target-mips: Fix accumulator selection for MIPS16 and microMIPS This guy ^ has some nasty conflicts I don't trust myself with. Can you send a version that's backported to v1.4.0 via git cherry-pick -x? 8b758d0568a986d58c254b3c209691c82e0f82a1 target-mips: fix rndrashift_short_acc and code for EXTR_ instructions They are five fixing instruction emulation. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH stable-1.4 v2 0/4] monitor: do not rely on O_NONBLOCK for passed file descriptors
On Thu, Apr 04, 2013 at 04:18:27PM +0200, Stefan Hajnoczi wrote: Backported to QEMU 1.4 stable branch. Original series: http://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg04756.html v2: * Use git cherry-pick -x and give rationale for conflict resolutions. Applied to the 1.4.1 staging tree: https://github.com/mdroth/qemu/commits/stable-1.4-staging Thanks for the backport! Stefan Hajnoczi (4): oslib-posix: rename socket_set_nonblock() to qemu_set_nonblock() net: ensure socket backend uses non-blocking fds qemu-socket: set passed fd non-blocking in socket_connect() chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors block/nbd.c| 2 +- block/sheepdog.c | 6 +++--- include/qemu/sockets.h | 4 ++-- migration-tcp.c| 2 +- migration-unix.c | 2 +- migration.c| 2 +- nbd.c | 8 net/socket.c | 13 + qemu-char.c| 11 +++ slirp/misc.c | 2 +- slirp/tcp_subr.c | 4 ++-- ui/vnc.c | 2 +- util/oslib-posix.c | 4 ++-- util/oslib-win32.c | 4 ++-- util/qemu-sockets.c| 5 +++-- 15 files changed, 40 insertions(+), 31 deletions(-) -- 1.8.1.4
Re: [Qemu-devel] Patch Round-up for stable 1.4.1, freeze next Tuesday
On Thu, Apr 04, 2013 at 07:55:02AM +0200, Peter Lieven wrote: On 02.04.2013 23:45, Michael Roth wrote: Hi everyone, The following new patches are queued for QEMU stable v1.4.1: https://github.com/mdroth/qemu/commits/stable-1.4-staging The release is planned for 04-15-2013: http://wiki.qemu.org/Planning/1.4 Please CC qemu-sta...@nongnu.org on any patches you think should be included in the release. The cut-off date is 04-09-2013 for new patches. Testing/feedback is greatly appreciated. Please include 5c91668 Revert block: complete all IOs before .bdrv_truncate 92b7a08 block: complete all IOs before resizing a device 142c6b1 vl.c: call bdrv_init_with_whitelist() before cmdline parsing This one ^ causes a segfault on top of 1.4.0: mdroth@loki:~/w/qemu-build-stable$ gdb --args x86_64-softmmu/qemu-system-x86_64 GNU gdb (Ubuntu/Linaro 7.4-2012.04-0ubuntu2.1) 7.4-2012.04 Copyright (C) 2012 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type show copying and show warranty for details. This GDB was configured as x86_64-linux-gnu. For bug reporting instructions, please see: http://bugs.launchpad.net/gdb-linaro/... Reading symbols from /home/mdroth/dev/kvm/qemu-build-stable/x86_64-softmmu/qemu-system-x86_64...done. (gdb) run Starting program: /home/mdroth/dev/kvm/qemu-build-stable/x86_64-softmmu/qemu-system-x86_64 [Thread debugging using libthread_db enabled] Using host libthread_db library /lib/x86_64-linux-gnu/libthread_db.so.1. Program received signal SIGSEGV, Segmentation fault. aio_set_fd_handler (ctx=0x0, fd=8, io_read=0x5570a2d0 event_notifier_ready, io_write=0, io_flush=0x5570a1d0 thread_pool_active, opaque=0x5600b9e0) at /home/mdroth/w/qemu-stable.git/aio-posix.c:269 269 } (gdb) bt #0 aio_set_fd_handler (ctx=0x0, fd=8, io_read=0x5570a2d0 event_notifier_ready, io_write=0, io_flush=0x5570a1d0 thread_pool_active, opaque=0x5600b9e0) at /home/mdroth/w/qemu-stable.git/aio-posix.c:269 #1 0x5570a27d in thread_pool_init () at /home/mdroth/w/qemu-stable.git/thread-pool.c:282 #2 0x5583529a in module_call_init (type=optimized out) at /home/mdroth/w/qemu-stable.git/util/module.c:79 #3 0x555c6ad1 in main (argc=1, argv=0x7fffe3c8, envp=optimized out) at /home/mdroth/w/qemu-stable.git/vl.c:2890 (gdb) From what I can tell initialization to before qemu_init_main_loop() causes thread-pools block_init() function to get called before qemu_aio_context is set. This isn't an issue upstream due to the following patch: f7311ccc630d925e7351e9440b7ad8bc6f0a51de Which I think is out of scope for stable. Not sure what the right approach is here. Peter
Re: [Qemu-devel] [PATCH v2 0/4] monitor: do not rely on O_NONBLOCK for passed file descriptors
On Wed, Mar 27, 2013 at 09:17:30AM -0400, Luiz Capitulino wrote: On Wed, 27 Mar 2013 10:10:42 +0100 Stefan Hajnoczi stefa...@redhat.com wrote: There are several places where QEMU accidentally relies on the O_NONBLOCK state of passed file descriptors. Exposing O_NONBLOCK state makes it part of the QMP API whenever getfd or fdset_add_fd are used! Whether or not QEMU will use O_NONBLOCK is an implementation detail and should be hidden from QMP clients. This patch series addresses this in 3 steps: Nice series: Applied to the qmp branch, thanks. Hi Luiz, Eric/mjt have noted that this series fixes a number of issues in 1.4.0 and have requested it for 1.4.1 http://thread.gmane.org/gmane.comp.emulators.qemu/203851 The cut-off for 1.4.1 is Tuesday. Do you plan to send a pull for these soon? 1. Fix callers of monitor_handle_fd_param(), monitor_fdset_get_fd(), and monitor_get_fd() that depend on O_NONBLOCK being set. Luckily there are only two instances and they are fixed in Patches 1 2. 2. Rename socket_set_nonblock() to qemu_set_nonblock() just like qemu_set_cloexec(). This makes code cleaner when working with arbitrary file descriptors that may not be sockets. See Patch 3. 3. Clear O_NONBLOCK when a chardev receives file descriptors. From now on QEMU can assume that passed file descriptors are in blocking mode. Simply use qemu_set_nonblock(fd) if you want to enable O_NONBLOCK. See Patch 4. This fixes live migration with recent libvirt. Libvirt checks if QEMU supports file descriptor passing and, if yes, hands QEMU a socket with O_NONBLOCK set. The migrate fd:foo code assumes the socket is in blocking mode. The result is a corrupted migration stream. For more info on this bug, see: https://bugzilla.redhat.com/show_bug.cgi?id=923124 Note that Michal Privoznik mpriv...@redhat.com also sent a libvirt patch so that old QEMUs work with new libvirts: https://www.redhat.com/archives/libvir-list/2013-March/msg01486.html My patch series fixes the QMP API and allows old libvirts to work again with new QEMUs. v2: * Rename socket_set_nonblock() in Patch 1 to avoid code churn [eblake] * Avoid qemu_set_block(-1) calls that clobber errno [quintela] Stefan Hajnoczi (4): oslib-posix: rename socket_set_nonblock() to qemu_set_nonblock() net: ensure socket backend uses non-blocking fds qemu-socket: set passed fd non-blocking in socket_connect() chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors block/nbd.c| 2 +- block/sheepdog.c | 2 +- include/qemu/sockets.h | 4 ++-- migration.c| 2 +- nbd.c | 8 net/socket.c | 13 + qemu-char.c| 11 +++ savevm.c | 2 +- slirp/misc.c | 2 +- slirp/tcp_subr.c | 4 ++-- ui/vnc.c | 2 +- util/oslib-posix.c | 4 ++-- util/oslib-win32.c | 4 ++-- util/qemu-sockets.c| 5 +++-- 14 files changed, 37 insertions(+), 28 deletions(-)
Re: [Qemu-devel] [PATCH] hw/mc146818rtc.c: Fix reading and writing of time registers
On Thu, Feb 14, 2013 at 08:54:20AM +0100, Antoine Mathys wrote:
This patch consolidates the bit twidling involved in reading and
writing the time registers to four functions that are used consistently.
This also has the effect of fixing bug 1090558.
Signed-off-by: Antoine Mathys barsa...@gmail.com
This issue still seems to be present upstream and I'm looking at pulling
it in for stable. Does anyone want to apply this, or are we still waiting
on a test case for the bug it fixes?
---
hw/mc146818rtc.c | 163
+-
1 file changed, 99 insertions(+), 64 deletions(-)
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2fb11f6..646cbd0 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -86,8 +86,14 @@ typedef struct RTCState {
static void rtc_set_time(RTCState *s);
static void rtc_update_time(RTCState *s);
+
+/* data encoding / decoding */
+static inline uint8_t rtc_encode(RTCState *s, uint8_t a);
+static inline int rtc_decode(RTCState *s, uint8_t a);
+static inline uint8_t rtc_hour_encode(RTCState *s, uint8_t hour);
+static inline int rtc_hour_decode(RTCState *s, uint8_t a);
+
static void rtc_set_cmos(RTCState *s, const struct tm *tm);
-static inline int rtc_from_bcd(RTCState *s, int a);
static uint64_t get_next_alarm(RTCState *s);
static inline bool rtc_running(RTCState *s)
@@ -254,17 +260,84 @@ static void check_update_timer(RTCState *s)
}
}
-static inline uint8_t convert_hour(RTCState *s, uint8_t hour)
+/* data encoding / decoding */
+
+static inline uint8_t rtc_encode(RTCState *s, uint8_t a)
+{
+if (s-cmos_data[RTC_REG_B] REG_B_DM) {
+return a;
+} else {
+return to_bcd(a);
+}
+}
+
+static inline int rtc_decode(RTCState *s, uint8_t a)
+{
+if ((a 0xc0) == 0xc0) {
+return -1;
+}
+if (s-cmos_data[RTC_REG_B] REG_B_DM) {
+return a;
+} else {
+return from_bcd(a);
+}
+}
+
+/*
+ Note: The next two functions implement the following mapping between
+ the 12 hour and 24 hour formats:
+
+ 0-12 AM
+ 1-11 -1 - 11 AM
+ 12 -12 PM
+ 13-23-1 - 11 PM
+*/
+
+static inline uint8_t rtc_hour_encode(RTCState *s, uint8_t hour)
+{
+uint8_t tmp;
+
+if (s-cmos_data[RTC_REG_B] REG_B_24H) {
+/* 24 hour format */
+tmp = rtc_encode(s, hour);
+} else {
+/* 12 hour format */
+uint8_t h = (hour % 12) ? (hour % 12) : 12;
+tmp = rtc_encode(s, h);
+if (hour = 12) {
+tmp |= 0x80;
+}
+}
+return tmp;
+}
+
+static inline int rtc_hour_decode(RTCState *s, uint8_t a)
{
-if (!(s-cmos_data[RTC_REG_B] REG_B_24H)) {
+uint8_t hour;
+
+/* Note: in 12 hour mode we clear bit 7 before calling
+ rtc_decode(), hence we cannot rely on the later to check the
+ don't care condition. While we could skip this check in 24 hour
+ mode it is simpler to do it in any case. */
+if ((a 0xc0) == 0xc0) {
+return -1;
+}
+
+if (s-cmos_data[RTC_REG_B] REG_B_24H) {
+/* 24 hour format */
+hour = rtc_decode(s, a);
+} else {
+/* 12 hour format */
+hour = rtc_decode(s, a 0x7f);
hour %= 12;
-if (s-cmos_data[RTC_HOURS] 0x80) {
+if (a 0x80) {
hour += 12;
}
}
return hour;
}
+
static uint64_t get_next_alarm(RTCState *s)
{
int32_t alarm_sec, alarm_min, alarm_hour, cur_hour, cur_min, cur_sec;
@@ -272,15 +345,13 @@ static uint64_t get_next_alarm(RTCState *s)
rtc_update_time(s);
-alarm_sec = rtc_from_bcd(s, s-cmos_data[RTC_SECONDS_ALARM]);
-alarm_min = rtc_from_bcd(s, s-cmos_data[RTC_MINUTES_ALARM]);
-alarm_hour = rtc_from_bcd(s, s-cmos_data[RTC_HOURS_ALARM]);
-alarm_hour = alarm_hour == -1 ? -1 : convert_hour(s, alarm_hour);
+alarm_sec = rtc_decode(s, s-cmos_data[RTC_SECONDS_ALARM]);
+alarm_min = rtc_decode(s, s-cmos_data[RTC_MINUTES_ALARM]);
+alarm_hour = rtc_hour_decode(s, s-cmos_data[RTC_HOURS_ALARM]);
-cur_sec = rtc_from_bcd(s, s-cmos_data[RTC_SECONDS]);
-cur_min = rtc_from_bcd(s, s-cmos_data[RTC_MINUTES]);
-cur_hour = rtc_from_bcd(s, s-cmos_data[RTC_HOURS]);
-cur_hour = convert_hour(s, cur_hour);
+cur_sec = rtc_decode(s, s-cmos_data[RTC_SECONDS]);
+cur_min = rtc_decode(s, s-cmos_data[RTC_MINUTES]);
+cur_hour = rtc_hour_decode(s, s-cmos_data[RTC_HOURS]);
if (alarm_hour == -1) {
alarm_hour = cur_hour;
@@ -486,44 +557,17 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
}
}
-static inline int rtc_to_bcd(RTCState *s, int a)
-{
-if (s-cmos_data[RTC_REG_B] REG_B_DM) {
-return a;
-} else {
-return ((a / 10) 4) | (a % 10);
-}
-}
-
-static inline int rtc_from_bcd(RTCState *s,
Re: [Qemu-devel] [PATCH] fix wrong output with 'info chardev' for tcp socket.
On Thu, Feb 21, 2013 at 04:28:27PM -0600, mdroth wrote: On Fri, Feb 22, 2013 at 12:29:44AM +0400, Michael Tokarev wrote: 22.02.2013 00:20, Serge E. Hallyn wrote: The snprintf format isn't taking into account the new 'left' and 'right' variables (for ipv6 []) when placing the ':', which should go immediately before the port. This fixes actual isse (also found by Serge), where `info chardev' prints `tcp:127.0.0.1,server,nowait' for a monitor running on port . This is definitely a stable material (CCed). Fix made it upstream through a separate patch: ec9f828341cb5e9cc3ad0bdbbd6989884daf823a Reviewed-by: Michael Tokarev m...@tls.msk.ru Reviewed-by: Michael Roth mdr...@linux.vnet.ibm.com Thanks! /mjt Signed-off-by: Serge Hallyn serge.hal...@ubuntu.com --- qemu-char.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu-char.c b/qemu-char.c index e4b0f53..3e152e1 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2482,7 +2482,7 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay, s-do_nodelay = do_nodelay; getnameinfo((struct sockaddr *) ss, ss_len, host, sizeof(host), serv, sizeof(serv), NI_NUMERICHOST | NI_NUMERICSERV); -snprintf(chr-filename, 256, %s:%s:%s%s%s%s, +snprintf(chr-filename, 256, %s:%s%s%s:%s%s, is_telnet ? telnet : tcp, left, host, right, serv, is_listen ? ,server : );
Re: [Qemu-devel] [PATCH] help: add docs for missing 'queues' option of tap
On Fri, Feb 22, 2013 at 10:57:52PM +0800, Jason Wang wrote:
Cc: Markus Armbruster arm...@redhat.com
Cc: qemu-sta...@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
Reviewed-by: Michael Roth mdr...@linux.vnet.ibm.com
Documentation is still missing upstream. Looking to pull this in for stable.
---
qapi-schema.json |2 ++
qemu-options.hx |3 ++-
2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index cd7ea25..b3844e6 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2526,6 +2526,8 @@
#
# @vhostforce: #optional vhost on for non-MSIX virtio guests
#
+# @queues: #optional number of queues to be created for multiqueue capable
tap
+#
# Since 1.2
##
{ 'type': 'NetdevTapOptions',
diff --git a/qemu-options.hx b/qemu-options.hx
index 2832d82..3928620 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1354,7 +1354,7 @@ DEF(net, HAS_ARG, QEMU_OPTION_net,
-net tap[,vlan=n][,name=str],ifname=name\n
connect the host TAP network interface to VLAN 'n'\n
#else
--net
tap[,vlan=n][,name=str][,fd=h][,fds=x:y:...:z][,ifname=name][,script=file][,downscript=dfile][,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off][,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off]\n
+-net
tap[,vlan=n][,name=str][,fd=h][,fds=x:y:...:z][,ifname=name][,script=file][,downscript=dfile][,helper=helper][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off][,vhostfd=h][,vhostfds=x:y:...:z][,vhostforce=on|off][,queues=n]\n
connect the host TAP network interface to VLAN 'n'\n
use network scripts 'file' (default=
DEFAULT_NETWORK_SCRIPT )\n
to configure it and 'dfile' (default=
DEFAULT_NETWORK_DOWN_SCRIPT )\n
@@ -1373,6 +1373,7 @@ DEF(net, HAS_ARG, QEMU_OPTION_net,
use vhostforce=on to force vhost on for non-MSIX virtio
guests\n
use 'vhostfd=h' to connect to an already opened vhost
net device\n
use 'vhostfds=x:y:...:z to connect to multiple already
opened vhost net devices\n
+use 'queues=n' to specify the number of queues to be
created for multiqueue TAP\n
-net bridge[,vlan=n][,name=str][,br=bridge][,helper=helper]\n
connects a host TAP network interface to a host bridge
device 'br'\n
(default= DEFAULT_BRIDGE_INTERFACE ) using the
program 'helper'\n
--
1.7.1
Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib
On Thu, Mar 28, 2013 at 03:55:52PM +0100, Stefan Hajnoczi wrote: On Thu, Mar 28, 2013 at 03:55:51PM +0800, Liu Ping Fan wrote: From: Liu Ping Fan pingf...@linux.vnet.ibm.com These series aim to make the whole network re-entrant, here only apply backend and frontend, and for the netcore, separated patches have been sent out. All of these will prepare us for moving towards making network layer mutlit-thread. Finally it would be omething like qemu -object io-thread,id=thread0 \ -device virtio-net,rx[0]=thread0,tx[0]=thread0 The brief of the whole aim and plan is documented on http://wiki.qemu.org/Features/network_reentrant The main issue is about GSource or AioContext, http://marc.info/?t=13631545332r=1w=3 And I sumary the main points: disadvantage for current AioContext 1st. need to define and expand interface for other fd events, while glib open this interface for user * 2nd. need to add support for IOCanReadHandler, while gsource provide prepare, check method to allow more flexible control 3rd. block layer's AioContext will block other AioContexts on the same thread. 4th. need more document disadvantage for glib 1st. if more than one fds on the same GSource, need re-implement something like aio_set_file_handler Since I have successed to port frontend on glib, there is no obstale to use glib. v1-v2: 1.NetClientState can associate with up to 2 GSource, for virtio net, one for tx, one for rx, so vq can run on different threads. 2.make network front-end onto glib, currently virtio net dataplane Liu Ping Fan (4): net: port tap onto glib net: resolve race of tap backend and its peer net: port hub onto glib net: port virtio net onto glib hw/qdev-properties-system.c |1 + hw/virtio-net.c | 165 +++ hw/virtio.c |6 ++ hw/virtio.h |2 + include/net/net.h | 27 +++ include/net/queue.h | 14 net/hub.c | 34 - net/net.c | 97 + net/queue.c |4 +- net/tap.c | 62 +--- 10 files changed, 397 insertions(+), 15 deletions(-) It seems the AioContext vs glib issue hasn't been settled yet. My take is that glib is preferrable *if* we don't need to write too many helpers/wrappers on top (then we're basically back to rolling our own, AioContext). I was surprised by the amount of code required to listen on a file descriptor. Are you sure there isn't a glib way of doing this that avoids rolling our own GSource? GIOChannel provides a pre-baked GSource you can pass around, but this might add more confusion to the mix when you consider things like Slirp which will likely require a custom GSource. It also assumes a different signature than GSourceFunc for the callback which further adds to the confusion. Keeping the interfaces centered around normal GSources I think would help to avoid the proliferation of more event-handling registration mechanisms in the future, and make conversions easier if we do need to change things. A generic GSource for handling FDs that we can re-use for basic use cases would help curtail some of the boilerplate later for common fd handlers. Probably doesn't make sense to generalize until we reach a decision on glib vs. aiocontext though. In the next series, please drop the hub re-entrancy stuff and virtio-net data plane. Instead just focus on systematically moving existing net clients onto the event loop (net/*.c and NICs). The only controversial issue there is AioContext vs glib, and once that's settled we can merge the patches. Please avoid layering violations - for example a comment about virtio-net in net.h, a comment about vhost in tap, or putting net_source_funcs in net.h. I think converting all existing net clients will help make the code changes appropriate and eliminate these kinds of hacks which are because you're focussing just on virtio, tap, and hub here. Stefan
