Re: [Qemu-devel] [PATCH 1/2] QMP: Introduce commands doc
On Thu, 13 May 2010 19:23:11 +0300 Avi Kivity a...@redhat.com wrote: On 05/13/2010 06:01 PM, Daniel P. Berrange wrote: Yes, we do and it's used by libvirt iirc. This command has been in QEMU for quite a long time now (0.9.x IIRC). It wasn't in QMP until 0.12. We shouldn't have put it there in that form. Your main concern is stability? QMP in 0.12 and current are in preview mode, ie. no stability contract until 0.13. We broke it already with a few changes (like capabilities support), so anyone using QMP in 0.12 will have to update, although I don't think QMP in 0.12 is that useful. Having said that, it's been stable for several weeks now and incompatible changes have to be coordinated with libvirt. So, if you think this is worth the trouble someone could do it. PS: I remember Anthony saying that he'd put somewhere that QMP is a preview version in 0.12. But I did not find anything, will submit a patch for stable.
[Qemu-devel] Re: [PATCH] Fix docs for block stats monitor command
On Fri, 14 May 2010 10:15:32 +0200 Kevin Wolf kw...@redhat.com wrote: Am 13.05.2010 12:30, schrieb Daniel P. Berrange: The 'parent' field in the 'query-blockstats' monitor command is part of the top level block device QDict, not part of the 2nd level 'stats' QDict. * block.c: Fix docs for 'parent' field in block stats monitor command output Signed-off-by: Daniel P. Berrange berra...@redhat.com Oops, you're right of course. Luiz, there was this patch that moved all the documentation to a different place and will conflict with this one. Should we fix the documentation on top of your patch or are you going to rebase? I'm going to rebase, have lots of stuff to fix in the doc.
Re: [Qemu-devel] [RFC] New Monitor command: 'info netdevices'
On Fri, 14 May 2010 09:38:58 -0300 Miguel Di Ciurcio Filho miguel.fi...@gmail.com wrote: On Fri, May 14, 2010 at 6:46 AM, Markus Armbruster arm...@redhat.com wrote: There's also 3. Convert it anyway. Clean up the mess. Change the output. I agree. It seams to me that no one is concerned with any Monitor output change with this particular command. Plus, no one have shown any concerns about the problems I've found in net/socket.c reported previously either. Sending patches increases the chances someone will look into it.
Re: [Qemu-devel] [PATCH 1/2] QMP: Introduce commands doc
On Fri, 14 May 2010 10:39:29 +0200
Markus Armbruster arm...@redhat.com wrote:
Luiz Capitulino lcapitul...@redhat.com writes:
On Thu, 13 May 2010 16:48:13 +0300
Avi Kivity a...@redhat.com wrote:
On 05/05/2010 10:11 PM, Luiz Capitulino wrote:
One of the most important missing feature in QMP today is its
supported commands documentation.
The plan is to make it part of self-description support, however
self-description is a big task we have been postponing for a
long time now and still don't know when it's going to be done.
In order not to compromise QMP adoption and make users' life easier,
this commit adds a simple text documentation which fully describes
all QMP supported commands.
This is not ideal for a number of reasons (harder to maintain,
text-only, etc) but does improve the current situation.
[...]
+pmemsave
+
+
+Save to disk physical memory dump starting at 'val' of size 'size'.
+
+Arguments:
+
+- val: the starting address (json-int)
Why val instead of address or physical-address?
Mea culpa, for this and most inconsistencies you find in the protocol.
The reason is that I did a lot of conversions in a hurry, so that
we could get libvirt working under QMP as soon as possible.
Unfortunately, some bad names and some bad behaviors from the user
Monitor leaked into QMP. Lesson learned, although sometimes it's very
difficult to define what a name/behavior should be in QMP.
Once we declare QMP stable, we're stuck with bad names forever. So
better fix them before we stabilize. Best to fix them all in one go,
and prepare the (hopefully straightforward) libvirt patch to go with it.
I think we have three kinds of things to be fixed. If we do it right
they will cause breakage to libvirt, but sometimes it's possible to
fix them in a compatible way:
1) Simple renames
2) Bad user Monitor commands exposed in QMP
3) Bad/incomplete design decisions
Doing 1) is easy, you have been fixing 2) for some commands, although
I'm not sure what we should do for do_change().
Now, 3) concerns me more. We have the QError stuff and events. What
concerns me is that we're likely going to work on this in hurry again..
The problem with events is that today, they look like this:
{ event: VNC_CONNECTED,
data: {
server: { auth: sasl, family: ipv4,
service: 5901, host: 0.0.0.0 },
client: { family: ipv4, service: 58425,
host: 127.0.0.1 } },
timestamp: { seconds: 1262976601, microseconds: 975795 } }
Which means that a subsystem/driver/whatever which needs to report
multiple 'actions', will have to do so through multiple events. For
example, vnc has: VNC_CONNECTED, VNC_DISCONNECTED and VNC_INITIALIZED.
A simple solution would be to add a 'subsystem' member, like:
{ event: CONNECTED, subsystem: vnc }
But this has a number of small problems too, like a specific driver
is not a subsystem and what's the subsystem for events like SHUTDOWN?
Anthony has suggested integrating with qdev, which is something I
think we should do, but alone it doesn't fix the problem explained above.
We had this discussion when the vnc events were introduced, but we
decided to wait for other events to think better about this. Events
have grown a little already and we have spice and migration in the queue..
Re: [Qemu-devel] [PATCH 1/2] QMP: Introduce commands doc
On Fri, 14 May 2010 19:03:36 +0200 Markus Armbruster arm...@redhat.com wrote: What about PCI domains? Good point. Better to provide for them neatly now, instead of kludging them in later. When I did this conversion I asked Micheal for help with that and he said QEMU doesn't support PCI domains.
[Qemu-devel] Re: [PATCH 1/2] QMP: Introduce commands doc
On Fri, 14 May 2010 19:08:07 +0200 Jan Kiszka jan.kis...@siemens.com wrote: Avi Kivity wrote: On 05/14/2010 08:01 PM, Avi Kivity wrote: On 05/14/2010 07:52 PM, Jan Kiszka wrote: In order not to compromise QMP adoption and make users' life easier, this commit adds a simple text documentation which fully describes all QMP supported commands. This is not ideal for a number of reasons (harder to maintain, text-only, etc) but does improve the current situation. Even if it's temporary - maintaining it in a separate file looks rather unhandy. Can't we generate the per-command documentation snippets also from qemu-monitor.hx and merge them with a header/footer into some text file? That .hx file is the one anyone adding/converting commands has to touch anyway. If we do, then the generated documentation should be included in the patch changelog for review. I mean, a patch introducing or modifying a monitor command. The snippets should be readable by themselves. I'm only proposing to keep them in the central file, at the same location where the others are. There is no difference compared to existing monitor commands, we just add the third documentation snippet, this time for QMP. It's not only the snippets. We add a json type for each parameter, a more descriptive info and notes. Only QMP commands should be shown this way. I'm sure there's a way to hack the qemu's doc script to do all this, but the right solution is to move _everything_ to json and generate good, well formatted documentation from there (along with self-description). Also, adapting things will take time and this will delay even more this doc to be merged, which is what I'm trying to avoid.
[Qemu-devel] Re: [PATCH 1/2] QMP: Introduce commands doc
On Sat, 15 May 2010 10:42:44 +0200 Jan Kiszka jan.kis...@web.de wrote: Luiz Capitulino wrote: On Fri, 14 May 2010 19:08:07 +0200 Jan Kiszka jan.kis...@siemens.com wrote: Avi Kivity wrote: On 05/14/2010 08:01 PM, Avi Kivity wrote: On 05/14/2010 07:52 PM, Jan Kiszka wrote: In order not to compromise QMP adoption and make users' life easier, this commit adds a simple text documentation which fully describes all QMP supported commands. This is not ideal for a number of reasons (harder to maintain, text-only, etc) but does improve the current situation. Even if it's temporary - maintaining it in a separate file looks rather unhandy. Can't we generate the per-command documentation snippets also from qemu-monitor.hx and merge them with a header/footer into some text file? That .hx file is the one anyone adding/converting commands has to touch anyway. If we do, then the generated documentation should be included in the patch changelog for review. I mean, a patch introducing or modifying a monitor command. The snippets should be readable by themselves. I'm only proposing to keep them in the central file, at the same location where the others are. There is no difference compared to existing monitor commands, we just add the third documentation snippet, this time for QMP. It's not only the snippets. We add a json type for each parameter, a more descriptive info and notes. Only QMP commands should be shown this way. Whatever its semantic is, technically it's a text block of a few lines that has to be added somewhere. I'm sure there's a way to hack the qemu's doc script to do all this, but the right solution is to move _everything_ to json and generate good, well formatted documentation from there (along with self-description). I'm not talking about The Right solution, I'm talking about a more handy temporary setup. When do you plan to refactor the command documentation that way? And how many commands will be touched in the meantime? It's something we would like to do ASAP, but there are a number of things we'll have to do first: bug fixes and some commands libvirt wants to use. Also, adapting things will take time and this will delay even more this doc to be merged, which is what I'm trying to avoid. I can provide you the patch for hxtool and Makefile (a few lines), and I'm willing to split up the existing doc, just drop me a note. So nothing needs to be delayed any further. I'd love that.
Re: [Qemu-devel] [PATCH 1/2] QMP: Introduce commands doc
On Tue, 18 May 2010 11:51:26 +0200 Markus Armbruster arm...@redhat.com wrote: Anthony Liguori anth...@codemonkey.ws writes: On 05/15/2010 01:19 AM, Avi Kivity wrote: On 05/15/2010 01:54 AM, Luiz Capitulino wrote: On Fri, 14 May 2010 19:03:36 +0200 Markus Armbrusterarm...@redhat.com wrote: What about PCI domains? Good point. Better to provide for them neatly now, instead of kludging them in later. When I did this conversion I asked Micheal for help with that and he said QEMU doesn't support PCI domains. That's very different from will never support pci domains. The protocol must be forward looking, or we will need endless fixes for it. But we can always add a domain property to extend the address (with a default domain of 0). Why not add it right away? The plan is to merge the doc and work on all changes suggested by Avi (always coordinating with libvirt, of course). Although query-pci is not used there. Note that if we'd decide to adopt Avi's suggestion to make this a nested object (list of buses, each containing a list of slots, each containing a list of functions), then we can't easily add domains later, because that would insert a level of nesting near the top. Right.
Re: [Qemu-devel] Re: [PATCH 1/2] QMP: Introduce commands doc
On Tue, 18 May 2010 13:21:36 +0200 Markus Armbruster arm...@redhat.com wrote: Jan Kiszka jan.kis...@web.de writes: [...] nothing needs to be delayed any further. Well, it's being delayed :) Let's commit the sucker as is. We can still move it into qemu-monitor.hx afterwards. Commits are cheap, waiting for the perfect patch isn't. Jan has already adapted qemu-monitor.hx and moved the entire doc into there. I didn't take a look at the patches yet, though. Will do it today and work on the (_doc_) proposed changes in the process.
[Qemu-devel] Re: [RFC] 0.13.0 Release plan
On Tue, 18 May 2010 09:32:32 -0500 Anthony Liguori aligu...@linux.vnet.ibm.com wrote: Hi, Here's my current thinking for the 0.13.0 release. Since there's a lot of activity going on with QMP, I'd like to move the release out to July 1st. Here's what I'd like to do between now and then: - Do a detailed review of the QMP specification by sending out portions of the spec to the mailing list and waiting for at least 3 acks (Stefan/Anthony) Agreed. Actually I was wondering if this shouldn't be QMP's development plan, ie. any QMP change which is protocol visible, must: 1. Add the proper entry in qmp-commands.txt 2. Get three ACKs on the list Of course that we need qmp-commands.txt merged. Jan has just finished incorporating its contents in qemu-monitor.hx, now qmp-commands.txt is generated from there. I will fix the doc's reported problems in his series and submit it. - Redesign QError to be more consistent (Markus/Luiz) Yes, we have all issues pointed out by Avi too. I plan to look at them as soon as the document is merged. However, the ones used by libvirt should be considered with extra care. - Host a bug day on June 1st (more details in later note) Excellent idea, suggest asking in that thread if anyone volunteers to be the issue tracker's 'maintainer' (main job is bug triage). - Freeze stable-0.13 on June 21st and release 0.13.0-rc - Accept only bug fixes for stable-0.13 - 0.13.0-rc2 release on June 28th - 0.13.0 release on July 1st ACK Any feedback and/or suggestions would be appreciated. Regards, Anthony Liguori
Re: [Qemu-devel] Re: KVM call agenda for May 18
On Tue, 18 May 2010 15:55:41 +0100
Daniel P. Berrange berra...@redhat.com wrote:
On Tue, May 18, 2010 at 09:34:06AM -0500, Anthony Liguori wrote:
On 05/18/2010 09:09 AM, Daniel P. Berrange wrote:
On Tue, May 18, 2010 at 08:53:19AM -0500, Anthony Liguori wrote:
On 05/17/2010 10:23 PM, Chris Wright wrote:
Please send in any agenda items you are interested in covering.
If we have a lack of agenda items I'll cancel the week's call.
- Slipping 0.13 release out to July 1st.
What is the plan wrt QMP and 0.13 ? Is the intention to have 100%[1] of the
existing monitor commands converted to QMP?
No. I don't think our goal is to ever fully convert monitor commands to
QMP. Some commands simply don't make sense as QMP commands (like x and xp).
We're a really long way from a complete conversion even ignoring
commands which don't make sense in QMP. The current state almost
covers the commands libvirt currently uses, but there's much more
beyond that.
As far as I understood it, the plan for the first QMP release has always
been to only convert the subset of commands relevant/used by libvirt.
We've been trying to figure out this set for a long time.
Is there a set of commands that you think need to be converted that
currently aren't?
Notable outstanding commands that libvirt has a non-negligable
chance of wanting to use in the not too distant future
- blockdev_add/del (to replace drive_add/del)
Markus is working on this.
- commit/delvm/loadvm/savevm
I did a first try, but errors in those handlers are a mess and didn't
map well to QMP. I think QError improvements are needed to get this done.
- screendump
- set_link
Both already converted.
- mouse_{move,button,set}
- sendkey
- acl_{add,remove,policy,reset,show}
- boot_set
- watchdog_action
Not converted and I'm not sure how hard they are.
The full list of unconverted commands though is much long:
[...]
I don't think we can claim all those are irrelevant for QMP.
So are we still targetting complete conversion of relevant commands
for 0.13, or is it just going to be a stepping stone where declare
QMP stable, but known to be incomplete for coverage of commands ?
The first thing to do is to agree on what a 'complete coverage' would be,
what we have been trying to do since January is to provide a complete set
for libvirt, our unique well known client so far.
Apart from the 'outstanding' set above, can you elaborate on how
QMP on 0.13 would not satisfy libvirt needs?
Re: [Qemu-devel] Re: KVM call agenda for May 18
On Tue, 18 May 2010 17:16:54 +0100 Daniel P. Berrange berra...@redhat.com wrote: On Tue, May 18, 2010 at 01:00:40PM -0300, Luiz Capitulino wrote: On Tue, 18 May 2010 15:55:41 +0100 Daniel P. Berrange berra...@redhat.com wrote: On Tue, May 18, 2010 at 09:34:06AM -0500, Anthony Liguori wrote: On 05/18/2010 09:09 AM, Daniel P. Berrange wrote: On Tue, May 18, 2010 at 08:53:19AM -0500, Anthony Liguori wrote: On 05/17/2010 10:23 PM, Chris Wright wrote: Please send in any agenda items you are interested in covering. If we have a lack of agenda items I'll cancel the week's call. - Slipping 0.13 release out to July 1st. What is the plan wrt QMP and 0.13 ? Is the intention to have 100%[1] of the existing monitor commands converted to QMP? No. I don't think our goal is to ever fully convert monitor commands to QMP. Some commands simply don't make sense as QMP commands (like x and xp). We're a really long way from a complete conversion even ignoring commands which don't make sense in QMP. The current state almost covers the commands libvirt currently uses, but there's much more beyond that. As far as I understood it, the plan for the first QMP release has always been to only convert the subset of commands relevant/used by libvirt. Well we've evolved the plan several times since QMP started the set of commands used by libvirt has evolved too! So I just want to define exactly what we're proposing to support for 0.13 release. The current set of libvirt used commands plus the must haves ones, this is the maximum we can be committed with for the next release. If time allows, I will pick more from the outstanding list. I don't think we can claim all those are irrelevant for QMP. So are we still targetting complete conversion of relevant commands for 0.13, or is it just going to be a stepping stone where declare QMP stable, but known to be incomplete for coverage of commands ? The first thing to do is to agree on what a 'complete coverage' would be, what we have been trying to do since January is to provide a complete set for libvirt, our unique well known client so far. Apart from the 'outstanding' set above, can you elaborate on how QMP on 0.13 would not satisfy libvirt needs? The must haves are blockdev_add, and the commit/delvm/loadvm/savevm stuff, since they're already in use. The problem I fear is that we're aiming for a moving target here. eg, by the time QEMU 0.13 comes out libvirt may have received patches using yet more QEMU monitor commands. So the list I mention above is my best guesstimate at the commands that could appear in libvirt in the not too distant future. The more of those we can support the better so that we avoid geting into a case where QEMU 0.13 is released, but a yet newer libvirt wants extra QMP commands that weren't included in 0.13. This problem isn't 0.13 specific as I expect new Monitor commands to be added in qemu at every release. It's true that QMP makes this more evident due its limited set, still we have to deal with the fact that the command set is always going to be extended. Here's what I think we should do: 1. Clients should check for the availability of commands by issuing query-commands. Eg. A) Issue query-commands at startup and cache its output B) Before issuing any command, check whether it's supported C) Issue an error message if the command is not supported 2. Both projects should work closely, so that features are in sync at every release 3. We could create an official 'priority list' in qemu's tree, with a listing of commands to be converted for the next release and clients should work closely by suggesting changes to this list
[Qemu-devel] [PATCH 1/3] monitor: Reorder info documentation
From: Jan Kiszka jan.kis...@siemens.com
Push the doc fragments for the info command to the end of
qemu-monitor.hx. This helps to establish a proper layout in the upcoming
QMP documentation.
Signed-off-by: Jan Kiszka jan.kis...@siemens.com
---
qemu-monitor.hx | 166 --
1 files changed, 86 insertions(+), 80 deletions(-)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index a8f194c..3709708 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -38,86 +38,6 @@ Commit changes to the disk images (if -snapshot is used) or
backing files.
ETEXI
{
-.name = info,
-.args_type = item:s?,
-.params = [subcommand],
-.help = show various information about the system state,
-.user_print = monitor_user_noop,
-.mhandler.cmd_new = do_info,
-},
-
-STEXI
-...@item info @var{subcommand}
-...@findex info
-Show various information about the system state.
-
-...@table @option
-...@item info version
-show the version of QEMU
-...@item info network
-show the various VLANs and the associated devices
-...@item info chardev
-show the character devices
-...@item info block
-show the block devices
-...@item info block
-show block device statistics
-...@item info registers
-show the cpu registers
-...@item info cpus
-show infos for each CPU
-...@item info history
-show the command line history
-...@item info irq
-show the interrupts statistics (if available)
-...@item info pic
-show i8259 (PIC) state
-...@item info pci
-show emulated PCI device info
-...@item info tlb
-show virtual to physical memory mappings (i386 only)
-...@item info mem
-show the active virtual memory mappings (i386 only)
-...@item info hpet
-show state of HPET (i386 only)
-...@item info kvm
-show KVM information
-...@item info usb
-show USB devices plugged on the virtual USB hub
-...@item info usbhost
-show all USB host devices
-...@item info profile
-show profiling information
-...@item info capture
-show information about active capturing
-...@item info snapshots
-show list of VM snapshots
-...@item info status
-show the current VM status (running|paused)
-...@item info pcmcia
-show guest PCMCIA status
-...@item info mice
-show which guest mouse is receiving events
-...@item info vnc
-show the vnc server status
-...@item info name
-show the current VM name
-...@item info uuid
-show the current VM UUID
-...@item info cpustats
-show CPU statistics
-...@item info usernet
-show user network stack connection states
-...@item info migrate
-show migration status
-...@item info balloon
-show balloon information
-...@item info qtree
-show device tree
-...@end table
-ETEXI
-
-{
.name = q|quit,
.args_type = ,
.params = ,
@@ -1182,6 +1102,92 @@ STEXI
Enable the specified QMP capabilities
ETEXI
+
+HXCOMM Keep the 'info' command at the end!
+HXCOMM This is required for the QMP documentation layout.
+
+{
+.name = info,
+.args_type = item:s?,
+.params = [subcommand],
+.help = show various information about the system state,
+.user_print = monitor_user_noop,
+.mhandler.cmd_new = do_info,
+},
+
+STEXI
+...@item info @var{subcommand}
+...@findex info
+Show various information about the system state.
+
+...@table @option
+...@item info version
+show the version of QEMU
+...@item info network
+show the various VLANs and the associated devices
+...@item info chardev
+show the character devices
+...@item info block
+show the block devices
+...@item info block
+show block device statistics
+...@item info registers
+show the cpu registers
+...@item info cpus
+show infos for each CPU
+...@item info history
+show the command line history
+...@item info irq
+show the interrupts statistics (if available)
+...@item info pic
+show i8259 (PIC) state
+...@item info pci
+show emulated PCI device info
+...@item info tlb
+show virtual to physical memory mappings (i386 only)
+...@item info mem
+show the active virtual memory mappings (i386 only)
+...@item info hpet
+show state of HPET (i386 only)
+...@item info kvm
+show KVM information
+...@item info usb
+show USB devices plugged on the virtual USB hub
+...@item info usbhost
+show all USB host devices
+...@item info profile
+show profiling information
+...@item info capture
+show information about active capturing
+...@item info snapshots
+show list of VM snapshots
+...@item info status
+show the current VM status (running|paused)
+...@item info pcmcia
+show guest PCMCIA status
+...@item info mice
+show which guest mouse is receiving events
+...@item info vnc
+show the vnc server status
+...@item info name
+show the current VM name
+...@item info uuid
+show the current VM UUID
+...@item info cpustats
+show CPU statistics
+...@item info usernet
+show user network stack connection states
+...@item info migrate
+show migration status
+...@item info balloon
+show balloon information
+...@item info qtree
[Qemu-devel] [PATCH v3 0/3]: QMP: Commands doc
This new version moves the documentation to qemu-monitor.hx and now QMP/qmp-commands.txt is generated from there (thanks Jan!). I hope I've addressed all review comments in this version and now it should describe reality. Next step is to fix glitches (after this series is merged, of course). changelog - v2 - v3 - Rebased - Addressed review comments - Move contents to qemu-monitor.hx (Jan) v1 - v2 - Rebased - Addressed Markus's comments - Changed indentation style - Minor fixes and clarifications
[Qemu-devel] [PATCH 2/3] QMP: Introduce commands documentation
From: Jan Kiszka jan.kis...@siemens.com
One of the most important missing feature in QMP today is its
supported commands documentation.
The plan is to make it part of self-description support, however
self-description is a big task we have been postponing for a
long time now and still don't know when it's going to be done.
In order not to compromise QMP adoption and make users' life easier,
this commit adds a simple text documentation which fully describes
all QMP supported commands.
This is not ideal for a number of reasons (harder to maintain,
text-only, etc) but does improve the current situation. To avoid at
least divering from the user monitor help and texi snippets, QMP bits
are also maintained inside qemu-monitor.hx, and hxtool is extended to
generate a single text file from them.
Signed-off-by: Jan Kiszka jan.kis...@siemens.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
Makefile|5 +-
QMP/README |5 +-
configure |4 +
hxtool | 44 ++-
qemu-monitor.hx | 1314 ++-
5 files changed, 1367 insertions(+), 5 deletions(-)
diff --git a/Makefile b/Makefile
index 306a1a4..110698e 100644
--- a/Makefile
+++ b/Makefile
@@ -29,7 +29,7 @@ $(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)
LIBS+=-lz $(LIBS_TOOLS)
ifdef BUILD_DOCS
-DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8
+DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8
QMP/qmp-commands.txt
else
DOCS=
endif
@@ -259,6 +259,9 @@ qemu-options.texi: $(SRC_PATH)/qemu-options.hx
qemu-monitor.texi: $(SRC_PATH)/qemu-monitor.hx
$(call quiet-command,sh $(SRC_PATH)/hxtool -t $ $@, GEN $@)
+QMP/qmp-commands.txt: $(SRC_PATH)/qemu-monitor.hx
+ $(call quiet-command,sh $(SRC_PATH)/hxtool -q $ $@, GEN $@)
+
qemu-img-cmds.texi: $(SRC_PATH)/qemu-img-cmds.hx
$(call quiet-command,sh $(SRC_PATH)/hxtool -t $ $@, GEN $@)
diff --git a/QMP/README b/QMP/README
index 9334c25..35a80c7 100644
--- a/QMP/README
+++ b/QMP/README
@@ -15,8 +15,9 @@ QMP is JSON[1] based and has the following features:
For more information, please, refer to the following files:
-o qmp-spec.txtQEMU Monitor Protocol current specification
-o qmp-events.txt List of available asynchronous events
+o qmp-spec.txt QEMU Monitor Protocol current specification
+o qmp-commands.txt QMP supported commands
+o qmp-events.txtList of available asynchronous events
There are also two simple Python scripts available:
diff --git a/configure b/configure
index 36d028f..6738e0b 100755
--- a/configure
+++ b/configure
@@ -2807,3 +2807,7 @@ ln -s $source_path/Makefile.user $d/Makefile
if test $static = no -a $user_pie = yes ; then
echo QEMU_CFLAGS+=-fpie $d/config.mak
fi
+
+if test $docs = yes ; then
+ mkdir -p QMP
+fi
diff --git a/hxtool b/hxtool
index 8f65532..d499dc0 100644
--- a/hxtool
+++ b/hxtool
@@ -7,7 +7,7 @@ hxtoh()
case $str in
HXCOMM*)
;;
-STEXI*|ETEXI*) flag=$(($flag^1))
+STEXI*|ETEXI*|SQMP*|EQMP*) flag=$(($flag^1))
;;
*)
test $flag -eq 1 printf %s\n $str
@@ -38,6 +38,12 @@ hxtotexi()
fi
flag=0
;;
+SQMP*|EQMP*)
+if test $flag -eq 1 ; then
+echo line $line: syntax error: expected ETEXI, found $str 2
+exit 1
+fi
+;;
DEFHEADING*)
echo $(expr $str : DEFHEADING(\(.*\)))
;;
@@ -49,9 +55,45 @@ hxtotexi()
done
}
+hxtoqmp()
+{
+IFS=
+flag=0
+while read -r str; do
+case $str in
+HXCOMM*)
+;;
+SQMP*)
+if test $flag -eq 1 ; then
+echo line $line: syntax error: expected EQMP, found $str 2
+exit 1
+fi
+flag=1
+;;
+EQMP*)
+if test $flag -ne 1 ; then
+echo line $line: syntax error: expected SQMP, found $str 2
+exit 1
+fi
+flag=0
+;;
+STEXI*|ETEXI*)
+if test $flag -eq 1 ; then
+echo line $line: syntax error: expected EQMP, found $str 2
+exit 1
+fi
+;;
+*)
+test $flag -eq 1 echo $str
+;;
+esac
+done
+}
+
case $1 in
-h) hxtoh ;;
-t) hxtotexi ;;
+-q) hxtoqmp ;;
*) exit 1 ;;
esac
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 3709708..c8f1789 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1,10 +1,48 @@
HXCOMM Use DEFHEADING() to define headings in both help text and texi
HXCOMM Text between STEXI and ETEXI are copied to texi version and
HXCOMM discarded from C version
+HXCOMM Text between SQMP and EQMP is copied to the QMP documention file and
+HXCOMM does not show up in the other formats
[Qemu-devel] [PATCH 3/3] Monitor: Drop QMP documentation from code
Previous commit added the QMP/qmp-commands.txt file, which is a
copy of this information.
While it's good to keep it near code, maintaining two copies of
the same information is too hard and has little benefit as we
don't expect client writers to consult the code to find how to
use a QMP command.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
block.c | 69
hw/pci.c| 61 ---
hw/qdev.c | 13 ---
input.c | 18 --
migration.c | 38 --
monitor.c | 102 ---
net.c | 22 -
qemu-char.c | 16 -
vnc.c | 29 -
9 files changed, 0 insertions(+), 368 deletions(-)
diff --git a/block.c b/block.c
index bfe46e3..73a8182 100644
--- a/block.c
+++ b/block.c
@@ -1429,33 +1429,6 @@ void bdrv_info_print(Monitor *mon, const QObject *data)
qlist_iter(qobject_to_qlist(data), bdrv_print_dict, mon);
}
-/**
- * bdrv_info(): Block devices information
- *
- * Each block device information is stored in a QDict and the
- * returned QObject is a QList of all devices.
- *
- * The QDict contains the following:
- *
- * - device: device name
- * - type: device type
- * - removable: true if the device is removable, false otherwise
- * - locked: true if the device is locked, false otherwise
- * - inserted: only present if the device is inserted, it is a QDict
- *containing the following:
- * - file: device file name
- * - ro: true if read-only, false otherwise
- * - drv: driver format name
- * - backing_file: backing file name if one is used
- * - encrypted: true if encrypted, false otherwise
- *
- * Example:
- *
- * [ { device: ide0-hd0, type: hd, removable: false, locked: false,
- * inserted: { file: /tmp/foobar, ro: false, drv: qcow2 } },
- * { device: floppy0, type: floppy, removable: true,
- * locked: false } ]
- */
void bdrv_info(Monitor *mon, QObject **ret_data)
{
QList *bs_list;
@@ -1561,48 +1534,6 @@ static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
return res;
}
-/**
- * bdrv_info_stats(): show block device statistics
- *
- * Each device statistic information is stored in a QDict and
- * the returned QObject is a QList of all devices.
- *
- * The QDict contains the following:
- *
- * - device: device name
- * - stats: A QDict with the statistics information, it contains:
- * - rd_bytes: bytes read
- * - wr_bytes: bytes written
- * - rd_operations: read operations
- * - wr_operations: write operations
- * - wr_highest_offset: Highest offset of a sector written since the
- * BlockDriverState has been opened
- * - parent: A QDict recursively holding the statistics of the underlying
- *protocol (e.g. the host file for a qcow2 image). If there is no
- *underlying protocol, this field is omitted.
- *
- * Example:
- *
- * [ { device: ide0-hd0,
- * stats: { rd_bytes: 512,
- * wr_bytes: 0,
- * rd_operations: 1,
- * wr_operations: 0,
- * wr_highest_offset: 0 },
- * parent: {
- * stats: { rd_bytes: 1024,
- * wr_bytes: 0,
- * rd_operations: 2,
- * wr_operations: 0,
- * wr_highest_offset: 0,
- * } } },
- * { device: ide1-cd0,
- * stats: { rd_bytes: 0,
- * wr_bytes: 0,
- * rd_operations: 0,
- * wr_operations: 0,
- * wr_highest_offset: 0 } },
- */
void bdrv_info_stats(Monitor *mon, QObject **ret_data)
{
QObject *obj;
diff --git a/hw/pci.c b/hw/pci.c
index 8d84651..4636193 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1362,67 +1362,6 @@ static QObject *pci_get_bus_dict(PCIBus *bus, int
bus_num)
return NULL;
}
-/**
- * do_pci_info(): PCI buses and devices information
- *
- * The returned QObject is a QList of all buses. Each bus is
- * represented by a QDict, which has a key with a QList of all
- * PCI devices attached to it. Each device is represented by
- * a QDict.
- *
- * The bus QDict contains the following:
- *
- * - bus: bus number
- * - devices: a QList of QDicts, each QDict represents a PCI
- * device
- *
- * The PCI device QDict contains the following:
- *
- * - bus: identical to the parent's bus number
- * - slot: slot number
- * - function: function number
- * - class_info: a QDict containing:
- * - desc: device class description (optional)
- * - class: device class number
- * - id: a QDict containing:
- * - device: device ID
- * - vendor: vendor ID
- * - irq: device's IRQ if assigned (optional)
- * - qdev_id: qdev id string
[Qemu-devel] Re: [PATCH 2/3] QMP: Introduce commands documentation
On Wed, 19 May 2010 11:15:16 +0200
Jan Kiszka jan.kis...@siemens.com wrote:
Luiz Capitulino wrote:
From: Jan Kiszka jan.kis...@siemens.com
One of the most important missing feature in QMP today is its
supported commands documentation.
The plan is to make it part of self-description support, however
self-description is a big task we have been postponing for a
long time now and still don't know when it's going to be done.
In order not to compromise QMP adoption and make users' life easier,
this commit adds a simple text documentation which fully describes
all QMP supported commands.
This is not ideal for a number of reasons (harder to maintain,
text-only, etc) but does improve the current situation. To avoid at
least divering from the user monitor help and texi snippets, QMP bits
are also maintained inside qemu-monitor.hx, and hxtool is extended to
generate a single text file from them.
Signed-off-by: Jan Kiszka jan.kis...@siemens.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
Makefile|5 +-
QMP/README |5 +-
configure |4 +
hxtool | 44 ++-
qemu-monitor.hx | 1314
++-
5 files changed, 1367 insertions(+), 5 deletions(-)
diff --git a/Makefile b/Makefile
index 306a1a4..110698e 100644
--- a/Makefile
+++ b/Makefile
@@ -29,7 +29,7 @@ $(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)
LIBS+=-lz $(LIBS_TOOLS)
ifdef BUILD_DOCS
-DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8
+DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8
QMP/qmp-commands.txt
else
DOCS=
endif
@@ -259,6 +259,9 @@ qemu-options.texi: $(SRC_PATH)/qemu-options.hx
qemu-monitor.texi: $(SRC_PATH)/qemu-monitor.hx
$(call quiet-command,sh $(SRC_PATH)/hxtool -t $ $@, GEN
$@)
+QMP/qmp-commands.txt: $(SRC_PATH)/qemu-monitor.hx
+ $(call quiet-command,sh $(SRC_PATH)/hxtool -q $ $@, GEN
$@)
+
qemu-img-cmds.texi: $(SRC_PATH)/qemu-img-cmds.hx
$(call quiet-command,sh $(SRC_PATH)/hxtool -t $ $@, GEN
$@)
diff --git a/QMP/README b/QMP/README
index 9334c25..35a80c7 100644
--- a/QMP/README
+++ b/QMP/README
@@ -15,8 +15,9 @@ QMP is JSON[1] based and has the following features:
For more information, please, refer to the following files:
-o qmp-spec.txtQEMU Monitor Protocol current specification
-o qmp-events.txt List of available asynchronous events
+o qmp-spec.txt QEMU Monitor Protocol current specification
+o qmp-commands.txt QMP supported commands
+o qmp-events.txtList of available asynchronous events
There are also two simple Python scripts available:
diff --git a/configure b/configure
index 36d028f..6738e0b 100755
--- a/configure
+++ b/configure
@@ -2807,3 +2807,7 @@ ln -s $source_path/Makefile.user $d/Makefile
if test $static = no -a $user_pie = yes ; then
echo QEMU_CFLAGS+=-fpie $d/config.mak
fi
+
+if test $docs = yes ; then
+ mkdir -p QMP
+fi
diff --git a/hxtool b/hxtool
index 8f65532..d499dc0 100644
--- a/hxtool
+++ b/hxtool
@@ -7,7 +7,7 @@ hxtoh()
case $str in
HXCOMM*)
;;
-STEXI*|ETEXI*) flag=$(($flag^1))
+STEXI*|ETEXI*|SQMP*|EQMP*) flag=$(($flag^1))
;;
*)
test $flag -eq 1 printf %s\n $str
@@ -38,6 +38,12 @@ hxtotexi()
fi
flag=0
;;
+SQMP*|EQMP*)
+if test $flag -eq 1 ; then
+echo line $line: syntax error: expected ETEXI, found
$str 2
+exit 1
+fi
+;;
DEFHEADING*)
echo $(expr $str : DEFHEADING(\(.*\)))
;;
@@ -49,9 +55,45 @@ hxtotexi()
done
}
+hxtoqmp()
+{
+IFS=
+flag=0
+while read -r str; do
+case $str in
+HXCOMM*)
+;;
+SQMP*)
+if test $flag -eq 1 ; then
+echo line $line: syntax error: expected EQMP, found $str
2
+exit 1
+fi
+flag=1
+;;
+EQMP*)
+if test $flag -ne 1 ; then
+echo line $line: syntax error: expected SQMP, found $str
2
+exit 1
+fi
+flag=0
+;;
+STEXI*|ETEXI*)
+if test $flag -eq 1 ; then
+echo line $line: syntax error: expected EQMP, found $str
2
+exit 1
+fi
+;;
+*)
+test $flag -eq 1 echo $str
+;;
+esac
+done
+}
+
case $1 in
-h) hxtoh ;;
-t) hxtotexi ;;
+-q) hxtoqmp ;;
*) exit 1 ;;
esac
Unfortunately
[Qemu-devel] Re: [PATCH 2/3] QMP: Introduce commands documentation
On Wed, 19 May 2010 15:30:43 +0200 Jan Kiszka jan.kis...@siemens.com wrote: Luiz Capitulino wrote: [...] I didn't submit the syntax checking bits on purpose, there's something failing there and I didn't want to check it now. You already did for QMP, just skipped the STEXI/ETEXI balancing checks. And if that reports an error for TEXI, better have a closer look. It worked fine with my tree, so something may have regressed. Here you go: GEN qemu-img-cmds.texi line 10: syntax error: expected ETEXI, found STEXI make: *** [qemu-img-cmds.texi] Error 1 make: *** Deleting file `qemu-img-cmds.texi' Indeed, looks like a bug in qemu-img-cmds.hx: STEXI @table @option STEXI
[Qemu-devel] Re: [PATCH 2/3] QMP: Introduce commands documentation
On Wed, 19 May 2010 15:50:56 +0200 Jan Kiszka jan.kis...@siemens.com wrote: Luiz Capitulino wrote: On Wed, 19 May 2010 15:30:43 +0200 Jan Kiszka jan.kis...@siemens.com wrote: Luiz Capitulino wrote: [...] I didn't submit the syntax checking bits on purpose, there's something failing there and I didn't want to check it now. You already did for QMP, just skipped the STEXI/ETEXI balancing checks. And if that reports an error for TEXI, better have a closer look. It worked fine with my tree, so something may have regressed. Here you go: GEN qemu-img-cmds.texi line 10: syntax error: expected ETEXI, found STEXI make: *** [qemu-img-cmds.texi] Error 1 make: *** Deleting file `qemu-img-cmds.texi' Indeed, looks like a bug in qemu-img-cmds.hx: STEXI @table @option STEXI Oh, then I also did no make clean all run with my patch applied that night... :] Still, good to know that it works. I can handle this if you don't want to. Please do, you could send both patches in the same series (ie. fix and feature), so that we don't break the build.
Re: [Qemu-devel] [RFC] Bug Day - June 1st, 2010
On Tue, 18 May 2010 17:38:27 -0500 Anthony Liguori aligu...@linux.vnet.ibm.com wrote: Hi, In an effort to improve the 0.13 release quality, I'd like to host a Bug Day on June 1st, 2010. I've setup a quick wiki page with some more info (http://wiki.qemu.org/BugDay/June2010). Tuesday is our call conf day and other people have reported that they have more confs during that day. Suggest Jun 2. Here's my basic thinking: - Anyone who can should try to spend some time either triaging bugs, updating bug status, or actually fixing bugs. And testing, Fedora has a number of test cases already written, but I guess that just a few are qemu specific: https://fedoraproject.org/wiki/Test_Day:2010-04-08_Virtualization We could link those and write our own, or at least list the major features to be tested.. Of course we could have a different day for testing too, but I think this is a way to get everyone busy. - We'll have a special IRC channel (#qemu-bugday) on OFTC. As many QEMU and KVM developers as possible should join this channel for that day to help assist people working on bugs. - We'll try to migrate as many confirmable bugs from the Source Forge tracker to Launchpad. Can't this be automated? If this is successful, we'll try to have regular bug days. Any suggestions on how to make the experience as fun and productive as possible are certainly appreciated! Lots of foods and a Monty Python session in the evening.
[Qemu-devel] [PATCH 3/7] Revert monitor: Convert do_pci_device_hot_remove() to QObject
From: Markus Armbruster arm...@redhat.com
We don't want pci_del in QMP. Use device_del instead.
This reverts commit 6848d827162fea039f2658414a4adb6164a4f9b0.
Conflicts:
hw/pci-hotplug.c
sysemu.h
Signed-off-by: Markus Armbruster arm...@redhat.com
---
hw/pci-hotplug.c |5 ++---
qemu-monitor.hx |3 +--
sysemu.h |3 +--
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 22a7ce4..37ac015 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -277,8 +277,7 @@ int pci_device_hot_remove(Monitor *mon, const char
*pci_addr)
return qdev_unplug(d-qdev);
}
-int do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,
- QObject **ret_data)
+void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict)
{
-return pci_device_hot_remove(mon, qdict_get_str(qdict, pci_addr));
+pci_device_hot_remove(mon, qdict_get_str(qdict, pci_addr));
}
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index fba4c3f..b6e3467 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -874,8 +874,7 @@ ETEXI
.args_type = pci_addr:s,
.params = [[domain:]bus:]slot,
.help = hot remove PCI device,
-.user_print = monitor_user_noop,
-.mhandler.cmd_new = do_pci_device_hot_remove,
+.mhandler.cmd = do_pci_device_hot_remove,
},
#endif
diff --git a/sysemu.h b/sysemu.h
index 47975b5..643c0c6 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -204,8 +204,7 @@ DriveInfo *add_init_drive(const char *opts);
void pci_device_hot_add(Monitor *mon, const QDict *qdict);
void drive_hot_add(Monitor *mon, const QDict *qdict);
int pci_device_hot_remove(Monitor *mon, const char *pci_addr);
-int do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,
- QObject **ret_data);
+void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);
/* serial ports */
--
1.7.1.86.g0e460
[Qemu-devel] [PATCH 2/7] Revert PCI: Convert pci_device_hot_add() to QObject
From: Markus Armbruster arm...@redhat.com
Short story: We don't want pci_add in QMP. Long story follows.
pci_add can do two things:
* Hot plug a PCI NIC. device_add is more general.
* Hot plug a PCI disk controller, and a drive connected to it.
The controller is either virtio-blk-pci (if=virtio) or lsi53c895a
(if=scsi). With the latter, the drive is optional. Use drive_add to
hotplug additional SCSI drives. Except drive_add is not available in
QMP.
device_add is more general for controllers and the guest part of
drives. I'm working on a more general alternative for the host part
of drives.
Why am I proposing to remove pci_add from QMP before its replacement is
ready? I want it out sooner rather than later, because it isn't fully
functional (errors and drive_add are missing), and we do not plan to
complete the job. In other words, it's not really usable over QMP now,
and it's not what we want for QMP anyway. Since we don't want it to be
used over QMP, we should take it out, not leave it around as a trap for
the uninitiated.
Dan Berrange confirmed that libvirt has no need for pci_add friends
over QMP.
This reverts commit 7a344f7ac7bb651d0556a933ed8060d3a9e5d949.
Conflicts:
hw/pci-hotplug.c
sysemu.h
Signed-off-by: Markus Armbruster arm...@redhat.com
---
hw/pci-hotplug.c | 46 +-
qemu-monitor.hx |3 +--
sysemu.h |3 +--
3 files changed, 7 insertions(+), 45 deletions(-)
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index cc45c50..22a7ce4 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -33,7 +33,6 @@
#include scsi.h
#include virtio-blk.h
#include qemu-config.h
-#include qemu-objects.h
#if defined(TARGET_I386)
static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
@@ -224,36 +223,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
return dev;
}
-void pci_device_hot_add_print(Monitor *mon, const QObject *data)
-{
-QDict *qdict;
-
-assert(qobject_type(data) == QTYPE_QDICT);
-qdict = qobject_to_qdict(data);
-
-monitor_printf(mon, OK domain %d, bus %d, slot %d, function %d\n,
- (int) qdict_get_int(qdict, domain),
- (int) qdict_get_int(qdict, bus),
- (int) qdict_get_int(qdict, slot),
- (int) qdict_get_int(qdict, function));
-
-}
-
-/**
- * pci_device_hot_add(): Hot add a PCI device
- *
- * Return a QDict with the following device information:
- *
- * - domain: domain number
- * - bus: bus number
- * - slot: slot number
- * - function: function number
- *
- * Example:
- *
- * { domain: 0, bus: 0, slot: 5, function: 0 }
- */
-int pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void pci_device_hot_add(Monitor *mon, const QDict *qdict)
{
PCIDevice *dev = NULL;
const char *pci_addr = qdict_get_str(qdict, pci_addr);
@@ -278,20 +248,14 @@ int pci_device_hot_add(Monitor *mon, const QDict *qdict,
QObject **ret_data)
dev = qemu_pci_hot_add_storage(mon, pci_addr, opts);
} else {
monitor_printf(mon, invalid type: %s\n, type);
-return -1;
}
if (dev) {
-*ret_data =
-qobject_from_jsonf({ 'domain': 0, 'bus': %d, 'slot': %d,
- 'function': %d }, pci_bus_num(dev-bus),
- PCI_SLOT(dev-devfn), PCI_FUNC(dev-devfn));
-} else {
+monitor_printf(mon, OK domain %d, bus %d, slot %d, function %d\n,
+ 0, pci_bus_num(dev-bus), PCI_SLOT(dev-devfn),
+ PCI_FUNC(dev-devfn));
+} else
monitor_printf(mon, failed to add %s\n, opts);
-return -1;
-}
-
-return 0;
}
#endif
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index a8f194c..fba4c3f 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -858,8 +858,7 @@ ETEXI
.args_type = pci_addr:s,type:s,opts:s?,
.params = auto|[[domain:]bus:]slot nic|storage
[[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]...,
.help = hot-add PCI device,
-.user_print = pci_device_hot_add_print,
-.mhandler.cmd_new = pci_device_hot_add,
+.mhandler.cmd = pci_device_hot_add,
},
#endif
diff --git a/sysemu.h b/sysemu.h
index fa921df..47975b5 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -201,8 +201,7 @@ extern DriveInfo *drive_init(QemuOpts *arg, void *machine,
int *fatal_error);
DriveInfo *add_init_drive(const char *opts);
/* pci-hotplug */
-void pci_device_hot_add_print(Monitor *mon, const QObject *data);
-int pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
+void pci_device_hot_add(Monitor *mon, const QDict *qdict);
void drive_hot_add(Monitor *mon, const QDict *qdict);
int pci_device_hot_remove(Monitor *mon, const char *pci_addr);
int do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,
--
1.7.1.86.g0e460
[Qemu-devel] [PATCH 1/7] QMP: Add Downstream extension of QMP to spec
From: Markus Armbruster arm...@redhat.com Signed-off-by: Markus Armbruster arm...@redhat.com --- QMP/qmp-spec.txt | 55 ++ 1 files changed, 55 insertions(+), 0 deletions(-) diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt index f3c0327..9d30a8c 100644 --- a/QMP/qmp-spec.txt +++ b/QMP/qmp-spec.txt @@ -215,3 +215,58 @@ Additionally, Clients must not assume any particular: - Order of json-object members or json-array elements - Amount of errors generated by a command, that is, new errors can be added to any existing command in newer versions of the Server + +6. Downstream extension of QMP +-- + +We recommend that downstream consumers of QEMU do *not* modify QMP. +Management tools should be able to support both upstream and downstream +versions of QMP without special logic, and downstream extensions are +inherently at odds with that. + +However, we recognize that it is sometimes impossible for downstreams to +avoid modifying QMP. Both upstream and downstream need to take care to +preserve long-term compatibility and interoperability. + +To help with that, QMP reserves JSON object member names beginning with +'__' (double underscore) for downstream use (downstream names). This +means upstream will never use any downstream names for its commands, +arguments, errors, asynchronous events, and so forth. + +Any new names downstream wishes to add must begin with '__'. To +ensure compatibility with other downstreams, it is strongly +recommended that you prefix your downstram names with '__RFQDN_' where +RFQDN is a valid, reverse fully qualified domain name which you +control. For example, a qemu-kvm specific monitor command would be: + +(qemu) __org.linux-kvm_enable_irqchip + +Downstream must not change the server greeting (section 2.2) other than +to offer additional capabilities. But see below for why even that is +discouraged. + +Section '5 Compatibility Considerations' applies to downstream as well +as to upstream, obviously. It follows that downstream must behave +exactly like upstream for any input not containing members with +downstream names (downstream members), except it may add members +with downstream names to its output. + +Thus, a client should not be able to distinguish downstream from +upstream as long as it doesn't send input with downstream members, and +properly ignores any downstream members in the output it receives. + +Advice on downstream modifications: + +1. Introducing new commands is okay. If you want to extend an existing + command, consider introducing a new one with the new behaviour + instead. + +2. Introducing new asynchronous messages is okay. If you want to extend + an existing message, consider adding a new one instead. + +3. Introducing new errors for use in new commands is okay. Adding new + errors to existing commands counts as extension, so 1. applies. + +4. New capabilities are strongly discouraged. Capabilities are for + evolving the basic protocol, and multiple diverging basic protocol + dialects are most undesirable. -- 1.7.1.86.g0e460
[Qemu-devel] [PATCH 7/7] Fix qtypes' licenses
- Change from GPL to LGPL - Add license text when missing - Minor cosmetic changes to make all headers look the same Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- check-qdict.c |3 +++ check-qfloat.c |5 - check-qint.c|3 +++ check-qlist.c |4 ++-- check-qstring.c |3 +++ qbool.c |8 qdict.c |6 +++--- qdict.h | 12 qemu-objects.h |5 +++-- qerror.c|2 +- qerror.h|2 +- qfloat.c|8 qint.c |7 --- qint.h | 12 qlist.c |7 --- qlist.h |7 --- qobject.h |4 ++-- qstring.c |7 --- qstring.h | 12 19 files changed, 73 insertions(+), 44 deletions(-) diff --git a/check-qdict.c b/check-qdict.c index f2b4826..2c3089f 100644 --- a/check-qdict.c +++ b/check-qdict.c @@ -5,6 +5,9 @@ * * Authors: * Luiz Capitulino lcapitul...@redhat.com + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. */ #include check.h diff --git a/check-qfloat.c b/check-qfloat.c index 3758700..b71d983 100644 --- a/check-qfloat.c +++ b/check-qfloat.c @@ -1,11 +1,6 @@ /* * QFloat unit-tests. * - * Copyright (C) 2009 Red Hat Inc. - * - * Authors: - * Luiz Capitulino lcapitul...@redhat.com - * * Copyright IBM, Corp. 2009 * * Authors: diff --git a/check-qint.c b/check-qint.c index 49887bb..f3b0316 100644 --- a/check-qint.c +++ b/check-qint.c @@ -5,6 +5,9 @@ * * Authors: * Luiz Capitulino lcapitul...@redhat.com + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. */ #include check.h diff --git a/check-qlist.c b/check-qlist.c index 0117ef3..58984cb 100644 --- a/check-qlist.c +++ b/check-qlist.c @@ -6,8 +6,8 @@ * Authors: * Luiz Capitulino lcapitul...@redhat.com * - * This work is licensed under the terms of the GNU GPL, version 2. See - * the COPYING file in the top-level directory. + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. */ #include check.h diff --git a/check-qstring.c b/check-qstring.c index c308a63..c9bafc2 100644 --- a/check-qstring.c +++ b/check-qstring.c @@ -5,6 +5,9 @@ * * Authors: * Luiz Capitulino lcapitul...@redhat.com + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. */ #include check.h diff --git a/qbool.c b/qbool.c index 5ab734c..ad4873f 100644 --- a/qbool.c +++ b/qbool.c @@ -1,14 +1,6 @@ /* * QBool Module * - * Copyright (C) 2009 Red Hat Inc. - * - * Authors: - * Luiz Capitulino lcapitul...@redhat.com - * - * This work is licensed under the terms of the GNU GPL, version 2. See - * the COPYING file in the top-level directory. - * * Copyright IBM, Corp. 2009 * * Authors: diff --git a/qdict.c b/qdict.c index aae57bf..175bc17 100644 --- a/qdict.c +++ b/qdict.c @@ -1,13 +1,13 @@ /* - * QDict data type. + * QDict Module * * Copyright (C) 2009 Red Hat Inc. * * Authors: * Luiz Capitulino lcapitul...@redhat.com * - * This work is licensed under the terms of the GNU GPL, version 2. See - * the COPYING file in the top-level directory. + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. */ #include qint.h diff --git a/qdict.h b/qdict.h index 579dcdd..5e5902c 100644 --- a/qdict.h +++ b/qdict.h @@ -1,3 +1,15 @@ +/* + * QDict Module + * + * Copyright (C) 2009 Red Hat Inc. + * + * Authors: + * Luiz Capitulino lcapitul...@redhat.com + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + */ + #ifndef QDICT_H #define QDICT_H diff --git a/qemu-objects.h b/qemu-objects.h index e1d1e0c..c53fbaa 100644 --- a/qemu-objects.h +++ b/qemu-objects.h @@ -6,9 +6,10 @@ * Authors: * Luiz Capitulino lcapitul...@redhat.com * - * This work is licensed under the terms of the GNU GPL, version 2. See - * the COPYING file in the top-level directory. + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. */ + #ifndef QEMU_OBJECTS_H #define QEMU_OBJECTS_H diff --git a/qerror.c b/qerror.c index 034c7de..44d0bf8 100644 --- a/qerror.c +++ b/qerror.c @@ -1,5 +1,5 @@ /* - * QError: QEMU Error data-type. + * QError Module * * Copyright (C) 2009 Red Hat Inc. * diff --git a/qerror.h b/qerror.h index c98c61a..77ae574 100644 --- a/qerror.h +++ b/qerror.h @@ -1,5 +1,5 @@ /* - * QError header file. + * QError Module * * Copyright (C) 2009 Red Hat Inc. * diff --git a/qfloat.c b/qfloat.c
[Qemu-devel] [PATCH 4/7] Revert Monitor: Return before exiting with 'quit'
This reverts commit 0e8d2b5575938b8876a3c4bb66ee13c5d306fb6d.
Next commits will do the same thing in a better way.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
monitor.c |3 +--
sysemu.h |2 --
vl.c | 18 --
3 files changed, 1 insertions(+), 22 deletions(-)
diff --git a/monitor.c b/monitor.c
index a1ebc5d..2e202ff 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1020,8 +1020,7 @@ static void do_info_cpu_stats(Monitor *mon)
*/
static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
-monitor_suspend(mon);
-qemu_system_exit_request();
+exit(0);
return 0;
}
diff --git a/sysemu.h b/sysemu.h
index 643c0c6..48ee66c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -45,11 +45,9 @@ void cpu_disable_ticks(void);
void qemu_system_reset_request(void);
void qemu_system_shutdown_request(void);
void qemu_system_powerdown_request(void);
-void qemu_system_exit_request(void);
int qemu_shutdown_requested(void);
int qemu_reset_requested(void);
int qemu_powerdown_requested(void);
-int qemu_exit_requested(void);
extern qemu_irq qemu_system_powerdown;
void qemu_system_reset(void);
diff --git a/vl.c b/vl.c
index d77b47c..8c818f0 100644
--- a/vl.c
+++ b/vl.c
@@ -1708,7 +1708,6 @@ static int shutdown_requested;
static int powerdown_requested;
int debug_requested;
int vmstop_requested;
-static int exit_requested;
int qemu_shutdown_requested(void)
{
@@ -1731,12 +1730,6 @@ int qemu_powerdown_requested(void)
return r;
}
-int qemu_exit_requested(void)
-{
-/* just return it, we'll exit() anyway */
-return exit_requested;
-}
-
static int qemu_debug_requested(void)
{
int r = debug_requested;
@@ -1807,12 +1800,6 @@ void qemu_system_powerdown_request(void)
qemu_notify_event();
}
-void qemu_system_exit_request(void)
-{
-exit_requested = 1;
-qemu_notify_event();
-}
-
#ifdef _WIN32
static void host_main_loop_wait(int *timeout)
{
@@ -1949,8 +1936,6 @@ static int vm_can_run(void)
return 0;
if (debug_requested)
return 0;
-if (exit_requested)
-return 0;
return 1;
}
@@ -2003,9 +1988,6 @@ static void main_loop(void)
if ((r = qemu_vmstop_requested())) {
vm_stop(r);
}
-if (qemu_exit_requested()) {
-exit(0);
-}
}
pause_all_vcpus();
}
--
1.7.1.86.g0e460
[Qemu-devel] [PATCH 0/7][PULL]: QMP/Monitor queue
Hi Anthony, The following QMP/Monitor patches have been sent to the list and look good to me. I have also tested most of them. Please, note that I didn't add the QMP doc patches, since they are still under discussion. The changes (since 0d5d46993840c1e6a04c1db38a554aad4ee83835) are available in the following repository: git://repo.or.cz/qemu/qmp-unstable.git for-anthony Luiz Capitulino (4): Revert Monitor: Return before exiting with 'quit' sysemu: Export 'no_shutdown' Monitor: Return before exiting with 'quit' Fix qtypes' licenses Markus Armbruster (3): QMP: Add Downstream extension of QMP to spec Revert PCI: Convert pci_device_hot_add() to QObject Revert monitor: Convert do_pci_device_hot_remove() to QObject QMP/qmp-spec.txt | 55 ++ check-qdict.c|3 ++ check-qfloat.c |5 check-qint.c |3 ++ check-qlist.c|4 +- check-qstring.c |3 ++ hw/pci-hotplug.c | 51 ++--- monitor.c|4 ++- qbool.c |8 --- qdict.c |6 ++-- qdict.h | 12 +++ qemu-monitor.hx |6 +--- qemu-objects.h |5 ++- qerror.c |2 +- qerror.h |2 +- qfloat.c |8 --- qint.c |7 +++-- qint.h | 12 +++ qlist.c |7 +++-- qlist.h |7 +++-- qobject.h|4 +- qstring.c|7 +++-- qstring.h| 12 +++ sysemu.h |9 ++- vl.c | 18 - 25 files changed, 143 insertions(+), 117 deletions(-)
[Qemu-devel] [PATCH 5/7] sysemu: Export 'no_shutdown'
It's a global variable already, do_quit() will use it. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- sysemu.h |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/sysemu.h b/sysemu.h index 48ee66c..879446a 100644 --- a/sysemu.h +++ b/sysemu.h @@ -128,6 +128,7 @@ extern int max_cpus; extern int cursor_hide; extern int graphic_rotate; extern int no_quit; +extern int no_shutdown; extern int semihosting_enabled; extern int old_param; extern int boot_menu; -- 1.7.1.86.g0e460
[Qemu-devel] [PATCH 6/7] Monitor: Return before exiting with 'quit'
This is a new version of the (now reverted) following commit:
0e8d2b5575938b8876a3c4bb66ee13c5d306fb6d
The 'quit' Monitor command (implemented by do_quit()) calls
exit() directly, this is problematic under QMP because QEMU
exits before having a chance to send the ok response.
Clients don't know if QEMU exited because of a problem or
because the 'quit' command has been executed.
This commit fixes that by making do_quit() use
qemu_system_shutdown_request(), so that we exit gracefully.
Thanks to Paolo Bonzini pbonz...@redhat.com for suggesting
this solution.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
monitor.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/monitor.c b/monitor.c
index 2e202ff..ad50f12 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1020,7 +1020,10 @@ static void do_info_cpu_stats(Monitor *mon)
*/
static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
-exit(0);
+monitor_suspend(mon);
+no_shutdown = 0;
+qemu_system_shutdown_request();
+
return 0;
}
--
1.7.1.86.g0e460
[Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer
Hi Anthony,
While investigating a QMP bug reported by a user, I've found a few issues
in our parser/lexer.
The patches in this series fix the problems I was able to solve, but we
still have the following issues:
1. Our 'private extension' is open to the public
Eg. The following input issued by a client is valid:
{ 'execute': 'query-pci' }
I don't think it's a good idea to have clients relying on this kind of
JSON extension.
To fix this we could add a 'extension' flag to JSONLexer and set it to
nonzero in internal functions (eg. qobject_from_jsonf()), of course that
the lexer code should handle this too.
2. QMP doesn't check the return of json_message_parser_feed()
Which means we don't handle JSON syntax errors. While the fix might seem
trivial (ie. just return an error!), I'm not sure what's the best way
to handle this, because the streamer seems to return multiple errors for
the same input string.
For example, this input:
{ execute: yy_uu }
Seems to return an error for each bad character (yy_uu), shouldn't it
return only once and stop processing the whole string?
3. The lexer enter in ERROR state when processing is done
Not sure whether this is an issue, but I found it while reviewing the code
and maybe this is related with item 2 above.
When json_lexer_feed_char() is finished scanning a string, (ie. ch='\0')
the JSON_SKIP clause will set lexer-state to ERROR as there's no entry
for '\0' in the IN_START array.
Shouldn't we have a LEXER_DONE or something like it instead?
4. Lexer expects a 'terminal' char to process a token
Which means clients must send a sort of end of line char, so that we
process their input.
Maybe I'm missing something here, but I thought that the whole point of
writing our own parser was to avoid this.
[Qemu-devel] [PATCH 1/6] json-lexer: Initialize 'x' and 'y'
The 'lexer' variable is passed by the caller, it can contain anything (eg. garbage). Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- json-lexer.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/json-lexer.c b/json-lexer.c index 9d64920..0b145d1 100644 --- a/json-lexer.c +++ b/json-lexer.c @@ -275,6 +275,7 @@ void json_lexer_init(JSONLexer *lexer, JSONLexerEmitter func) lexer-emit = func; lexer-state = IN_START; lexer-token = qstring_new(); +lexer-x = lexer-y = 0; } static int json_lexer_feed_char(JSONLexer *lexer, char ch) -- 1.7.1.86.g0e460
[Qemu-devel] [PATCH 2/6] json-lexer: Handle missing escapes
The JSON escape sequence \/ and \\ are valid and should be
handled.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
json-lexer.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/json-lexer.c b/json-lexer.c
index 0b145d1..5cc7e6c 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -97,6 +97,8 @@ static const uint8_t json_lexer[][256] = {
['n'] = IN_DQ_STRING,
['r'] = IN_DQ_STRING,
['t'] = IN_DQ_STRING,
+['/'] = IN_DQ_STRING,
+['\\'] = IN_DQ_STRING,
['\''] = IN_DQ_STRING,
['\'] = IN_DQ_STRING,
['u'] = IN_DQ_UCODE0,
@@ -134,6 +136,8 @@ static const uint8_t json_lexer[][256] = {
['n'] = IN_SQ_STRING,
['r'] = IN_SQ_STRING,
['t'] = IN_SQ_STRING,
+['/'] = IN_DQ_STRING,
+['\\'] = IN_DQ_STRING,
['\''] = IN_SQ_STRING,
['\'] = IN_SQ_STRING,
['u'] = IN_SQ_UCODE0,
--
1.7.1.86.g0e460
[Qemu-devel] [PATCH 4/6] check-qjson: Add more escape tests
While there make the fail_unless() calls print error messages.
IMPORTANT: The test for \/ is failing, don't know why.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
check-qjson.c | 14 --
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/check-qjson.c b/check-qjson.c
index 109e777..d365799 100644
--- a/check-qjson.c
+++ b/check-qjson.c
@@ -29,6 +29,13 @@ START_TEST(escaped_string)
const char *decoded;
int skip;
} test_cases[] = {
+{ \\\b\, \b },
+{ \\\f\, \f },
+{ \\\n\, \n },
+{ \\\r\, \r },
+{ \\\t\, \t },
+{ \\\/\, \\/ },
+{ \\, \\ },
{ \, \ },
{ \hello world \\\embedded string,
hello world \embedded string\ },
@@ -49,11 +56,14 @@ START_TEST(escaped_string)
fail_unless(qobject_type(obj) == QTYPE_QSTRING);
str = qobject_to_qstring(obj);
-fail_unless(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
+fail_unless(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0,
+%s != %s\n, qstring_get_str(str), test_cases[i].decoded);
if (test_cases[i].skip == 0) {
str = qobject_to_json(obj);
-fail_unless(strcmp(qstring_get_str(str), test_cases[i].encoded) ==
0);
+fail_unless(strcmp(qstring_get_str(str),test_cases[i].encoded) ==
0,
+%s != %s\n, qstring_get_str(str),
+ test_cases[i].encoded);
qobject_decref(obj);
}
--
1.7.1.86.g0e460
[Qemu-devel] [PATCH 3/6] qjson: Handle \f
It's valid JSON and should be handled. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- json-parser.c |4 qjson.c |3 +++ 2 files changed, 7 insertions(+), 0 deletions(-) diff --git a/json-parser.c b/json-parser.c index b55d763..83212bc 100644 --- a/json-parser.c +++ b/json-parser.c @@ -206,6 +206,10 @@ static QString *qstring_from_escaped_str(JSONParserContext *ctxt, QObject *token qstring_append(str, \b); ptr++; break; +case 'f': +qstring_append(str, \f); +ptr++; +break; case 'n': qstring_append(str, \n); ptr++; diff --git a/qjson.c b/qjson.c index 483c667..e4ee433 100644 --- a/qjson.c +++ b/qjson.c @@ -158,6 +158,9 @@ static void to_json(const QObject *obj, QString *str) case '\b': qstring_append(str, \\b); break; +case '\f': +qstring_append(str, \\f); +break; case '\n': qstring_append(str, \\n); break; -- 1.7.1.86.g0e460
[Qemu-devel] [PATCH 6/6] json-streamer: Don't use qdict_put_obj()
It's not needed, use qobject_put() instead and get a cleaner code. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- json-streamer.c |8 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/json-streamer.c b/json-streamer.c index 610ffea..f7e7a68 100644 --- a/json-streamer.c +++ b/json-streamer.c @@ -43,11 +43,11 @@ static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTok } dict = qdict_new(); -qdict_put_obj(dict, type, QOBJECT(qint_from_int(type))); +qdict_put(dict, type, qint_from_int(type)); QINCREF(token); -qdict_put_obj(dict, token, QOBJECT(token)); -qdict_put_obj(dict, x, QOBJECT(qint_from_int(x))); -qdict_put_obj(dict, y, QOBJECT(qint_from_int(y))); +qdict_put(dict, token, token); +qdict_put(dict, x, qint_from_int(x)); +qdict_put(dict, y, qint_from_int(y)); qlist_append(parser-tokens, dict); -- 1.7.1.86.g0e460
[Qemu-devel] [PATCH 5/6] json-lexer: Drop 'buf'
QString supports adding a single char, 'buf' is unneeded.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
json-lexer.c |7 +--
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/json-lexer.c b/json-lexer.c
index 5cc7e6c..1d9b81f 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -284,8 +284,6 @@ void json_lexer_init(JSONLexer *lexer, JSONLexerEmitter
func)
static int json_lexer_feed_char(JSONLexer *lexer, char ch)
{
-char buf[2];
-
lexer-x++;
if (ch == '\n') {
lexer-x = 0;
@@ -313,10 +311,7 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
break;
}
-buf[0] = ch;
-buf[1] = 0;
-
-qstring_append(lexer-token, buf);
+qstring_append_chr(lexer-token, ch);
return 0;
}
--
1.7.1.86.g0e460
Re: [Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer
On Wed, 19 May 2010 16:43:08 -0500
Anthony Liguori anth...@codemonkey.ws wrote:
On 05/19/2010 04:15 PM, Luiz Capitulino wrote:
Hi Anthony,
While investigating a QMP bug reported by a user, I've found a few issues
in our parser/lexer.
The patches in this series fix the problems I was able to solve, but we
still have the following issues:
1. Our 'private extension' is open to the public
Eg. The following input issued by a client is valid:
{ 'execute': 'query-pci' }
I don't think it's a good idea to have clients relying on this kind of
JSON extension.
To fix this we could add a 'extension' flag to JSONLexer and set it to
nonzero in internal functions (eg. qobject_from_jsonf()), of course that
the lexer code should handle this too.
The JSON specification explicitly says:
A JSON parser transforms a JSON text into another representation. A
JSON parser MUST accept all texts that conform to the JSON grammar. A
JSON parser MAY accept non-JSON forms or extensions.
IOW, we're under no obligation to reject extensions and I can't think of
a reason why we should.
I know we're legal, but what's the point to offer this extension to clients?
The main motivation behind this was to write JSON in C strings w/o the
need of repetitive escapes. This is internal to QEMU, but it's also
available to clients for no reason.
And you know, after 0.13 we won't be able to remove it.
2. QMP doesn't check the return of json_message_parser_feed()
Which means we don't handle JSON syntax errors. While the fix might seem
trivial (ie. just return an error!), I'm not sure what's the best way
to handle this, because the streamer seems to return multiple errors for
the same input string.
For example, this input:
{ execute: yy_uu }
Seems to return an error for each bad character (yy_uu), shouldn't it
return only once and stop processing the whole string?
It probably should kill the connection.
Ok.
3. The lexer enter in ERROR state when processing is done
Not sure whether this is an issue, but I found it while reviewing the
code
and maybe this is related with item 2 above.
When json_lexer_feed_char() is finished scanning a string, (ie. ch='\0')
the JSON_SKIP clause will set lexer-state to ERROR as there's no entry
for '\0' in the IN_START array.
Shouldn't we have a LEXER_DONE or something like it instead?
No, you must have malformed input if an error occurs.
Yes, json_message_parser_feed() returns OK.
[IN_WHITESPACE] - TERMINAL(JSON_SKIP)
JSON_SKIP is a terminal so once you're in that state, you go back to
IN_START.
Yes, but what I'm trying to say is that when ch='\0' and you do:
lexer-state = json_lexer[IN_START][(uint8_t)ch];
Then 'lexer-state' becomes 0, which is what the code recognizes as ERROR.
Again, not sure if this is an issue. Just caught my attention.
4. Lexer expects a 'terminal' char to process a token
Which means clients must send a sort of end of line char, so that we
process their input.
Maybe I'm missing something here, but I thought that the whole point of
writing our own parser was to avoid this.
If the lexer gets:
abc
It has no way of knowing if that's a token or if we're going to get:
abcd
As a token. You can fix this in two ways. You can either flush() the
lexer to significant end of input or you can wait until there's some
other valid symbol to cause the previous symbol to be emitted.
IOW, a client either needs to: 1) send the request and follow it with a
newline or some form of whitespace or 2) close the connection to flush
the request
Ok.
Re: [Qemu-devel] [PATCH 2/6] json-lexer: Handle missing escapes
On Wed, 19 May 2010 16:44:47 -0500
Anthony Liguori anth...@codemonkey.ws wrote:
On 05/19/2010 04:15 PM, Luiz Capitulino wrote:
The JSON escape sequence \/ and \\ are valid and should be
handled.
Signed-off-by: Luiz Capitulinolcapitul...@redhat.com
Good catch.
I think there's another issue in the handling of strings.
The spec says that valid unescaped chars are in the following range:
unescaped = %x20-21 / %x23-5B / %x5D-10
But we do:
[IN_DQ_STRING] = {
[1 ... 0xFF] = IN_DQ_STRING,
['\\'] = IN_DQ_STRING_ESCAPE,
[''] = IN_DONE_STRING,
},
Shouldn't we cover 0x20 .. 0xFF instead?
[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes
On Thu, 20 May 2010 17:16:01 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
I think there's another issue in the handling of strings.
The spec says that valid unescaped chars are in the following range:
unescaped = %x20-21 / %x23-5B / %x5D-10
But we do:
[IN_DQ_STRING] = {
[1 ... 0xFF] = IN_DQ_STRING,
['\\'] = IN_DQ_STRING_ESCAPE,
[''] = IN_DONE_STRING,
},
Shouldn't we cover 0x20 .. 0xFF instead?
If it's the lexer, isn't just it being liberal in what it accepts?
Yes, it's the lexer, but you meant that the fix should be in
somewhere else?
[Qemu-devel] Re: [PATCH 0/6]: QMP: Fix issues in parser/lexer
On Thu, 20 May 2010 17:18:23 +0200 Paolo Bonzini pbonz...@redhat.com wrote: On 05/19/2010 11:43 PM, Anthony Liguori wrote: 4. Lexer expects a 'terminal' char to process a token Which means clients must send a sort of end of line char, so that we process their input. Maybe I'm missing something here, but I thought that the whole point of writing our own parser was to avoid this. If the lexer gets: abc It has no way of knowing if that's a token or if we're going to get: abcd Only } and ] are valid characters at the end of a JSON object, and neither requires lookahead. Good point.
[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes
On Thu, 20 May 2010 17:26:03 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
On 05/20/2010 05:25 PM, Luiz Capitulino wrote:
On Thu, 20 May 2010 17:16:01 +0200
Paolo Bonzinipbonz...@redhat.com wrote:
On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
I think there's another issue in the handling of strings.
The spec says that valid unescaped chars are in the following range:
unescaped = %x20-21 / %x23-5B / %x5D-10
But we do:
[IN_DQ_STRING] = {
[1 ... 0xFF] = IN_DQ_STRING,
['\\'] = IN_DQ_STRING_ESCAPE,
[''] = IN_DONE_STRING,
},
Shouldn't we cover 0x20 .. 0xFF instead?
If it's the lexer, isn't just it being liberal in what it accepts?
Yes, it's the lexer, but you meant that the fix should be in
somewhere else?
I meant that we're just accepting some invalid JSON and that's not a big
deal.
It can become a big deal if clients rely on it and for some reason we
decide we should drop it. Ie. after QMP is declared stable such changes
won't be allowed.
Yes, I know, the chances of someone relying on this kind of thing is
probably almost zero. At the same time I think we should be very
conservative if there's no good reason to do otherwise.
[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes
On Thu, 20 May 2010 10:50:41 -0500
Anthony Liguori anth...@codemonkey.ws wrote:
On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
I think there's another issue in the handling of strings.
The spec says that valid unescaped chars are in the following range:
unescaped = %x20-21 / %x23-5B / %x5D-10
That's a spec bug IMHO. Tab is %x09. Surely you can include tabs in
strings. Any parser that didn't accept that would be broken.
Honestly, I had the impression this should be encoded as: %x5C %x74, but
if you're right, wouldn't this be true for other sequences as well?
But we do:
[IN_DQ_STRING] = {
[1 ... 0xFF] = IN_DQ_STRING,
['\\'] = IN_DQ_STRING_ESCAPE,
[''] = IN_DONE_STRING,
},
Shouldn't we cover 0x20 .. 0xFF instead?
If it's the lexer, isn't just it being liberal in what it accepts?
I believe the parser correctly rejects invalid UTF-8 sequences.
Will check.
[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes
On Thu, 20 May 2010 10:54:42 -0500 Anthony Liguori anth...@codemonkey.ws wrote: On 05/20/2010 10:35 AM, Luiz Capitulino wrote: I meant that we're just accepting some invalid JSON and that's not a big deal. It can become a big deal if clients rely on it and for some reason we decide we should drop it. Ie. after QMP is declared stable such changes won't be allowed. Clients should only rely on standard JSON. Anything else is a bug in the client. I feel this is like a trap, why exposing it if don't want clients to use them?
[Qemu-devel] Re: [PATCH 0/6]: QMP: Fix issues in parser/lexer
On Thu, 20 May 2010 10:52:58 -0500 Anthony Liguori anth...@codemonkey.ws wrote: On 05/20/2010 10:18 AM, Paolo Bonzini wrote: On 05/19/2010 11:43 PM, Anthony Liguori wrote: 4. Lexer expects a 'terminal' char to process a token Which means clients must send a sort of end of line char, so that we process their input. Maybe I'm missing something here, but I thought that the whole point of writing our own parser was to avoid this. If the lexer gets: abc It has no way of knowing if that's a token or if we're going to get: abcd Only } and ] are valid characters at the end of a JSON object, and neither requires lookahead. Having look ahead operate differently for different states really complicates the lexer. I don't see this as a big problem in practice. Would be a nice feature, but it's fine for me too and we'll have to note that in the QMP's spec.
[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes
On Thu, 20 May 2010 11:55:00 -0500
Anthony Liguori anth...@codemonkey.ws wrote:
On 05/20/2010 11:27 AM, Luiz Capitulino wrote:
On Thu, 20 May 2010 10:50:41 -0500
Anthony Liguorianth...@codemonkey.ws wrote:
On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
I think there's another issue in the handling of strings.
The spec says that valid unescaped chars are in the following range:
unescaped = %x20-21 / %x23-5B / %x5D-10
That's a spec bug IMHO. Tab is %x09. Surely you can include tabs in
strings. Any parser that didn't accept that would be broken.
Honestly, I had the impression this should be encoded as: %x5C %x74, but
if you're right, wouldn't this be true for other sequences as well?
I don't think most reasonable clients are going to quote tabs as '\t'.
That would be a bug, wouldn't it?
Python example:
json.dumps('\t')
'\\t'
YAJL example (inlined below):
/tmp/ ./teste
0x22 0x5c 0x74 0x22
/tmp/
I think we should strictly conform to the spec, quirks should only be
added when really needed.
#include stdio.h
#include yajl/yajl_gen.h
int main(void)
{
yajl_gen g;
unsigned int i, len = 0;
const unsigned char *str = NULL;
yajl_gen_config conf = { 0, };
g = yajl_gen_alloc(conf, NULL);
if (yajl_gen_string(g, (unsigned char *) \t, 1) != yajl_gen_status_ok)
return 1;
if (yajl_gen_get_buf(g, str, len) != yajl_gen_status_ok)
return 1;
for (i = 0; i len; i++)
printf(0x%x , str[i]);
printf(\n);
return 0;
}
[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes
On Thu, 20 May 2010 13:52:08 -0500 Anthony Liguori anth...@codemonkey.ws wrote: On 05/20/2010 01:47 PM, Luiz Capitulino wrote: On Thu, 20 May 2010 11:55:00 -0500 Anthony Liguorianth...@codemonkey.ws wrote: On 05/20/2010 11:27 AM, Luiz Capitulino wrote: On Thu, 20 May 2010 10:50:41 -0500 Anthony Liguorianth...@codemonkey.ws wrote: On 05/20/2010 10:16 AM, Paolo Bonzini wrote: On 05/20/2010 03:44 PM, Luiz Capitulino wrote: I think there's another issue in the handling of strings. The spec says that valid unescaped chars are in the following range: unescaped = %x20-21 / %x23-5B / %x5D-10 That's a spec bug IMHO. Tab is %x09. Surely you can include tabs in strings. Any parser that didn't accept that would be broken. Honestly, I had the impression this should be encoded as: %x5C %x74, but if you're right, wouldn't this be true for other sequences as well? I don't think most reasonable clients are going to quote tabs as '\t'. That would be a bug, wouldn't it? Tabs are valid in JavaScript strings and I don't think it's reasonable to expect that a valid JavaScript string is not a valid JSON string. IMO, we should do what the spec says and what bug free clients expect, what we consider reasonable or unreasonable is a different matter. I would be with you if the spec was proved wrong, specially if reference implementations out there didn't follow it either, but everything I found so far shows this is not the case. Another example: http://www.json.org/json2.js Search for 'character substitutions'.
Re: [Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer
On Thu, 20 May 2010 10:35:52 -0300
Luiz Capitulino lcapitul...@redhat.com wrote:
2. QMP doesn't check the return of json_message_parser_feed()
Which means we don't handle JSON syntax errors. While the fix might
seem
trivial (ie. just return an error!), I'm not sure what's the best way
to handle this, because the streamer seems to return multiple errors
for
the same input string.
For example, this input:
{ execute: yy_uu }
Seems to return an error for each bad character (yy_uu), shouldn't it
return only once and stop processing the whole string?
It probably should kill the connection.
Ok.
I was working on this and it's not that simple.
As we talked by IRC, there isn't an easy way to just drop a qemu-char
connection
as a char device doesn't have a notion of connect/disconnect (as you explained).
So, another way of doing this would be to close the chardev and open it again,
however this is also complicated for two reasons:
1. qemu_chr_close() completely destroys the chardev and creating it again
requires
access to the original QemuOpts, created at boot time
2. Some char devices' open() method have specific boot-time only options
(eg. socket wait)
It's possible to hack, we could make a copy of the QemOpts, remove boot-time
options and store it in CharDriverState. But this is ugly and fragile.
Isn't it easier to handle this in the lexer itself? For example, couldn't it
just mark the token as bad and keep scanning?
We handle parser's errors just fine.
[Qemu-devel] Re: [PATCH v2 12/15] monitor: Add basic device state visualization
On Sun, 23 May 2010 09:57:43 +0200 Jan Kiszka jan.kis...@web.de wrote: Avi Kivity wrote: [...] +- full: report full state (json-bool, optional) Is this needed for QMP? The client can always truncate it to any length. The effect may not be needed for QMP, but I do need this channel from the command line to the monitor pretty-printer. I could just stick full: json-bool back into the return dict, but that would look somehow strange IMO. We could have a form of optional key which is only present internally, we have that async commands.
Re: [Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes
On Mon, 24 May 2010 14:29:58 -0500 Anthony Liguori anth...@codemonkey.ws wrote: On 05/20/2010 02:22 PM, Luiz Capitulino wrote: On Thu, 20 May 2010 13:52:08 -0500 Anthony Liguorianth...@codemonkey.ws wrote: On 05/20/2010 01:47 PM, Luiz Capitulino wrote: On Thu, 20 May 2010 11:55:00 -0500 Anthony Liguorianth...@codemonkey.ws wrote: On 05/20/2010 11:27 AM, Luiz Capitulino wrote: On Thu, 20 May 2010 10:50:41 -0500 Anthony Liguorianth...@codemonkey.wswrote: On 05/20/2010 10:16 AM, Paolo Bonzini wrote: On 05/20/2010 03:44 PM, Luiz Capitulino wrote: I think there's another issue in the handling of strings. The spec says that valid unescaped chars are in the following range: unescaped = %x20-21 / %x23-5B / %x5D-10 That's a spec bug IMHO. Tab is %x09. Surely you can include tabs in strings. Any parser that didn't accept that would be broken. Honestly, I had the impression this should be encoded as: %x5C %x74, but if you're right, wouldn't this be true for other sequences as well? I don't think most reasonable clients are going to quote tabs as '\t'. That would be a bug, wouldn't it? Tabs are valid in JavaScript strings and I don't think it's reasonable to expect that a valid JavaScript string is not a valid JSON string. IMO, we should do what the spec says and what bug free clients expect, what we consider reasonable or unreasonable is a different matter. How we encode strings is one thing, what we accept is something else. True. Why shouldn't we be liberal in what we accept? It doesn't violate the spec to accept more than it requires so why shouldn't we? For the reasons outlined by Avi, not sure how this serious this is though.
Re: [Qemu-devel] [PATCH 1/5] Exit if incoming migration fails
On Tue, 25 May 2010 16:21:01 +0200
Juan Quintela quint...@redhat.com wrote:
Signed-off-by: Juan Quintela quint...@redhat.com
---
migration.c | 16 ++--
migration.h |2 +-
vl.c|7 ++-
3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/migration.c b/migration.c
index 05f6cc5..9c1d4b6 100644
--- a/migration.c
+++ b/migration.c
@@ -36,22 +36,26 @@ static uint32_t max_throttle = (32 20);
static MigrationState *current_migration;
-void qemu_start_incoming_migration(const char *uri)
+int qemu_start_incoming_migration(const char *uri)
{
const char *p;
+int ret;
if (strstart(uri, tcp:, p))
-tcp_start_incoming_migration(p);
+ret = tcp_start_incoming_migration(p);
#if !defined(WIN32)
else if (strstart(uri, exec:, p))
-exec_start_incoming_migration(p);
+ret = exec_start_incoming_migration(p);
else if (strstart(uri, unix:, p))
-unix_start_incoming_migration(p);
+ret = unix_start_incoming_migration(p);
else if (strstart(uri, fd:, p))
-fd_start_incoming_migration(p);
+ret = fd_start_incoming_migration(p);
#endif
-else
+else {
fprintf(stderr, unknown migration protocol: %s\n, uri);
+ret = -EPROTONOSUPPORT;
+}
+return ret;
}
int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
diff --git a/migration.h b/migration.h
index 385423f..dd423a1 100644
--- a/migration.h
+++ b/migration.h
@@ -50,7 +50,7 @@ struct FdMigrationState
void *opaque;
};
-void qemu_start_incoming_migration(const char *uri);
+int qemu_start_incoming_migration(const char *uri);
int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
diff --git a/vl.c b/vl.c
index 328395e..d13440d 100644
--- a/vl.c
+++ b/vl.c
@@ -3823,7 +3823,12 @@ int main(int argc, char **argv, char **envp)
}
if (incoming) {
-qemu_start_incoming_migration(incoming);
+int ret = qemu_start_incoming_migration(incoming);
+if (ret 0) {
+fprintf(stderr, Migration failed. Exit code %s(%d), exiting.\n,
+incoming, ret);
+exit(ret);
While I agree on the change, I have two comments:
1. By taking a look at the code I have the impression that most of the
fun failures will happen on the handler passed to qemu_set_fd_handler2(),
do you agree? Any plan to address that?
1. Is exit()ing the best thing to be done? I understand it's the easiest
and maybe better than nothing, but wouldn't it be better to enter in
paused-forever state so that clients can query and decide what to do?
+}
} else if (autostart) {
vm_start();
}
[Qemu-devel] Re: [PATCH 3/5] QMP: Introduce MIGRATION events
On Tue, 25 May 2010 11:10:23 -0500 Anthony Liguori anth...@codemonkey.ws wrote: There should be some information about why it failed, no? Preferrably in a QError format. At this point, we have basically -1 :( I can add a field with an error number, but we are very bad at the moment about moving errno's upstack. We need a better solution for reporting errors via notifications. Suggestions? Notice that what we need now is a way to know if migration ended with success or in any other way, as soon as possible. Markus/Luiz? We need to redesign QError. I could give it a try, but quite frankly, I don't know how do it good enough.. Markus has worked more on error handling than me though and I think he's the best person to do it, but he's busy at other things atm. Note that major work is design, not code churn (could turn out into an issue, but I doubt it).
Re: [Qemu-devel] Re: [PATCH 3/5] QMP: Introduce MIGRATION events
On Tue, 25 May 2010 17:35:53 +0200 Juan Quintela quint...@redhat.com wrote: Anthony Liguori anth...@codemonkey.ws wrote: On 05/25/2010 09:21 AM, Juan Quintela wrote: +MIGRATION_CANCELED +-- + +Emitted when migration is canceled. This is emitted in the source. +Target will emit MIGRATION_FAILED (no way to differentiate a FAILED +and CANCELED migration for target). But the management tool is the one that cancels so surely, it knows why already. ok, then that one is ok. Isn't this one important for the destination instead?
Re: [Qemu-devel] [PATCH 3/5] QMP: Introduce MIGRATION events
On Tue, 25 May 2010 16:21:03 +0200
Juan Quintela quint...@redhat.com wrote:
They are emitted when migration starts, ends, has a failure or is canceled.
Signed-off-by: Juan Quintela quint...@redhat.com
---
QMP/qmp-events.txt | 50 ++
monitor.c | 12
monitor.h |4
3 files changed, 66 insertions(+), 0 deletions(-)
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 01ec85f..93caa4d 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -26,6 +26,56 @@ Example:
Note: If action is stop, a STOP event will eventually follow the
BLOCK_IO_ERROR event.
+MIGRATION_CANCELED
+--
+
+Emitted when migration is canceled. This is emitted in the source.
Shouldn't this one be emitted in the destination?
+Target will emit MIGRATION_FAILED (no way to differentiate a FAILED
+and CANCELED migration for target).
+
+Data: None
+
+Example:
+
+{ event: MIGRATION_CANCELED,
+timestamp: {seconds: 1274687575, microseconds: 592483} }
+
+MIGRATION_ENDED
+---
+
+Emitted when migration ends (both in source and target)
+
+Data: None
+
+Example:
+
+{ event: MIGRATION_ENDED,
+timestamp: {seconds: 1274687575, microseconds: 592483} }
+
+MIGRATION_FAILED
+
+
+Emitted when migration fails (both is source and target).
+
+Data: None
+
+Example:
+
+{ event: MIGRATION_FAILED,
+timestamp: {seconds: 1274687575, microseconds: 592483} }
What about a MIGRATION_FINISHED event, which contains a 'success'
key which is a bool?
The only disadvantage of this is if we decide to add more information
to the event (say, stats) then it'd get ugly. Otherwise, one event is enough.
Anyway, the counterpart of MIGRATION_FAILED is MIGRATION_SUCCEEDED.
+
+MIGRATION_STARTED
+-
+
+Emitted when migration starts (both in source and target).
Don't you need this only on the destination?
Re: [Qemu-devel] [PATCH 3/5] QMP: Introduce MIGRATION events
On Tue, 25 May 2010 13:51:01 -0500
Anthony Liguori anth...@codemonkey.ws wrote:
On 05/25/2010 01:31 PM, Luiz Capitulino wrote:
On Tue, 25 May 2010 16:21:03 +0200
Juan Quintelaquint...@redhat.com wrote:
They are emitted when migration starts, ends, has a failure or is canceled.
Signed-off-by: Juan Quintelaquint...@redhat.com
---
QMP/qmp-events.txt | 50
++
monitor.c | 12
monitor.h |4
3 files changed, 66 insertions(+), 0 deletions(-)
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 01ec85f..93caa4d 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -26,6 +26,56 @@ Example:
Note: If action is stop, a STOP event will eventually follow the
BLOCK_IO_ERROR event.
+MIGRATION_CANCELED
+--
+
+Emitted when migration is canceled. This is emitted in the source.
Shouldn't this one be emitted in the destination?
Destination can't distinguish a cancelled from a closed pipe. But the
idea is that a third party is talking to both source and destination so
it knows if it's cancelled the migration.
Yeah, got it.
+Target will emit MIGRATION_FAILED (no way to differentiate a FAILED
+and CANCELED migration for target).
+
+Data: None
+
+Example:
+
+{ event: MIGRATION_CANCELED,
+timestamp: {seconds: 1274687575, microseconds: 592483} }
+
+MIGRATION_ENDED
+---
+
+Emitted when migration ends (both in source and target)
+
+Data: None
+
+Example:
+
+{ event: MIGRATION_ENDED,
+timestamp: {seconds: 1274687575, microseconds: 592483} }
+
+MIGRATION_FAILED
+
+
+Emitted when migration fails (both is source and target).
+
+Data: None
+
+Example:
+
+{ event: MIGRATION_FAILED,
+timestamp: {seconds: 1274687575, microseconds: 592483} }
What about a MIGRATION_FINISHED event, which contains a 'success'
key which is a bool?
The only disadvantage of this is if we decide to add more information
to the event (say, stats) then it'd get ugly. Otherwise, one event is
enough.
Anyway, the counterpart of MIGRATION_FAILED is MIGRATION_SUCCEEDED.
I see MIGRATION_FAILED as being very similar to block I/O error events.
I think we'll need a very similar solution for both. It boils down to,
how do we raise asynchronous events when something fails?
You mean we should have a sort of error specific events?
[Qemu-devel] Re: [PATCH 3/5] QMP: Introduce MIGRATION events
On Wed, 26 May 2010 11:55:31 -0500
Anthony Liguori anth...@codemonkey.ws wrote:
On 05/26/2010 10:15 AM, Daniel P. Berrange wrote:
On Wed, May 26, 2010 at 09:54:22AM -0500, Anthony Liguori wrote:
On 05/26/2010 05:33 AM, Daniel P. Berrange wrote:
I'm not sure why you would need a notification of when migration
starts (since you know when you've started migration).
But you don't know if the other end knows that it has also started.
started is needed only in incoming part, because we don't have a
monitor to ask if migration has started.
If we ever want to get closer to allowing multiple monitors, or allowing
apps to issue QMP commands directly via libvirt, then we still need the
'migration started' event on the source, because something else can
have issued the 'migrate' command without the mgmt app knowing.
Migration started doesn't help multiple monitors. You need locking of
some sort.
Part of the problem is the QMP migrate command is implemented as a
synchronous command. It really ought to be an asynchronous command.
That tells you when the migration has actually completed without polling.
Handling asynchronous commands is alot more complicated and error
prone for client apps, than providing a asynchronous event notification
of the lifecycle stages. If you want to also query status while waiting
for the completion, it means you can have to deal with overlapping
command execute+return pairs within a single monitor connection.
AFAICT this requires a change to QMP to require a unique ID to be
sent with the {'execute'..} command and be sent back with the later
corresponding {'return'...} data, so you can actually correlate
reliably.
That's exactly how the protocol is designed. That was one of the major
improvements of QMP over the human monior.
Yes and it already has 'id' support:
{ execute: cont, id: luiz }
{timestamp: {seconds: 1274966635, microseconds: 776813}, event:
RESUME}
{return: {}, id: luiz}
But it doesn't detect duplicates, this is something I think it's up
to the client to do, do you agree?
This is how the info balloon command works, BTW.
I won't remember the details now, but that interface has some issues and it
has to be reviewed.
Since there's a clear correlation between the request and the result of
the request, an asynchronous command is what makes the most sense. It
eliminates the problem of how to pass QErrors via an event which is one
of the problems with the current event proposal.
Not exactly, this is a problem with QError not the event proposal. We'll
have the same issue if we decide to include errno in the migrate errors and
the problem still exists with the BLOCK_IO_ERROR event.
That said, I do agree that migrate should be asynchronous. This yet another
thing we may want to fix before 0.13.
[...]
For tcp: and unix:, a CONNECTED event absolutely makes sense (every
socket server should emit a CONNECTED event). Unfortunately, after
CONNECTED you lose the monitor until migration is complete. If
something bad happens, you have to exit qemu so once the monitor
returns, migration has completed successfully.
If we introduce live incoming migration, we'll need to rethink things.
I would actually suggest that we deprecate the incoming command if we do
that and make incoming migration a monitor command. I would think it
should have the same semantics as migrate (as an asynchronous command).
A CONNECTED event still makes sense for tcp and unix protocols but I
don't think events make sense for start stop vs. an asynchronous command
completion.
Do you actually mean 'deprecate -incoming arg' here ?
Yes. And by deprecate, I really mean that -incoming just becomes
syntactic sugar for executing a monitor command immediately.
But we can't change -incoming itself, since our command-line is supposed
to be stable, right?
Also, Juan has said that replacing that arg with a monitor command
doesn't work, as qemu would have to be started in paused monitor for this
to work.
So, what about introducing a -incoming-monitor command, which puts qemu
in the right state for migration, but requires a migrate_incoming command
to actually start migration?
[Qemu-devel] Re: [PATCH 3/5] QMP: Introduce MIGRATION events
On Thu, 27 May 2010 17:58:03 +0200
Juan Quintela quint...@redhat.com wrote:
Luiz Capitulino lcapitul...@redhat.com wrote:
On Wed, 26 May 2010 11:55:31 -0500
Anthony Liguori anth...@codemonkey.ws wrote:
That's exactly how the protocol is designed. That was one of the major
improvements of QMP over the human monior.
Yes and it already has 'id' support:
{ execute: cont, id: luiz }
{timestamp: {seconds: 1274966635, microseconds: 776813}, event:
RESUME}
{return: {}, id: luiz}
But it doesn't detect duplicates, this is something I think it's up
to the client to do, do you agree?
This is how the info balloon command works, BTW.
I won't remember the details now, but that interface has some issues and it
has to be reviewed.
Since there's a clear correlation between the request and the result of
the request, an asynchronous command is what makes the most sense. It
eliminates the problem of how to pass QErrors via an event which is one
of the problems with the current event proposal.
Not exactly, this is a problem with QError not the event proposal. We'll
have the same issue if we decide to include errno in the migrate errors and
the problem still exists with the BLOCK_IO_ERROR event.
That said, I do agree that migrate should be asynchronous. This yet another
thing we may want to fix before 0.13.
How difficult is that?
Anthony is working on this and should have patches soon.
[...]
Yes. And by deprecate, I really mean that -incoming just becomes
syntactic sugar for executing a monitor command immediately.
But we can't change -incoming itself, since our command-line is supposed
to be stable, right?
Also, Juan has said that replacing that arg with a monitor command
doesn't work, as qemu would have to be started in paused monitor for this
to work.
So, what about introducing a -incoming-monitor command, which puts qemu
in the right state for migration, but requires a migrate_incoming command
to actually start migration?
this -incoming-monitor is called -S, that should have a long name of
-no-autostart
that is what it does, and what we need for incoming migration as monitor
command. Nothing new to see here.
Ok, I thought -S alone wasn't enough but if it's, let's go for it then.
[Qemu-devel] Re: [PATCH v4 0/10] Introduce 'info netdevices' with QMP support
On Tue, 18 May 2010 14:07:39 -0300 Miguel Di Ciurcio Filho miguel.fi...@gmail.com wrote: The VLANClientState structure has the member info_str, a simple string that is filled with information about NIC devices and used on monitor calls. There is no coherent formatting of this string by all the NIC devices, making it difficult to parse and represent this information over QMP. To make the transition to QMP safe, we leave the current 'info network' command as is, and introduce a new one: 'info netdevices'. Sorry for the long delay in reviewing this series.. Most important comment is: QMP development process is changing and now it's required that any protocol change updates the qemu-monitor.hx file with QMP info. However, it's not merged yet so you could base your series on top of this series, which is also in my master branch: http://lists.gnu.org/archive/html/qemu-devel/2010-05/msg01605.html Another option is to send the doc patch separately w/o any code and only go back to code when there's consensus wrt new command. Small comments on separate patches follow.
[Qemu-devel] Re: [PATCH v4 01/10] QObject API: introduce qdict_to_qstring() function
On Tue, 18 May 2010 14:07:40 -0300
Miguel Di Ciurcio Filho miguel.fi...@gmail.com wrote:
This is a helper function that converts a QDict to a QString, using
the format:
key1=value1 SEP key2=value2 SEP key3=value3
Handy for debugging and formating the Monitor output.
Signed-off-by: Miguel Di Ciurcio Filho miguel.fi...@gmail.com
It's very better now, some comments below.
---
qdict.c | 55 +++
qdict.h |9 +
2 files changed, 64 insertions(+), 0 deletions(-)
diff --git a/qdict.c b/qdict.c
index 175bc17..19c053f 100644
--- a/qdict.c
+++ b/qdict.c
@@ -267,6 +267,61 @@ const char *qdict_get_str(const QDict *qdict, const char
*key)
return qstring_get_str(qobject_to_qstring(obj));
}
+static void qdict_to_qstring_iter(const char *key, QObject *obj, void
*opaque)
+{
+struct qstring_pack *pack = opaque;
+qstring_append(pack-str, key);
+qstring_append(pack-str, =);
+switch (qobject_type(obj)) {
+case QTYPE_QSTRING:
+qstring_append(pack-str, qstring_get_str(qobject_to_qstring(obj)));
+break;
+case QTYPE_QINT:
+qstring_append_int(pack-str, qint_get_int(qobject_to_qint(obj)));
+break;
+case QTYPE_QBOOL:
+qstring_append(pack-str, qbool_get_int(qobject_to_qbool(obj)) ?
true :
+false );
+break;
+default:
+qstring_append(pack-str, NULL);
+}
+
+pack-qdict_iter_current_key++;
+
+if (pack-qdict_iter_current_key pack-qdict_iter_total_keys) {
+qstring_append(pack-str, pack-separator);
+}
+}
+
+/**
+ * qdict_to_qstring(): Format a string with the keys and values of a QDict.
+ *
+ * Nested lists and dicts are not supported, yet.
+ *
+ * Return a pointer to a QString, with the following format:
+ *key1=value1 SEP key2=value2 SEP key3=value3
+ */
+QString *qdict_to_qstring(const QDict *qdict, const char *separator)
+{
+struct qstring_pack *pack;
+QString *str;
+str = qstring_new();
+
+pack = qemu_malloc(sizeof(*pack));
You can allocate on the stack.
+pack-str = str;
+pack-qdict_iter_current_key = 0;
+pack-qdict_iter_total_keys = qdict_size(qdict);
+pack-separator = separator;
+
+qdict_iter(qdict, qdict_to_qstring_iter, pack);
+
+qemu_free(pack);
+
+return str;
+}
+
+
/**
* qdict_get_try_int(): Try to get integer mapped by 'key'
*
diff --git a/qdict.h b/qdict.h
index 5e5902c..8a54733 100644
--- a/qdict.h
+++ b/qdict.h
@@ -15,6 +15,7 @@
#include qobject.h
#include qlist.h
+#include qstring.h
#include qemu-queue.h
#include stdint.h
@@ -32,6 +33,13 @@ typedef struct QDict {
QLIST_HEAD(,QDictEntry) table[QDICT_HASH_SIZE];
} QDict;
+struct qstring_pack {
+QString *str;
+size_t qdict_iter_total_keys;
+size_t qdict_iter_current_key;
+const char *separator;
+};
The header file should contain only public interfaces, qstring_pack is
something internal to qdict.c so please move this right above
qdict_to_qstring_iter() also suggest shorter names like 'total_keys'.
+
/* Object API */
QDict *qdict_new(void);
size_t qdict_size(const QDict *qdict);
@@ -55,6 +63,7 @@ int qdict_get_bool(const QDict *qdict, const char *key);
QList *qdict_get_qlist(const QDict *qdict, const char *key);
QDict *qdict_get_qdict(const QDict *qdict, const char *key);
const char *qdict_get_str(const QDict *qdict, const char *key);
+QString *qdict_to_qstring(const QDict *qdict, const char *separator);
int64_t qdict_get_try_int(const QDict *qdict, const char *key,
int64_t err_value);
const char *qdict_get_try_str(const QDict *qdict, const char *key);
[Qemu-devel] Re: [PATCH v4 05/10] net: tap/tap-win32: introduce info_dict
On Tue, 18 May 2010 14:07:44 -0300
Miguel Di Ciurcio Filho miguel.fi...@gmail.com wrote:
Signed-off-by: Miguel Di Ciurcio Filho miguel.fi...@gmail.com
---
net/tap-win32.c |9 -
net/tap.c | 18 +-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 74348da..54c6725 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -32,6 +32,8 @@
#include net.h
#include sysemu.h
#include qemu-error.h
+#include qdict.h
+#include qstring.h
#include stdio.h
#include windows.h
#include winioctl.h
@@ -691,7 +693,12 @@ static int tap_win32_init(VLANState *vlan, const char
*model,
s = DO_UPCAST(TAPState, nc, nc);
snprintf(s-nc.info_str, sizeof(s-nc.info_str),
- tap: ifname=%s, ifname);
+tap: ifname=%s, ifname);
Did you change this by accident?
+
+nc-info_dict = qdict_new()
+
+qdict_put(nc-info_dict, ifname, qstring_from_str(ifname));
+qdict_put(nc-info_dict, model, qstring_from_str(tap));
So, this is the kind of thing that deserves attention when documenting,
we need to make it clear in the doc what's each format we support, ie.
xen, tap, vde, dump, slirp and socket.
s-handle = handle;
diff --git a/net/tap.c b/net/tap.c
index 0147dab..d71f312 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -39,6 +39,9 @@
#include qemu-char.h
#include qemu-common.h
#include qemu-error.h
+#include qjson.h
+#include qdict.h
+#include qint.h
#include net/tap-linux.h
@@ -448,17 +451,30 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const
char *name, VLANState *vlan
if (qemu_opt_get(opts, fd)) {
snprintf(s-nc.info_str, sizeof(s-nc.info_str), fd=%d, fd);
+assert(s-nc.info_dict == NULL);
+
+s-nc.info_dict = qdict_new();
+qdict_put(s-nc.info_dict, fd, qint_from_int(fd));
+
} else {
const char *ifname, *script, *downscript;
+QObject *obj;
ifname = qemu_opt_get(opts, ifname);
script = qemu_opt_get(opts, script);
downscript = qemu_opt_get(opts, downscript);
snprintf(s-nc.info_str, sizeof(s-nc.info_str),
- ifname=%s,script=%s,downscript=%s,
+ifname=%s,script=%s,downscript=%s,
Another accidental change, I think.
ifname, script, downscript);
+obj = qobject_from_jsonf({ 'model': 'tap', 'ifname': %s, \
+'script': %s,'downscript': %s },
+ifname, script, downscript);
+
+assert(s-nc.info_dict == NULL);
+s-nc.info_dict = qobject_to_qdict(obj);
+
if (strcmp(downscript, no) != 0) {
snprintf(s-down_script, sizeof(s-down_script), %s,
downscript);
snprintf(s-down_script_arg, sizeof(s-down_script_arg), %s,
ifname);
[Qemu-devel] Re: [PATCH v4 06/10] net: vde: introduce info_dict
On Tue, 18 May 2010 14:07:45 -0300
Miguel Di Ciurcio Filho miguel.fi...@gmail.com wrote:
Signed-off-by: Miguel Di Ciurcio Filho miguel.fi...@gmail.com
---
net/vde.c | 11 ++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/net/vde.c b/net/vde.c
index 0b46fa6..6a3d0ba 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -31,6 +31,9 @@
#include qemu-char.h
#include qemu-common.h
#include qemu-option.h
+#include qdict.h
+#include qstring.h
+#include qint.h
#include sysemu.h
typedef struct VDEState {
@@ -100,7 +103,13 @@ static int net_vde_init(VLANState *vlan, const char
*model,
nc = qemu_new_net_client(net_vde_info, vlan, NULL, model, name);
snprintf(nc-info_str, sizeof(nc-info_str), sock=%s,fd=%d,
- sock, vde_datafd(vde));
+sock, vde_datafd(vde));
This change is not needed.
+
+
+nc-info_dict = qdict_new();
+qdict_put(nc-info_dict, sock, qstring_from_str(sock));
+qdict_put(nc-info_dict, model, qstring_from_str(vde));
+qdict_put(nc-info_dict, fd, qint_from_int(vde_datafd(vde)));
s = DO_UPCAST(VDEState, nc, nc);
[Qemu-devel] Re: [PATCH] Add dependency of JSON unit tests on config-host.h
On Thu, 20 May 2010 09:18:52 +0200 Jan Kiszka jan.kis...@web.de wrote: From: Jan Kiszka jan.kis...@siemens.com Signed-off-by: Jan Kiszka jan.kis...@siemens.com Looks good. --- Makefile |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/Makefile b/Makefile index 110698e..aa81d9b 100644 --- a/Makefile +++ b/Makefile @@ -144,6 +144,8 @@ qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobj qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx $(call quiet-command,sh $(SRC_PATH)/hxtool -h $ $@, GEN $@) +check-qint.o check-qstring.o check-qdict.o check-qlist.o check-qfloat.o check-qjson.o: $(GENERATED_HEADERS) + check-qint: check-qint.o qint.o qemu-malloc.o check-qstring: check-qstring.o qstring.o qemu-malloc.o check-qdict: check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qemu-malloc.o qlist.o
[Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del
On Sun, 23 May 2010 12:59:19 +0200
Jan Kiszka jan.kis...@web.de wrote:
From: Jan Kiszka jan.kis...@siemens.com
Allow to specify the device to be removed via device_del not only by ID
but also by its full or abbreviated qtree path. For this purpose,
qdev_find is introduced which combines walking the qtree with searching
for device IDs if required.
[...]
Arguments:
-- id: the device's ID (json-string)
+- path: the device's qtree path or unique ID (json-string)
Example:
-- { execute: device_del, arguments: { id: net1 } }
+- { execute: device_del, arguments: { path: net1 } }
Doesn't seem like a good change to me, besides being incompatible[1] we
shouldn't overload arguments this way in QMP as overloading leads to
interface degradation (harder to use, understand, maintain).
Maybe we could have both arguments as optional, but one must be passed.
[1] It's 'legal' to break the protocol before 0.13, but this has to be
coordinated with libvirt so we should have a good reason to do this
- { return: {} }
EQMP
[Qemu-devel] Re: [PATCH v3 10/17] QMP: Reserve namespace for complex object classes
On Sun, 23 May 2010 12:59:23 +0200 Jan Kiszka jan.kis...@web.de wrote: From: Jan Kiszka jan.kis...@siemens.com This reserves JSON objects that contain the key '__class__' for QMP-specific complex objects. First user will be the buffer class. Signed-off-by: Jan Kiszka jan.kis...@siemens.com --- QMP/qmp-spec.txt | 16 +--- 1 files changed, 13 insertions(+), 3 deletions(-) diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt index 9d30a8c..fa1dd62 100644 --- a/QMP/qmp-spec.txt +++ b/QMP/qmp-spec.txt @@ -146,6 +146,15 @@ The format is: For a listing of supported asynchronous events, please, refer to the qmp-events.txt file. +2.6 Complex object classes +-- + +JSON objects that contain the key-value pair '__class__: json-string' are I'm not strong about this, but it's better to call it just a 'pair', as 'value' is a bit problematic because of json-value. +reserved for QMP-specific complex object classes that. QMP specifies which Early full stop? +further keys each of these objects include and how they are encoded. + +So far, no complex object class is specified. + 3. QMP Examples === @@ -229,9 +238,10 @@ avoid modifying QMP. Both upstream and downstream need to take care to preserve long-term compatibility and interoperability. To help with that, QMP reserves JSON object member names beginning with -'__' (double underscore) for downstream use (downstream names). This -means upstream will never use any downstream names for its commands, -arguments, errors, asynchronous events, and so forth. +'__' (double underscore) for downstream use (downstream names). Downstream +names MUST NOT end with '__' as this pattern is reserved for QMP-defined JSON +object classes. Upstream will never use any downstream names for its +commands, arguments, errors, asynchronous events, and so forth. Suggest mentioning subsection 2.6. Any new names downstream wishes to add must begin with '__'. To ensure compatibility with other downstreams, it is strongly
[Qemu-devel] Re: [PATCH v3 13/17] monitor: Allow to exclude commands from QMP
On Sun, 23 May 2010 12:59:26 +0200
Jan Kiszka jan.kis...@web.de wrote:
From: Jan Kiszka jan.kis...@siemens.com
Ported commands that are marked 'user_only' will not be considered for
QMP monitor sessions. This allows to implement new commands that do not
(yet) provide a sufficiently stable interface for QMP use (e.g.
device_show).
This is fine for me, but two things I've been wondering:
1. Isn't a 'flags' struct member better? So that we can do (in the
qemu-monitor.hx entry):
.flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,
I'm not suggesting this is an async handler, just exemplifying multiple
flags.
2. Getting QMP handlers right in the first time might be difficult, so
we could have a way to mark them unstable. Maybe a different namespace
which is only enabled at configure time with:
--enable-qmp-unstable-commands
If this were possible, we could have device_show and any command we
aren't sure is QMP-ready working in QMP this way.
Signed-off-by: Jan Kiszka jan.kis...@siemens.com
---
monitor.c | 13 ++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/monitor.c b/monitor.c
index 6766e49..5768c6e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -114,6 +114,7 @@ typedef struct mon_cmd_t {
MonitorCompletion *cb, void *opaque);
} mhandler;
int async;
+bool user_only;
} mon_cmd_t;
/* file descriptors passed via SCM_RIGHTS */
@@ -635,6 +636,11 @@ static int do_info(Monitor *mon, const QDict *qdict,
QObject **ret_data)
goto help;
}
+if (monitor_ctrl_mode(mon) cmd-user_only) {
+qerror_report(QERR_COMMAND_NOT_FOUND, item);
+return -1;
+}
+
if (monitor_handler_is_async(cmd)) {
if (monitor_ctrl_mode(mon)) {
qmp_async_info_handler(mon, cmd);
@@ -732,13 +738,14 @@ static void do_info_commands(Monitor *mon, QObject
**ret_data)
cmd_list = qlist_new();
for (cmd = mon_cmds; cmd-name != NULL; cmd++) {
-if (monitor_handler_ported(cmd) !compare_cmd(cmd-name, info)) {
+if (monitor_handler_ported(cmd) !cmd-user_only
+!compare_cmd(cmd-name, info)) {
qlist_append_obj(cmd_list, get_cmd_dict(cmd-name));
}
}
for (cmd = info_cmds; cmd-name != NULL; cmd++) {
-if (monitor_handler_ported(cmd)) {
+if (monitor_handler_ported(cmd) !cmd-user_only) {
char buf[128];
snprintf(buf, sizeof(buf), query-%s, cmd-name);
qlist_append_obj(cmd_list, get_cmd_dict(buf));
@@ -4416,7 +4423,7 @@ static void handle_qmp_command(JSONMessageParser
*parser, QList *tokens)
qobject_from_jsonf({ 'item': %s }, info_item));
} else {
cmd = monitor_find_command(cmd_name);
-if (!cmd || !monitor_handler_ported(cmd)) {
+if (!cmd || !monitor_handler_ported(cmd) || cmd-user_only) {
qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
goto err_input;
}
[Qemu-devel] Re: [PATCH v3 16/17] QMP: Fix python helper /wrt long return strings
On Sun, 23 May 2010 12:59:29 +0200 Jan Kiszka jan.kis...@web.de wrote: From: Jan Kiszka jan.kis...@siemens.com Remove the arbitrary limitation of 1024 characters per return string and read complete lines instead. Required for device_show. Thanks for both fixes, I have started working on a better version of this script that mimics better the user monitor but it's only half done. Signed-off-by: Jan Kiszka jan.kis...@siemens.com --- QMP/qmp.py |6 +- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/QMP/qmp.py b/QMP/qmp.py index d9da603..4062f84 100644 --- a/QMP/qmp.py +++ b/QMP/qmp.py @@ -63,10 +63,14 @@ class QEMUMonitorProtocol: def __json_read(self): try: -return json.loads(self.sock.recv(1024)) +while True: +line = json.loads(self.sockfile.readline()) +if not 'event' in line: +return line except ValueError: return def __init__(self, filename): self.filename = filename self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) +self.sockfile = self.sock.makefile()
[Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del
On Fri, 28 May 2010 00:19:48 +0200
Jan Kiszka jan.kis...@web.de wrote:
Luiz Capitulino wrote:
On Sun, 23 May 2010 12:59:19 +0200
Jan Kiszka jan.kis...@web.de wrote:
From: Jan Kiszka jan.kis...@siemens.com
Allow to specify the device to be removed via device_del not only by ID
but also by its full or abbreviated qtree path. For this purpose,
qdev_find is introduced which combines walking the qtree with searching
for device IDs if required.
[...]
Arguments:
-- id: the device's ID (json-string)
+- path: the device's qtree path or unique ID (json-string)
Example:
-- { execute: device_del, arguments: { id: net1 } }
+- { execute: device_del, arguments: { path: net1 } }
Doesn't seem like a good change to me, besides being incompatible[1] we
shouldn't overload arguments this way in QMP as overloading leads to
interface degradation (harder to use, understand, maintain).
It's not overloaded, think of an ID as a (weak) symbolic link in the
qtree filesystem. The advantage of basing everything on top of full or
abbreviated qtree paths is that IDs are not always assigned, paths are.
You mean there're cases where an ID is not assigned? I didn't know that,
I thought they were always assigned, at least via QMP.
In any case, my main question here is that if this change really makes
sense for QMP or if it's something for HMP where we can have features
like tab completion.
Maybe we could have both arguments as optional, but one must be passed.
This would at least require some way to keep the proposed unified path
specification for the human monitor (having separate arguments there is
really unhandy).
Agreed, perhaps we have to decouple QMP and HMP in the way proposed by
Anthony: the HMP should work by making QMP calls (IIUC).
This way HMP can accept whatever arguments make sense for it, but then
it should transform them in a QMP call.
This is a long term project though.
[Qemu-devel] Re: [PATCH v3 13/17] monitor: Allow to exclude commands from QMP
On Fri, 28 May 2010 00:20:08 +0200 Jan Kiszka jan.kis...@web.de wrote: Luiz Capitulino wrote: On Sun, 23 May 2010 12:59:26 +0200 Jan Kiszka jan.kis...@web.de wrote: From: Jan Kiszka jan.kis...@siemens.com Ported commands that are marked 'user_only' will not be considered for QMP monitor sessions. This allows to implement new commands that do not (yet) provide a sufficiently stable interface for QMP use (e.g. device_show). This is fine for me, but two things I've been wondering: 1. Isn't a 'flags' struct member better? So that we can do (in the qemu-monitor.hx entry): .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC, I'm not suggesting this is an async handler, just exemplifying multiple flags. Yes, can refactor this. 2. Getting QMP handlers right in the first time might be difficult, so we could have a way to mark them unstable. Maybe a different namespace which is only enabled at configure time with: --enable-qmp-unstable-commands If this were possible, we could have device_show and any command we aren't sure is QMP-ready working in QMP this way. Do you suggest this as an alternative to this patch? Or an extension later on? I have no opinion on this yet, I would just like to know how to proceed for this series. Both can be done worked later, as this is internal there's no problem in living with a simpler solution for a while.
Re: [Qemu-devel] Re: RFC: blockdev_add friends, brief rationale, QMP docs
On Fri, 28 May 2010 14:17:07 -0500
Anthony Liguori anth...@codemonkey.ws wrote:
On 05/28/2010 02:13 PM, Kevin Wolf wrote:
Am 28.05.2010 20:21, schrieb Markus Armbruster:
I'd like to give posting documentation of new QMP commands for review
before posting code a try. But first let me explain briefly why we need
new commands.
We want a clean separation between host part (blockdev_add) and guest
part (device_add). Existing -drive and drive_add don't provide that;
they were designed to specify both parts together. Moreover, drive_add
is limited to adding virtio drives (with pci_add's help) and SCSI
drives.
Support for defining just a host part for use with -device and was
grafted onto -drive (if=none), but it's a mess. Some parts are
redundant, other parts are broken.
For instance, unit, bus, index, addr are redundant: -device does not use
them, it provides its own parameters to specify bus and bus-specific
address.
The checks whether rerror, werror, readonly, cyls, heads, secs are sane
for a particular guest driver are broken. The checks are in the -drive
code, which used to know the guest driver, but doesn't with if=none.
Additionally, removable media are flawed. Many parameters set with
-drive silently revert to defaults on media change.
My proposed solution is a new option -blockdev and monitor command
blockdev_add. These specify only the host drive. Guest drive
properties are left to -device / device_add. We keep -drive for
backwards compatibility and command line convenience. Except we get rid
of if=none (may need a grace period).
New monitor command blockdev_del works regardless of how the host block
device was created.
New monitor command blockdev_change provides full control over the host
part, unlike the existing change command.
Summary of the host / guest split:
-drive options host or guest?
bus, unit, if, index, addr guest, already covered by qdev
cyls, heads, secs, transguest, new qdev properties
(but defaults depend on image)
media guest
snapshot, file, cache, aio, format host, blockdev_add options
rerror, werror host, guest drivers will reject
values they don't support
serial guest, new qdev properties
readonlyboth host guest, qdev will refuse
to connect readonly host to read/
write guest
QMP command docs:
blockdev_add
Add host block device.
Arguments:
- id: the host block device's ID, must be unique (json-string)
- file: the disk image file to use (json-string, optional)
- format: disk format (json-string, optional)
- Possible values: raw, qcow2, ...
- aio: host AIO (json-string, optional)
- Possible values: threads (default), native
- cache: host cache usage (json-string, optional)
- Possible values: writethrough (default), writeback, unsafe,
none
- readonly: open image read-only (json-bool, optional, default false)
- rerror: what to do on read error (json-string, optional)
- Possible values: report (default), ignore, stop
- werror: what to do on write error (json-string, optional)
- Possible values: enospc (default), report, ignore, stop
- snapshot: enable snapshot (json-bool, optional, default false)
Example:
- { execute: blockdev_add,
arguments: { format: raw, id: blk1,
file: fedora.img } }
- { return: {} }
Notes:
(1) If argument file is missing, all other optional arguments must be
missing as well. This defines a block device with no media
inserted.
(2) It's possible to list supported disk formats by running QEMU with
arguments -blockdev_add \?.
-blockdev without _add you probably mean, if it's a command line option.
Maybe one more thing to consider is encrypted images. With change in
the user monitor you're automatically prompted for the password.
I'm not sure how this is supposed to work with QMP. From the
do_change_block code it looks as if you'd get an error and had to send a
block_set_passwd as a response to that. In the meantime the image would
be kind of half-open? What do devices do with it until the key is provided?
If a password is needed, we should throw an error and let the QMP client
set the password and try again.
It's what we do today, a password should be set with block_passwd before
issuing the change command. Otherwise an error is throw.
Regards,
Anthony Liguori
Would it make s1ense to add a password field to blockdev_add/change to
avoid this?
Kevin
Re: [Qemu-devel] Re: RFC: blockdev_add friends, brief rationale, QMP docs
On Mon, 31 May 2010 13:05:37 +0200 Markus Armbruster arm...@redhat.com wrote: Avi Kivity a...@redhat.com writes: On 05/28/2010 10:24 PM, Luiz Capitulino wrote: If a password is needed, we should throw an error and let the QMP client set the password and try again. It's what we do today, a password should be set with block_passwd before issuing the change command. Otherwise an error is throw. Is the password some kind of global or per-monitor property? In that case it doesn't work with parallel execution of commands; better to have a password field (or assign IDs to passwords and require a passwordid=... argument). It sets the password in the host BlockDriverState. Which must already exist, i.e. you do it after blockdev_add. What happens if the guest device accesses the host drive before the key is set? It's supposed to fail, right Kevin? Anything wrong with passing the password as argument? Did we avoid that to protect naive users from exposing their password via argv[]? That argument doesn't apply to QMP.
[Qemu-devel] Re: [PATCH v3 0/3]: QMP: Commands doc
On Sat, 29 May 2010 10:50:55 +0200 Jan Kiszka jan.kis...@web.de wrote: Luiz Capitulino wrote: This new version moves the documentation to qemu-monitor.hx and now QMP/qmp-commands.txt is generated from there (thanks Jan!). I hope I've addressed all review comments in this version and now it should describe reality. Next step is to fix glitches (after this series is merged, of course). This is a fragile series, breaking quickly when something changes in qemu-monitor.hx (as just happened). Can we get this rebased and merged ASAP? Will rebase and resend later today.
[Qemu-devel] [PATCH 1/3] monitor: Reorder info documentation
From: Jan Kiszka jan.kis...@siemens.com
Push the doc fragments for the info command to the end of
qemu-monitor.hx. This helps to establish a proper layout in the upcoming
QMP documentation.
Signed-off-by: Jan Kiszka jan.kis...@siemens.com
---
qemu-monitor.hx | 186 --
1 files changed, 96 insertions(+), 90 deletions(-)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index f4f88df..b8c765b 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -38,96 +38,6 @@ Commit changes to the disk images (if -snapshot is used) or
backing files.
ETEXI
{
-.name = info,
-.args_type = item:s?,
-.params = [subcommand],
-.help = show various information about the system state,
-.user_print = monitor_user_noop,
-.mhandler.cmd_new = do_info,
-},
-
-STEXI
-...@item info @var{subcommand}
-...@findex info
-Show various information about the system state.
-
-...@table @option
-...@item info version
-show the version of QEMU
-...@item info commands
-list QMP available commands
-...@item info network
-show the various VLANs and the associated devices
-...@item info chardev
-show the character devices
-...@item info block
-show the block devices
-...@item info blockstats
-show block device statistics
-...@item info registers
-show the cpu registers
-...@item info cpus
-show infos for each CPU
-...@item info history
-show the command line history
-...@item info irq
-show the interrupts statistics (if available)
-...@item info pic
-show i8259 (PIC) state
-...@item info pci
-show emulated PCI device info
-...@item info tlb
-show virtual to physical memory mappings (i386 only)
-...@item info mem
-show the active virtual memory mappings (i386 only)
-...@item info hpet
-show state of HPET (i386 only)
-...@item info jit
-show dynamic compiler info
-...@item info kvm
-show KVM information
-...@item info numa
-show NUMA information
-...@item info usb
-show USB devices plugged on the virtual USB hub
-...@item info usbhost
-show all USB host devices
-...@item info profile
-show profiling information
-...@item info capture
-show information about active capturing
-...@item info snapshots
-show list of VM snapshots
-...@item info status
-show the current VM status (running|paused)
-...@item info pcmcia
-show guest PCMCIA status
-...@item info mice
-show which guest mouse is receiving events
-...@item info vnc
-show the vnc server status
-...@item info name
-show the current VM name
-...@item info uuid
-show the current VM UUID
-...@item info cpustats
-show CPU statistics
-...@item info usernet
-show user network stack connection states
-...@item info migrate
-show migration status
-...@item info balloon
-show balloon information
-...@item info qtree
-show device tree
-...@item info qdm
-show qdev device model list
-...@item info roms
-show roms
-...@end table
-ETEXI
-
-{
.name = q|quit,
.args_type = ,
.params = ,
@@ -1190,6 +1100,102 @@ STEXI
Enable the specified QMP capabilities
ETEXI
+
+HXCOMM Keep the 'info' command at the end!
+HXCOMM This is required for the QMP documentation layout.
+
+{
+.name = info,
+.args_type = item:s?,
+.params = [subcommand],
+.help = show various information about the system state,
+.user_print = monitor_user_noop,
+.mhandler.cmd_new = do_info,
+},
+
+STEXI
+...@item info @var{subcommand}
+...@findex info
+Show various information about the system state.
+
+...@table @option
+...@item info version
+show the version of QEMU
+...@item info commands
+list QMP available commands
+...@item info network
+show the various VLANs and the associated devices
+...@item info chardev
+show the character devices
+...@item info block
+show the block devices
+...@item info blockstats
+show block device statistics
+...@item info registers
+show the cpu registers
+...@item info cpus
+show infos for each CPU
+...@item info history
+show the command line history
+...@item info irq
+show the interrupts statistics (if available)
+...@item info pic
+show i8259 (PIC) state
+...@item info pci
+show emulated PCI device info
+...@item info tlb
+show virtual to physical memory mappings (i386 only)
+...@item info mem
+show the active virtual memory mappings (i386 only)
+...@item info hpet
+show state of HPET (i386 only)
+...@item info jit
+show dynamic compiler info
+...@item info kvm
+show KVM information
+...@item info numa
+show NUMA information
+...@item info usb
+show USB devices plugged on the virtual USB hub
+...@item info usbhost
+show all USB host devices
+...@item info profile
+show profiling information
+...@item info capture
+show information about active capturing
+...@item info snapshots
+show list of VM snapshots
+...@item info status
+show the current VM status (running|paused)
+...@item info pcmcia
+show guest PCMCIA status
+...@item info mice
+show which guest mouse is receiving
[Qemu-devel] [PATCH v4 0/3]: QMP: Commands doc
Rebase.. Let's try to merge this ASAP, please. changelog - v3 - v4 - Rebased - Minor commit log clarification v2 - v3 - Rebased - Addressed review comments - Move contents to qemu-monitor.hx (Jan) v1 - v2 - Rebased - Addressed Markus's comments - Changed indentation style - Minor fixes and clarifications
[Qemu-devel] [PATCH 2/3] QMP: Introduce commands documentation
From: Jan Kiszka jan.kis...@siemens.com
One of the most important missing feature in QMP today is its
supported commands documentation.
The plan is to make it part of self-description support, however
self-description is a big task we have been postponing for a
long time now and still don't know when it's going to be done.
In order not to compromise QMP adoption and make users' life easier,
this commit adds a simple text documentation which fully describes
all QMP supported commands.
This is not ideal for a number of reasons (harder to maintain,
text-only, etc) but does improve the current situation. To avoid at
least divering from the user monitor help and texi snippets, QMP bits
are also maintained inside qemu-monitor.hx, and hxtool is extended to
generate a single text file from them.
Signed-off-by: Jan Kiszka jan.kis...@siemens.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
Makefile|5 +-
QMP/README |5 +-
configure |4 +
hxtool | 44 ++-
qemu-monitor.hx | 1322 +++
5 files changed, 1376 insertions(+), 4 deletions(-)
diff --git a/Makefile b/Makefile
index 7986bf6..3a8a311 100644
--- a/Makefile
+++ b/Makefile
@@ -29,7 +29,7 @@ $(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)
LIBS+=-lz $(LIBS_TOOLS)
ifdef BUILD_DOCS
-DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8
+DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8
QMP/qmp-commands.txt
else
DOCS=
endif
@@ -259,6 +259,9 @@ qemu-options.texi: $(SRC_PATH)/qemu-options.hx
qemu-monitor.texi: $(SRC_PATH)/qemu-monitor.hx
$(call quiet-command,sh $(SRC_PATH)/hxtool -t $ $@, GEN $@)
+QMP/qmp-commands.txt: $(SRC_PATH)/qemu-monitor.hx
+ $(call quiet-command,sh $(SRC_PATH)/hxtool -q $ $@, GEN $@)
+
qemu-img-cmds.texi: $(SRC_PATH)/qemu-img-cmds.hx
$(call quiet-command,sh $(SRC_PATH)/hxtool -t $ $@, GEN $@)
diff --git a/QMP/README b/QMP/README
index 9334c25..35a80c7 100644
--- a/QMP/README
+++ b/QMP/README
@@ -15,8 +15,9 @@ QMP is JSON[1] based and has the following features:
For more information, please, refer to the following files:
-o qmp-spec.txtQEMU Monitor Protocol current specification
-o qmp-events.txt List of available asynchronous events
+o qmp-spec.txt QEMU Monitor Protocol current specification
+o qmp-commands.txt QMP supported commands
+o qmp-events.txtList of available asynchronous events
There are also two simple Python scripts available:
diff --git a/configure b/configure
index 3cd2c5f..653c8d2 100755
--- a/configure
+++ b/configure
@@ -2817,3 +2817,7 @@ ln -s $source_path/Makefile.user $d/Makefile
if test $static = no -a $user_pie = yes ; then
echo QEMU_CFLAGS+=-fpie $d/config.mak
fi
+
+if test $docs = yes ; then
+ mkdir -p QMP
+fi
diff --git a/hxtool b/hxtool
index 8f65532..d499dc0 100644
--- a/hxtool
+++ b/hxtool
@@ -7,7 +7,7 @@ hxtoh()
case $str in
HXCOMM*)
;;
-STEXI*|ETEXI*) flag=$(($flag^1))
+STEXI*|ETEXI*|SQMP*|EQMP*) flag=$(($flag^1))
;;
*)
test $flag -eq 1 printf %s\n $str
@@ -38,6 +38,12 @@ hxtotexi()
fi
flag=0
;;
+SQMP*|EQMP*)
+if test $flag -eq 1 ; then
+echo line $line: syntax error: expected ETEXI, found $str 2
+exit 1
+fi
+;;
DEFHEADING*)
echo $(expr $str : DEFHEADING(\(.*\)))
;;
@@ -49,9 +55,45 @@ hxtotexi()
done
}
+hxtoqmp()
+{
+IFS=
+flag=0
+while read -r str; do
+case $str in
+HXCOMM*)
+;;
+SQMP*)
+if test $flag -eq 1 ; then
+echo line $line: syntax error: expected EQMP, found $str 2
+exit 1
+fi
+flag=1
+;;
+EQMP*)
+if test $flag -ne 1 ; then
+echo line $line: syntax error: expected SQMP, found $str 2
+exit 1
+fi
+flag=0
+;;
+STEXI*|ETEXI*)
+if test $flag -eq 1 ; then
+echo line $line: syntax error: expected EQMP, found $str 2
+exit 1
+fi
+;;
+*)
+test $flag -eq 1 echo $str
+;;
+esac
+done
+}
+
case $1 in
-h) hxtoh ;;
-t) hxtotexi ;;
+-q) hxtoqmp ;;
*) exit 1 ;;
esac
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index b8c765b..f6a94f2 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1,10 +1,48 @@
HXCOMM Use DEFHEADING() to define headings in both help text and texi
HXCOMM Text between STEXI and ETEXI are copied to texi version and
HXCOMM discarded from C version
+HXCOMM Text between SQMP and EQMP is copied to the QMP documention file and
+HXCOMM does not show up in the other formats
[Qemu-devel] [PATCH 3/3] Monitor: Drop QMP documentation from code
Previous commit added QMP documentation to the qemu-monitor.hx
file, it's is a copy of this information.
While it's good to keep it near code, maintaining two copies of
the same information is too hard and has little benefit as we
don't expect client writers to consult the code to find how to
use a QMP command.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
block.c | 69
hw/pci.c| 61 ---
hw/qdev.c | 13 ---
input.c | 18 --
migration.c | 38 --
monitor.c | 102 ---
net.c | 22 -
qemu-char.c | 16 -
vnc.c | 29 -
9 files changed, 0 insertions(+), 368 deletions(-)
diff --git a/block.c b/block.c
index cd70730..67f0998 100644
--- a/block.c
+++ b/block.c
@@ -1444,33 +1444,6 @@ void bdrv_info_print(Monitor *mon, const QObject *data)
qlist_iter(qobject_to_qlist(data), bdrv_print_dict, mon);
}
-/**
- * bdrv_info(): Block devices information
- *
- * Each block device information is stored in a QDict and the
- * returned QObject is a QList of all devices.
- *
- * The QDict contains the following:
- *
- * - device: device name
- * - type: device type
- * - removable: true if the device is removable, false otherwise
- * - locked: true if the device is locked, false otherwise
- * - inserted: only present if the device is inserted, it is a QDict
- *containing the following:
- * - file: device file name
- * - ro: true if read-only, false otherwise
- * - drv: driver format name
- * - backing_file: backing file name if one is used
- * - encrypted: true if encrypted, false otherwise
- *
- * Example:
- *
- * [ { device: ide0-hd0, type: hd, removable: false, locked: false,
- * inserted: { file: /tmp/foobar, ro: false, drv: qcow2 } },
- * { device: floppy0, type: floppy, removable: true,
- * locked: false } ]
- */
void bdrv_info(Monitor *mon, QObject **ret_data)
{
QList *bs_list;
@@ -1576,48 +1549,6 @@ static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
return res;
}
-/**
- * bdrv_info_stats(): show block device statistics
- *
- * Each device statistic information is stored in a QDict and
- * the returned QObject is a QList of all devices.
- *
- * The QDict contains the following:
- *
- * - device: device name
- * - stats: A QDict with the statistics information, it contains:
- * - rd_bytes: bytes read
- * - wr_bytes: bytes written
- * - rd_operations: read operations
- * - wr_operations: write operations
- * - wr_highest_offset: Highest offset of a sector written since the
- * BlockDriverState has been opened
- * - parent: A QDict recursively holding the statistics of the underlying
- *protocol (e.g. the host file for a qcow2 image). If there is no
- *underlying protocol, this field is omitted.
- *
- * Example:
- *
- * [ { device: ide0-hd0,
- * stats: { rd_bytes: 512,
- * wr_bytes: 0,
- * rd_operations: 1,
- * wr_operations: 0,
- * wr_highest_offset: 0 },
- * parent: {
- * stats: { rd_bytes: 1024,
- * wr_bytes: 0,
- * rd_operations: 2,
- * wr_operations: 0,
- * wr_highest_offset: 0,
- * } } },
- * { device: ide1-cd0,
- * stats: { rd_bytes: 0,
- * wr_bytes: 0,
- * rd_operations: 0,
- * wr_operations: 0,
- * wr_highest_offset: 0 } },
- */
void bdrv_info_stats(Monitor *mon, QObject **ret_data)
{
QObject *obj;
diff --git a/hw/pci.c b/hw/pci.c
index 8d84651..4636193 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1362,67 +1362,6 @@ static QObject *pci_get_bus_dict(PCIBus *bus, int
bus_num)
return NULL;
}
-/**
- * do_pci_info(): PCI buses and devices information
- *
- * The returned QObject is a QList of all buses. Each bus is
- * represented by a QDict, which has a key with a QList of all
- * PCI devices attached to it. Each device is represented by
- * a QDict.
- *
- * The bus QDict contains the following:
- *
- * - bus: bus number
- * - devices: a QList of QDicts, each QDict represents a PCI
- * device
- *
- * The PCI device QDict contains the following:
- *
- * - bus: identical to the parent's bus number
- * - slot: slot number
- * - function: function number
- * - class_info: a QDict containing:
- * - desc: device class description (optional)
- * - class: device class number
- * - id: a QDict containing:
- * - device: device ID
- * - vendor: vendor ID
- * - irq: device's IRQ if assigned (optional)
- * - qdev_id: qdev
[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs
On Mon, 31 May 2010 16:13:12 +0200
Markus Armbruster arm...@redhat.com wrote:
We need Device IDs to be unique and not contain '/' so device tree
nodes can always be unambigously referenced by tree path.
We already have some protection against duplicate IDs, but it got
holes:
* We don't assign IDs to default devices.
* -device and device_add use the ID of a qemu_device_opts. Which
rejects duplicate IDs.
* pci_add nic -net use either the ID or option name of
qemu_net_opts. And there's our hole. Reproducible with -net user
-net nic,id=foo -device lsi,id=foo.
Two bugs that might not be related to this thread:
* id member is not mandatory for the device_add command:
{ execute: device_add, arguments: { driver: e1000 } }
{return: {}}
* id member remains in use when the netdev_add command fails:
{ execute: netdev_add, arguments: { id: foobar } }
{error: {class: MissingParameter, desc: Parameter 'type' is
missing, data: {name: type}}}
{ execute: netdev_add, arguments: { type: user, id: foobar } }
{error: {class: DuplicateId, desc: Duplicate ID 'foobar' for
netdev, data: {object: netdev, id: foobar}}}
[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs
On Tue, 01 Jun 2010 15:09:34 +0200
Jan Kiszka jan.kis...@siemens.com wrote:
Luiz Capitulino wrote:
Two bugs that might not be related to this thread:
* id member is not mandatory for the device_add command:
{ execute: device_add, arguments: { driver: e1000 } }
{return: {}}
Once we enable qtree paths for device_del, this is no longer an issue,
devices will remain addressable.
Main point is whether id is required or not, I think it should be.
[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs
On Tue, 01 Jun 2010 15:19:59 +0200
Jan Kiszka jan.kis...@siemens.com wrote:
Luiz Capitulino wrote:
On Tue, 01 Jun 2010 15:09:34 +0200
Jan Kiszka jan.kis...@siemens.com wrote:
Luiz Capitulino wrote:
Two bugs that might not be related to this thread:
* id member is not mandatory for the device_add command:
{ execute: device_add, arguments: { driver: e1000 } }
{return: {}}
Once we enable qtree paths for device_del, this is no longer an issue,
devices will remain addressable.
Main point is whether id is required or not, I think it should be.
And I think it might be recommended but should become mandatory
(specifically not for HMP).
Maybe you meant 'should not'?
Anyway, if we keep it optional for device_add we should also change
the others _add commands to have the same behavior.
Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce commands documentation
On Mon, 31 May 2010 21:22:22 +0100
Stefan Hajnoczi stefa...@gmail.com wrote:
On Mon, May 31, 2010 at 6:43 PM, Luiz Capitulino lcapitul...@redhat.com
wrote:
Hi Luiz,
I'm interested in QMP, have left some feedback. As I get up to speed
with QMP my questions and suggestions will hopefully be useful.
Apologies in advance if any of my thoughts here are old news and have
been discussed elsewhere :).
No problem.
I didn't see documentation on the errors that a command can encounter.
Error documentation would be useful for commands where the client
needs to react by trying an alternative command. If errors are opaque
or undocumented, then the client can't perform error recovery, they
can only log the error or prompt the user for help.
The immediate need is to have something which is minimally useful,
we're going to describe errors later.
Actually, ultimate goal is to have self-description support, which should
describe everything (commands, arguments, errors, async messages, etc). But
I'm wondering whether we're going to have this on time for 0.13.
@@ -113,6 +184,35 @@ Password:
�...@end table
ETEXI
+SQMP
+change
+--
+
+Change a removable medium or VNC configuration.
+
+Arguments:
+
+- device: device name (json-string)
+- target: filename or item (json-string)
+- arg: additional argument (json-string, optional)
+
+Examples:
+
+1. Change a removable medium
+
+- { execute: change,
+ arguments: { device: ide1-cd0,
+ target:
/srv/images/Fedora-12-x86_64-DVD.iso } }
+- { return: {} }
+
+2. Change VNC password
+
+- { execute: change,
+ arguments: { device: vnc, target: password,
+ arg: foobar1 } }
+- { return: {} }
+
+EQMP
{
.name = screendump,
For removable media is there a way to use, for instance, sheepdog, or
is the target strictly a file name?
Good question, hey, aren't you a block layer guy? :)
Anyway, this command has the same behavior of HMP's change command.
Can the VNC password be removed when arg is the empty string or arg is
not present?
Yes, when arg is the empty string the password seems to be removed, which
is an ill behavior.
Anyway, the plan is to drop this command from QMP and introduce
change_blockdev change_password.
@@ -532,6 +747,24 @@ STEXI
�...@findex cpu
Set the default CPU.
ETEXI
+SQMP
+cpu
+---
+
+Set the default CPU.
+
+Arguments:
+
+- index: the CPU's index (json-int)
+
+Example:
+
+- { execute: cpu, arguments: { index: 0 } }
+- { return: {} }
+
+Note: CPUs' indexes are obtained with the 'query-cpus' command.
+
+EQMP
{
.name = mouse_move,
I believe this is the per-monitor default cpu for memory dumping and
other per-cpu commands. The concept of the default cpu makes sense
for an interactive monitor, humans don't want to type the cpu index
for each cpu-specific command. For QMP it feels cleaner to have a cpu
argument for commands that are per-cpu. As a newcomer I'm not sure of
the philosophy for QMP commands, 1:1 with monitor commands or as
appropriate for a JSON interface?
The latter and you're right about the cpu command, Avi has also
pointed this out.
If the QMP cpu command is supported in a QEMU release, can it be
phased out later?
No, we have to fix all this stuff before 0.13 is out, otherwise we'll
have to support them forever. Actually, we'll probably have some kind of
deprecation process, even then we'll have to support bad commands for
long time.
Re: [Qemu-devel] [PATCH 2/3] QMP: Introduce commands documentation
On Tue, 01 Jun 2010 09:40:44 +0200
Jan Kiszka jan.kis...@siemens.com wrote:
Stefan Hajnoczi wrote:
On Mon, May 31, 2010 at 6:43 PM, Luiz Capitulino lcapitul...@redhat.com
wrote:
Hi Luiz,
I'm interested in QMP, have left some feedback. As I get up to speed
with QMP my questions and suggestions will hopefully be useful.
Apologies in advance if any of my thoughts here are old news and have
been discussed elsewhere :).
I didn't see documentation on the errors that a command can encounter.
Error documentation would be useful for commands where the client
needs to react by trying an alternative command. If errors are opaque
or undocumented, then the client can't perform error recovery, they
can only log the error or prompt the user for help.
@@ -113,6 +184,35 @@ Password:
@end table
ETEXI
+SQMP
+change
+--
+
+Change a removable medium or VNC configuration.
+
+Arguments:
+
+- device: device name (json-string)
+- target: filename or item (json-string)
+- arg: additional argument (json-string, optional)
+
+Examples:
+
+1. Change a removable medium
+
+- { execute: change,
+ arguments: { device: ide1-cd0,
+target:
/srv/images/Fedora-12-x86_64-DVD.iso } }
+- { return: {} }
+
+2. Change VNC password
+
+- { execute: change,
+ arguments: { device: vnc, target: password,
+arg: foobar1 } }
+- { return: {} }
+
+EQMP
{
.name = screendump,
For removable media is there a way to use, for instance, sheepdog, or
is the target strictly a file name?
Can the VNC password be removed when arg is the empty string or arg is
not present?
@@ -532,6 +747,24 @@ STEXI
@findex cpu
Set the default CPU.
ETEXI
+SQMP
+cpu
+---
+
+Set the default CPU.
+
+Arguments:
+
+- index: the CPU's index (json-int)
+
+Example:
+
+- { execute: cpu, arguments: { index: 0 } }
+- { return: {} }
+
+Note: CPUs' indexes are obtained with the 'query-cpus' command.
+
+EQMP
{
.name = mouse_move,
I believe this is the per-monitor default cpu for memory dumping and
other per-cpu commands. The concept of the default cpu makes sense
for an interactive monitor, humans don't want to type the cpu index
for each cpu-specific command. For QMP it feels cleaner to have a cpu
argument for commands that are per-cpu. As a newcomer I'm not sure of
the philosophy for QMP commands, 1:1 with monitor commands or as
appropriate for a JSON interface?
Makes some sense.
I've a simple patch in my queue that can be used to assign different
arguments for QMP and HMP use. So we could add a 'cpu' argument to all
those QMP commands that so far rely on the above mechanism.
Nice.
If the QMP cpu command is supported in a QEMU release, can it be
phased out later?
Better avoid it being rolled out.
After 0.13 no incompatible protocol change will be allowed.
That said, first let us merge this documentation as baseline, then fix
such things (including error documentation) on top of it.
Exactly, the goal now is to describe the current protocol state.
Re: [Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs
On Tue, 01 Jun 2010 16:44:24 +0200
Markus Armbruster arm...@redhat.com wrote:
Luiz Capitulino lcapitul...@redhat.com writes:
On Mon, 31 May 2010 16:13:12 +0200
Markus Armbruster arm...@redhat.com wrote:
We need Device IDs to be unique and not contain '/' so device tree
nodes can always be unambigously referenced by tree path.
We already have some protection against duplicate IDs, but it got
holes:
* We don't assign IDs to default devices.
* -device and device_add use the ID of a qemu_device_opts. Which
rejects duplicate IDs.
* pci_add nic -net use either the ID or option name of
qemu_net_opts. And there's our hole. Reproducible with -net user
-net nic,id=foo -device lsi,id=foo.
Two bugs that might not be related to this thread:
* id member is not mandatory for the device_add command:
{ execute: device_add, arguments: { driver: e1000 } }
{return: {}}
Works as designed.
What about netdev_add?
{ execute: netdev_add, arguments: { type: user } }
{error: {class: MissingParameter, desc: Parameter 'id' is missing,
data: {name: id}}}
[Qemu-devel] Re: [PATCH 2/3] QMP: Introduce commands documentation
On Tue, 01 Jun 2010 18:10:26 +0200 Jan Kiszka jan.kis...@siemens.com wrote: Luiz Capitulino wrote: From: Jan Kiszka jan.kis...@siemens.com One of the most important missing feature in QMP today is its supported commands documentation. The plan is to make it part of self-description support, however self-description is a big task we have been postponing for a long time now and still don't know when it's going to be done. In order not to compromise QMP adoption and make users' life easier, this commit adds a simple text documentation which fully describes all QMP supported commands. This is not ideal for a number of reasons (harder to maintain, text-only, etc) but does improve the current situation. To avoid at least divering from the user monitor help and texi snippets, QMP bits are also maintained inside qemu-monitor.hx, and hxtool is extended to generate a single text file from them. Signed-off-by: Jan Kiszka jan.kis...@siemens.com Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- Makefile|5 +- QMP/README |5 +- configure |4 + hxtool | 44 ++- qemu-monitor.hx | 1322 +++ 5 files changed, 1376 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 7986bf6..3a8a311 100644 --- a/Makefile +++ b/Makefile @@ -29,7 +29,7 @@ $(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw) LIBS+=-lz $(LIBS_TOOLS) ifdef BUILD_DOCS -DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8 +DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8 QMP/qmp-commands.txt else DOCS= endif @@ -259,6 +259,9 @@ qemu-options.texi: $(SRC_PATH)/qemu-options.hx qemu-monitor.texi: $(SRC_PATH)/qemu-monitor.hx $(call quiet-command,sh $(SRC_PATH)/hxtool -t $ $@, GEN $@) +QMP/qmp-commands.txt: $(SRC_PATH)/qemu-monitor.hx + $(call quiet-command,sh $(SRC_PATH)/hxtool -q $ $@, GEN $@) + qemu-img-cmds.texi: $(SRC_PATH)/qemu-img-cmds.hx $(call quiet-command,sh $(SRC_PATH)/hxtool -t $ $@, GEN $@) This hunk seem to have gained tab-to-spaces conversion, unfortunately in a makefile. Applying the patch generated another hunk warning: Are those problems worth a resping or can we fix then with additional patches? It works ok here.
[Qemu-devel] [PATCH 1/9] QDict: Introduce qdict_get_try_bool()
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
qdict.c | 18 ++
qdict.h |1 +
2 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/qdict.c b/qdict.c
index 175bc17..ca3c3b1 100644
--- a/qdict.c
+++ b/qdict.c
@@ -287,6 +287,24 @@ int64_t qdict_get_try_int(const QDict *qdict, const char
*key,
}
/**
+ * qdict_get_try_bool(): Try to get a bool mapped by 'key'
+ *
+ * Return bool mapped by 'key', if it is not present in the
+ * dictionary or if the stored object is not of QBool type
+ * 'err_value' will be returned.
+ */
+int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value)
+{
+QObject *obj;
+
+obj = qdict_get(qdict, key);
+if (!obj || qobject_type(obj) != QTYPE_QBOOL)
+return err_value;
+
+return qbool_get_int(qobject_to_qbool(obj));
+}
+
+/**
* qdict_get_try_str(): Try to get a pointer to the stored string
* mapped by 'key'
*
diff --git a/qdict.h b/qdict.h
index 5e5902c..5cca816 100644
--- a/qdict.h
+++ b/qdict.h
@@ -57,6 +57,7 @@ QDict *qdict_get_qdict(const QDict *qdict, const char *key);
const char *qdict_get_str(const QDict *qdict, const char *key);
int64_t qdict_get_try_int(const QDict *qdict, const char *key,
int64_t err_value);
+int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value);
const char *qdict_get_try_str(const QDict *qdict, const char *key);
#endif /* QDICT_H */
--
1.7.1.231.gd0b16
[Qemu-devel] [PATCH 0/9]: QMP: Replace client argument checker
Current QMP's client argument checker implementation is more complex than it should be and has a flaw: it ignores unknown arguments. This series solves both problems by introducing a new, simple and ultra-poweful argument checker. This wasn't trivial to get right due to the number of errors combinations, so review is very appreciated.
[Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code
This commit introduces the first half of qmp_check_client_args(),
which is the new client argument checker.
It's introduced on top of the existing code, so that there are
no regressions during the transition.
It works this way: the command's args_type field (from
qemu-monitor.hx) is transformed into a qdict. Then we iterate
over it checking whether each mandatory argument has been
provided by the client.
All comunication between the iterator and its caller is done
via the new QMPArgCheckRes type.
Next commit adds the second half of this work: type checking
and invalid argument detection.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
monitor.c | 107 +
1 files changed, 107 insertions(+), 0 deletions(-)
diff --git a/monitor.c b/monitor.c
index bc3cc18..47a0da8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4259,6 +4259,108 @@ static int invalid_qmp_mode(const Monitor *mon, const
char *cmd_name)
return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
}
+typedef struct QMPArgCheckRes {
+int result;
+int skip;
+QDict *qdict;
+} QMPArgCheckRes;
+
+/*
+ * Check if client passed all mandatory args
+ */
+static void check_mandatory_args(const char *cmd_arg_name,
+ QObject *obj, void *opaque)
+{
+QString *type;
+QMPArgCheckRes *res = opaque;
+
+if (res-result 0) {
+/* report only the first error */
+return;
+}
+
+type = qobject_to_qstring(obj);
+assert(type != NULL);
+
+if (qstring_get_str(type)[0] == 'O') {
+QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
+assert(opts_list);
+res-result = check_opts(opts_list, res-qdict);
+res-skip = 1;
+} else if (qstring_get_str(type)[0] != '-'
+ qstring_get_str(type)[1] != '?'
+ !qdict_haskey(res-qdict, cmd_arg_name)) {
+res-result = -1;
+qerror_report(QERR_MISSING_PARAMETER, cmd_arg_name);
+}
+}
+
+static QDict *qdict_from_args_type(const char *args_type)
+{
+int i;
+QDict *qdict;
+QString *key, *type, *cur_qs;
+
+assert(args_type != NULL);
+
+qdict = qdict_new();
+
+if (args_type == NULL || args_type[0] == '\0') {
+/* no args, empty qdict */
+goto out;
+}
+
+key = qstring_new();
+type = qstring_new();
+
+cur_qs = key;
+
+for (i = 0;; i++) {
+switch (args_type[i]) {
+case ',':
+case '\0':
+qdict_put(qdict, qstring_get_str(key), type);
+QDECREF(key);
+if (args_type[i] == '\0') {
+goto out;
+}
+type = qstring_new(); /* qdict has ref */
+cur_qs = key = qstring_new();
+break;
+case ':':
+cur_qs = type;
+break;
+default:
+qstring_append_chr(cur_qs, args_type[i]);
+break;
+}
+}
+
+out:
+return qdict;
+}
+
+/*
+ * Client argument checking rules:
+ *
+ * 1. Client must provide all mandatory arguments
+ */
+static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
+{
+QDict *cmd_args;
+QMPArgCheckRes res = { .result = 0, .skip = 0 };
+
+cmd_args = qdict_from_args_type(cmd-args_type);
+
+res.qdict = client_args;
+qdict_iter(cmd_args, check_mandatory_args, res);
+
+/* TODO: Check client args type */
+
+QDECREF(cmd_args);
+return res.result;
+}
+
static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
{
int err;
@@ -4334,6 +4436,11 @@ static void handle_qmp_command(JSONMessageParser
*parser, QList *tokens)
QDECREF(input);
+err = qmp_check_client_args(cmd, args);
+if (err 0) {
+goto err_out;
+}
+
err = monitor_check_qmp_args(cmd, args);
if (err 0) {
goto err_out;
--
1.7.1.231.gd0b16
[Qemu-devel] [PATCH 8/9] QMP: Introduce qmp_check_input_obj()
This is similar to qmp_check_client_args(), but checks if
the input object follows the specification (QMP/qmp-spec.txt
section 2.3).
As we're limited to three keys, the work here is quite simple:
we iterate over the input object, each time checking if the
given argument complies to the specification.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
monitor.c | 45 +
1 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/monitor.c b/monitor.c
index 1875731..654b193 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4271,6 +4271,45 @@ static int qmp_check_client_args(const mon_cmd_t *cmd,
QDict *client_args)
return res.result;
}
+/*
+ * Input object checking rules
+ *
+ * 1. execute key must exist (not checked here)
+ * 2. execute key must be a string
+ * 3. arguments key must be a dict
+ * 4. id key can be anything (ie. json-value)
+ * 5. Any argument not listed above is invalid
+ */
+static void qmp_check_input_obj(const char *input_obj_arg_name,
+QObject *input_obj_arg, void *opaque)
+{
+int *err = opaque;
+
+if (*err 0) {
+/* report only the first error */
+return;
+}
+
+if (!strcmp(input_obj_arg_name, execute)) {
+if (qobject_type(input_obj_arg) != QTYPE_QSTRING) {
+qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, execute,
+ string);
+*err = -1;
+}
+} else if (!strcmp(input_obj_arg_name, arguments)) {
+if (qobject_type(input_obj_arg) != QTYPE_QDICT) {
+qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, arguments,
+ object);
+*err = -1;
+}
+} else if (!strcmp(input_obj_arg_name, id)) {
+/* nothing to do */
+} else {
+qerror_report(QERR_QMP_INVALID_INPUT_OBJECT_MEMBER,
input_obj_arg_name);
+*err = -1;
+}
+}
+
static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
{
int err;
@@ -4295,6 +4334,12 @@ static void handle_qmp_command(JSONMessageParser
*parser, QList *tokens)
input = qobject_to_qdict(obj);
+err = 0;
+qdict_iter(input, qmp_check_input_obj, err);
+if (err 0) {
+goto err_out;
+}
+
mon-mc-id = qdict_get(input, id);
qobject_incref(mon-mc-id);
--
1.7.1.231.gd0b16
[Qemu-devel] [PATCH 2/9] Monitor: handle optional '-' arg as a bool
Historically, user monitor arguments beginning with '-' (eg. '-f')
were passed as integers down to handlers.
I've maintained this behavior in the new monitor because we didn't
have a boolean type at the very beginning of QMP. Today we have it
and this behavior is causing trouble to the code that checks QMP
client's arguments.
This commit fixes the problem by doing the following changes:
1. User Monitor
Before: the optional arg was represented as a QInt, we'd pass 1
down to handlers if the user specified the argument or
0 otherwise
This commit: the optional arg is represented as a QBool, we pass
true down to handlers if the user specified the
argument, otherwise _nothing_ is passed
2. QMP
Before: the client was required to pass the arg as QBool, but we'd
convert it to QInt internally. If the argument wasn't passed,
we'd pass 0 down
This commit: keep requiring a QBool, but doesn't do any conversion
and doesn't pass any default value
3. Convert existing handlers (do_eject()/do_migrate()) to the new way
Before: Both handlers would expect QInt value, either 0 or 1
This commit: Change the handlers to accept a QBool, they handle the
following cases:
A) true is passed: the option is enabled
B) false is passed: the option is disabled
C) nothing is passed: option not specified, use
default behavior
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
migration.c | 16 +++-
monitor.c | 19 ---
2 files changed, 11 insertions(+), 24 deletions(-)
diff --git a/migration.c b/migration.c
index 706fe55..efecbdc 100644
--- a/migration.c
+++ b/migration.c
@@ -58,7 +58,9 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject
**ret_data)
{
MigrationState *s = NULL;
const char *p;
-int detach = qdict_get_int(qdict, detach);
+int detach = qdict_get_try_bool(qdict, detach, 0);
+int blk = qdict_get_try_bool(qdict, blk, 0);
+int inc = qdict_get_try_bool(qdict, inc, 0);
const char *uri = qdict_get_str(qdict, uri);
if (current_migration
@@ -69,21 +71,17 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject
**ret_data)
if (strstart(uri, tcp:, p)) {
s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
- (int)qdict_get_int(qdict, blk),
- (int)qdict_get_int(qdict, inc));
+ blk, inc);
#if !defined(WIN32)
} else if (strstart(uri, exec:, p)) {
s = exec_start_outgoing_migration(mon, p, max_throttle, detach,
- (int)qdict_get_int(qdict, blk),
- (int)qdict_get_int(qdict, inc));
+ blk, inc);
} else if (strstart(uri, unix:, p)) {
s = unix_start_outgoing_migration(mon, p, max_throttle, detach,
- (int)qdict_get_int(qdict, blk),
- (int)qdict_get_int(qdict, inc));
+ blk, inc);
} else if (strstart(uri, fd:, p)) {
s = fd_start_outgoing_migration(mon, p, max_throttle, detach,
-(int)qdict_get_int(qdict, blk),
-(int)qdict_get_int(qdict, inc));
+blk, inc);
#endif
} else {
monitor_printf(mon, unknown migration protocol: %s\n, uri);
diff --git a/monitor.c b/monitor.c
index 15b53b9..bc3cc18 100644
--- a/monitor.c
+++ b/monitor.c
@@ -971,7 +971,7 @@ static int eject_device(Monitor *mon, BlockDriverState *bs,
int force)
static int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
BlockDriverState *bs;
-int force = qdict_get_int(qdict, force);
+int force = qdict_get_try_bool(qdict, force, 0);
const char *filename = qdict_get_str(qdict, device);
bs = bdrv_find(filename);
@@ -3684,7 +3684,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor
*mon,
case '-':
{
const char *tmp = p;
-int has_option, skip_key = 0;
+int skip_key = 0;
/* option */
c = *typestr++;
@@ -3692,7 +3692,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor
*mon,
goto bad_type;
while (qemu_isspace(*p))
p++;
-has_option = 0;
if (*p == '-') {
p++;
if(c != *p) {
@@ -3708,11 +3707,11 @@ static const mon_cmd_t *monitor_parse_command(Monitor
*mon,
if(skip_key) {
p = tmp
[Qemu-devel] [PATCH 6/9] QMP: check_opts(): Minor cleanup
We couldn't do it before, otherwise we would break the intention
of the previous checker, which was to ensure that opts_list wasn't
a NULL before checking it.
Debug code, pretty minor, still I decided to maintain its original
behavior.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
monitor.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/monitor.c b/monitor.c
index d2a510e..1875731 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4084,6 +4084,7 @@ static int monitor_can_read(void *opaque)
static int check_opts(QemuOptsList *opts_list, QDict *args)
{
+assert(opts_list);
assert(!opts_list-desc-name);
return 0;
}
@@ -4188,7 +4189,6 @@ static void check_mandatory_args(const char *cmd_arg_name,
if (qstring_get_str(type)[0] == 'O') {
QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
-assert(opts_list);
res-result = check_opts(opts_list, res-qdict);
res-skip = 1;
} else if (qstring_get_str(type)[0] != '-'
--
1.7.1.231.gd0b16
[Qemu-devel] [PATCH 9/9] QMP: Drop old input object checking code
Previous commit added qmp_check_input_obj(), it does this
checking for us.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
monitor.c |6 --
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/monitor.c b/monitor.c
index 654b193..f849456 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4347,9 +4347,6 @@ static void handle_qmp_command(JSONMessageParser *parser,
QList *tokens)
if (!obj) {
qerror_report(QERR_QMP_BAD_INPUT_OBJECT, execute);
goto err_input;
-} else if (qobject_type(obj) != QTYPE_QSTRING) {
-qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, execute, string);
-goto err_input;
}
cmd_name = qstring_get_str(qobject_to_qstring(obj));
@@ -4381,9 +4378,6 @@ static void handle_qmp_command(JSONMessageParser *parser,
QList *tokens)
obj = qdict_get(input, arguments);
if (!obj) {
args = qdict_new();
-} else if (qobject_type(obj) != QTYPE_QDICT) {
-qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, arguments, object);
-goto err_input;
} else {
args = qobject_to_qdict(obj);
QINCREF(args);
--
1.7.1.231.gd0b16
[Qemu-devel] [PATCH 5/9] QMP: Drop old client argument checker
Previous two commits added qmp_check_client_args(), which
fully replaces this code and is way better.
It's important to note that the new checker doesn't support
the '/' arg type. As we don't have any of those handlers
converted to QMP, this is just dead code.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
monitor.c | 170 -
1 files changed, 0 insertions(+), 170 deletions(-)
diff --git a/monitor.c b/monitor.c
index 14790e6..d2a510e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4082,177 +4082,12 @@ static int monitor_can_read(void *opaque)
return (mon-suspend_cnt == 0) ? 1 : 0;
}
-typedef struct CmdArgs {
-QString *name;
-int type;
-int flag;
-int optional;
-} CmdArgs;
-
-static int check_opt(const CmdArgs *cmd_args, const char *name, QDict *args)
-{
-if (!cmd_args-optional) {
-qerror_report(QERR_MISSING_PARAMETER, name);
-return -1;
-}
-
-return 0;
-}
-
-static int check_arg(const CmdArgs *cmd_args, QDict *args)
-{
-QObject *value;
-const char *name;
-
-name = qstring_get_str(cmd_args-name);
-
-if (!args) {
-return check_opt(cmd_args, name, args);
-}
-
-value = qdict_get(args, name);
-if (!value) {
-return check_opt(cmd_args, name, args);
-}
-
-switch (cmd_args-type) {
-case 'F':
-case 'B':
-case 's':
-if (qobject_type(value) != QTYPE_QSTRING) {
-qerror_report(QERR_INVALID_PARAMETER_TYPE, name, string);
-return -1;
-}
-break;
-case '/': {
-int i;
-const char *keys[] = { count, format, size, NULL };
-
-for (i = 0; keys[i]; i++) {
-QObject *obj = qdict_get(args, keys[i]);
-if (!obj) {
-qerror_report(QERR_MISSING_PARAMETER, name);
-return -1;
-}
-if (qobject_type(obj) != QTYPE_QINT) {
-qerror_report(QERR_INVALID_PARAMETER_TYPE, name, int);
-return -1;
-}
-}
-break;
-}
-case 'i':
-case 'l':
-case 'M':
-if (qobject_type(value) != QTYPE_QINT) {
-qerror_report(QERR_INVALID_PARAMETER_TYPE, name, int);
-return -1;
-}
-break;
-case 'f':
-case 'T':
-if (qobject_type(value) != QTYPE_QINT qobject_type(value) !=
QTYPE_QFLOAT) {
-qerror_report(QERR_INVALID_PARAMETER_TYPE, name, number);
-return -1;
-}
-break;
-case 'b':
-if (qobject_type(value) != QTYPE_QBOOL) {
-qerror_report(QERR_INVALID_PARAMETER_TYPE, name, bool);
-return -1;
-}
-break;
-case '-':
-if (qobject_type(value) != QTYPE_QINT
-qobject_type(value) != QTYPE_QBOOL) {
-qerror_report(QERR_INVALID_PARAMETER_TYPE, name, bool);
-return -1;
-}
-break;
-case 'O':
-default:
-/* impossible */
-abort();
-}
-
-return 0;
-}
-
-static void cmd_args_init(CmdArgs *cmd_args)
-{
-cmd_args-name = qstring_new();
-cmd_args-type = cmd_args-flag = cmd_args-optional = 0;
-}
-
static int check_opts(QemuOptsList *opts_list, QDict *args)
{
assert(!opts_list-desc-name);
return 0;
}
-/*
- * This is not trivial, we have to parse Monitor command's argument
- * type syntax to be able to check the arguments provided by clients.
- *
- * In the near future we will be using an array for that and will be
- * able to drop all this parsing...
- */
-static int monitor_check_qmp_args(const mon_cmd_t *cmd, QDict *args)
-{
-int err;
-const char *p;
-CmdArgs cmd_args;
-QemuOptsList *opts_list;
-
-if (cmd-args_type == NULL) {
-return (qdict_size(args) == 0 ? 0 : -1);
-}
-
-err = 0;
-cmd_args_init(cmd_args);
-opts_list = NULL;
-
-for (p = cmd-args_type;; p++) {
-if (*p == ':') {
-cmd_args.type = *++p;
-p++;
-if (cmd_args.type == '-') {
-cmd_args.flag = *p++;
-cmd_args.optional = 1;
-} else if (cmd_args.type == 'O') {
-opts_list = qemu_find_opts(qstring_get_str(cmd_args.name));
-assert(opts_list);
-} else if (*p == '?') {
-cmd_args.optional = 1;
-p++;
-}
-
-assert(*p == ',' || *p == '\0');
-if (opts_list) {
-err = check_opts(opts_list, args);
-opts_list = NULL;
-} else {
-err = check_arg(cmd_args, args);
-QDECREF(cmd_args.name);
-cmd_args_init(cmd_args
[Qemu-devel] [PATCH 7/9] QError: Introduce QERR_QMP_BAD_INPUT_OBJECT_MEMBER
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
qerror.c |4
qerror.h |3 +++
2 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/qerror.c b/qerror.c
index 44d0bf8..b26224e 100644
--- a/qerror.c
+++ b/qerror.c
@@ -177,6 +177,10 @@ static const QErrorStringTable qerror_table[] = {
.desc = QMP input object member '%(member)' expects
'%(expected)',
},
{
+.error_fmt = QERR_QMP_INVALID_INPUT_OBJECT_MEMBER,
+.desc = QMP input object member '%(member)' is invalid,
+},
+{
.error_fmt = QERR_SET_PASSWD_FAILED,
.desc = Could not set password,
},
diff --git a/qerror.h b/qerror.h
index 77ae574..eec9949 100644
--- a/qerror.h
+++ b/qerror.h
@@ -148,6 +148,9 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_QMP_BAD_INPUT_OBJECT_MEMBER \
{ 'class': 'QMPBadInputObjectMember', 'data': { 'member': %s, 'expected':
%s } }
+#define QERR_QMP_INVALID_INPUT_OBJECT_MEMBER \
+{ 'class': 'QMPInvalidInputObjectMember', 'data': { 'member': %s } }
+
#define QERR_SET_PASSWD_FAILED \
{ 'class': 'SetPasswdFailed', 'data': {} }
--
1.7.1.231.gd0b16
[Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code
This commit introduces check_client_args_type(), which is
called by qmp_check_client_args() and complements the
previous commit.
Now the new client's argument checker code is capable of
doing type checking and detecting unknown arguments.
It works this way: we iterate over the client's arguments
qdict and for each argument we check if it exists and if
its type is correct.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
monitor.c | 77 -
1 files changed, 76 insertions(+), 1 deletions(-)
diff --git a/monitor.c b/monitor.c
index 47a0da8..14790e6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes {
} QMPArgCheckRes;
/*
+ * Check if client's argument exists and type is correct
+ */
+static void check_client_args_type(const char *client_arg_name,
+ QObject *client_arg, void *opaque)
+{
+QObject *obj;
+QString *arg_type;
+QMPArgCheckRes *res = opaque;
+
+if (res-result 0) {
+/* report only the first error */
+return;
+}
+
+obj = qdict_get(res-qdict, client_arg_name);
+if (!obj) {
+/* client arg doesn't exist */
+res-result = -1;
+qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
+return;
+}
+
+arg_type = qobject_to_qstring(obj);
+assert(arg_type != NULL);
+
+/* check if argument's type is correct */
+switch (qstring_get_str(arg_type)[0]) {
+case 'F':
+case 'B':
+case 's':
+if (qobject_type(client_arg) != QTYPE_QSTRING) {
+qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
+ string);
+res-result = -1;
+}
+break;
+case 'i':
+case 'l':
+case 'M':
+if (qobject_type(client_arg) != QTYPE_QINT) {
+qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name, int);
+res-result = -1;
+}
+break;
+case 'f':
+case 'T':
+if (qobject_type(client_arg) != QTYPE_QINT
+qobject_type(client_arg) != QTYPE_QFLOAT) {
+qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
+ number);
+res-result = -1;
+}
+break;
+case 'b':
+case '-':
+if (qobject_type(client_arg) != QTYPE_QBOOL) {
+qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
bool);
+res-result = -1;
+}
+break;
+case 'O':
+/* Not checked here */
+break;
+default:
+abort();
+}
+}
+
+/*
* Check if client passed all mandatory args
*/
static void check_mandatory_args(const char *cmd_arg_name,
@@ -4344,6 +4413,9 @@ out:
* Client argument checking rules:
*
* 1. Client must provide all mandatory arguments
+ * 2. Each argument provided by the client must be valid
+ * 3. Each argument provided by the client must have the type expected
+ *by the command
*/
static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
{
@@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t *cmd,
QDict *client_args)
res.qdict = client_args;
qdict_iter(cmd_args, check_mandatory_args, res);
-/* TODO: Check client args type */
+if (!res.result !res.skip) {
+res.qdict = cmd_args;
+qdict_iter(client_args, check_client_args_type, res);
+}
QDECREF(cmd_args);
return res.result;
--
1.7.1.231.gd0b16
Re: [Qemu-devel] [PATCH 1/9] QDict: Introduce qdict_get_try_bool()
On Wed, 02 Jun 2010 08:35:16 +0200
Markus Armbruster arm...@redhat.com wrote:
Luiz Capitulino lcapitul...@redhat.com writes:
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
qdict.c | 18 ++
qdict.h |1 +
2 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/qdict.c b/qdict.c
index 175bc17..ca3c3b1 100644
--- a/qdict.c
+++ b/qdict.c
@@ -287,6 +287,24 @@ int64_t qdict_get_try_int(const QDict *qdict, const
char *key,
}
/**
+ * qdict_get_try_bool(): Try to get a bool mapped by 'key'
+ *
+ * Return bool mapped by 'key', if it is not present in the
+ * dictionary or if the stored object is not of QBool type
+ * 'err_value' will be returned.
+ */
+int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value)
+{
+QObject *obj;
+
+obj = qdict_get(qdict, key);
+if (!obj || qobject_type(obj) != QTYPE_QBOOL)
+return err_value;
+
+return qbool_get_int(qobject_to_qbool(obj));
+}
+
+/**
* qdict_get_try_str(): Try to get a pointer to the stored string
* mapped by 'key'
*
diff --git a/qdict.h b/qdict.h
index 5e5902c..5cca816 100644
--- a/qdict.h
+++ b/qdict.h
@@ -57,6 +57,7 @@ QDict *qdict_get_qdict(const QDict *qdict, const char
*key);
const char *qdict_get_str(const QDict *qdict, const char *key);
int64_t qdict_get_try_int(const QDict *qdict, const char *key,
int64_t err_value);
+int qdict_get_try_bool(const QDict *qdict, const char *key, int err_value);
const char *qdict_get_try_str(const QDict *qdict, const char *key);
#endif /* QDICT_H */
Nitpick: there's precedence for parameter name err_value in qdict.c,
but it's a misleading name all the same. The parameter is a default
value. Missing key is *not* an error.
Good point.
Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code
On Wed, 02 Jun 2010 08:59:11 +0200
Markus Armbruster arm...@redhat.com wrote:
Luiz Capitulino lcapitul...@redhat.com writes:
[...]
+
+type = qobject_to_qstring(obj);
+assert(type != NULL);
+
+if (qstring_get_str(type)[0] == 'O') {
+QemuOptsList *opts_list = qemu_find_opts(cmd_arg_name);
+assert(opts_list);
+res-result = check_opts(opts_list, res-qdict);
I doubt this is the right place for calling check_opts.
Can you suggest the right place?
Re: [Qemu-devel] [PATCH 8/9] QMP: Introduce qmp_check_input_obj()
On Wed, 02 Jun 2010 09:39:26 +0200 Markus Armbruster arm...@redhat.com wrote: Luiz Capitulino lcapitul...@redhat.com writes: This is similar to qmp_check_client_args(), but checks if the input object follows the specification (QMP/qmp-spec.txt section 2.3). As we're limited to three keys, the work here is quite simple: we iterate over the input object, each time checking if the given argument complies to the specification. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- monitor.c | 45 + 1 files changed, 45 insertions(+), 0 deletions(-) diff --git a/monitor.c b/monitor.c index 1875731..654b193 100644 --- a/monitor.c +++ b/monitor.c @@ -4271,6 +4271,45 @@ static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args) return res.result; } +/* + * Input object checking rules + * + * 1. execute key must exist (not checked here) + * 2. execute key must be a string + * 3. arguments key must be a dict + * 4. id key can be anything (ie. json-value) Really? Checking qmp-spec.txt... yes, really. Is it a good idea to permit objects and arrays? It was Avi's suggestion to allow anything, maybe arrays don't make sense but objects do.
Re: [Qemu-devel] [PATCH 4/9] QMP: Second half of the new argument checking code
On Wed, 02 Jun 2010 09:31:24 +0200
Markus Armbruster arm...@redhat.com wrote:
Luiz Capitulino lcapitul...@redhat.com writes:
This commit introduces check_client_args_type(), which is
called by qmp_check_client_args() and complements the
previous commit.
Now the new client's argument checker code is capable of
doing type checking and detecting unknown arguments.
It works this way: we iterate over the client's arguments
qdict and for each argument we check if it exists and if
its type is correct.
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
monitor.c | 77
-
1 files changed, 76 insertions(+), 1 deletions(-)
diff --git a/monitor.c b/monitor.c
index 47a0da8..14790e6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4266,6 +4266,75 @@ typedef struct QMPArgCheckRes {
} QMPArgCheckRes;
/*
+ * Check if client's argument exists and type is correct
+ */
+static void check_client_args_type(const char *client_arg_name,
+ QObject *client_arg, void *opaque)
+{
+QObject *obj;
+QString *arg_type;
+QMPArgCheckRes *res = opaque;
+
+if (res-result 0) {
+/* report only the first error */
+return;
+}
+
+obj = qdict_get(res-qdict, client_arg_name);
+if (!obj) {
+/* client arg doesn't exist */
+res-result = -1;
+qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
+return;
+}
+
+arg_type = qobject_to_qstring(obj);
+assert(arg_type != NULL);
+
+/* check if argument's type is correct */
+switch (qstring_get_str(arg_type)[0]) {
+case 'F':
+case 'B':
+case 's':
+if (qobject_type(client_arg) != QTYPE_QSTRING) {
+qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
+ string);
+res-result = -1;
+}
+break;
+case 'i':
+case 'l':
+case 'M':
+if (qobject_type(client_arg) != QTYPE_QINT) {
+qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
int);
+res-result = -1;
+}
+break;
+case 'f':
+case 'T':
+if (qobject_type(client_arg) != QTYPE_QINT
+qobject_type(client_arg) != QTYPE_QFLOAT) {
+qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
+ number);
+res-result = -1;
+}
+break;
+case 'b':
+case '-':
+if (qobject_type(client_arg) != QTYPE_QBOOL) {
+qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
bool);
+res-result = -1;
+}
+break;
+case 'O':
+/* Not checked here */
+break;
What about case '/'? I guess it doesn't make much sense for QMP, but
the old checker handles it. If we drop it from QMP, we should document
the restriction in the source.
Yes, there're two args_type we don't handle in QMP because we don't have
any of those handlers converted: '/' and '.'.
I think it's unlikely to get them converted in this form, so the current
implementation contains dead-code. I explained this in one commit log, but
maybe it's the wrong place.
+default:
+abort();
+}
+}
+
+/*
* Check if client passed all mandatory args
*/
static void check_mandatory_args(const char *cmd_arg_name,
@@ -4344,6 +4413,9 @@ out:
* Client argument checking rules:
*
* 1. Client must provide all mandatory arguments
+ * 2. Each argument provided by the client must be valid
+ * 3. Each argument provided by the client must have the type expected
+ *by the command
*/
static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
{
@@ -4355,7 +4427,10 @@ static int qmp_check_client_args(const mon_cmd_t
*cmd, QDict *client_args)
res.qdict = client_args;
qdict_iter(cmd_args, check_mandatory_args, res);
-/* TODO: Check client args type */
+if (!res.result !res.skip) {
+res.qdict = cmd_args;
+qdict_iter(client_args, check_client_args_type, res);
+}
What if we have both an O-type argument and other arguments? Then the
'O' makes check_client_args_type() set res.skip, and we duly skip
checking the other arguments here.
Oh, good catch. I thought that that was what the current code does,
but looks like I misread it.
Again, the iterator makes for tortuous code.
QDECREF(cmd_args);
return res.result;
[Qemu-devel] [Bug 391879] Re: migrate exec ignores exit status
** Changed in: qemu Status: New = Confirmed -- migrate exec ignores exit status https://bugs.launchpad.net/bugs/391879 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: Confirmed Status in “qemu-kvm” package in Ubuntu: Won't Fix Bug description: Binary package hint: kvm Using migrate exec:cat foo; false in the monitor results in the state of the VM being written to foo, as expected, and the VM then being stopped. This is surprising, as I think it stands to reason that in case of a failed migrate-exec process, which is what a non-zero exit status implies to me, the VM should continue. == Version information $ lsb_release -rd Description:Ubuntu 9.04 Release:9.04 $ apt-cache policy kvm kvm: Installed: 1:84+dfsg-0ubuntu11 Candidate: 1:84+dfsg-0ubuntu11 Version table: *** 1:84+dfsg-0ubuntu11 0 500 http://gb.archive.ubuntu.com jaunty/main Packages 100 /var/lib/dpkg/status
[Qemu-devel] [Bug 450522] Re: Unable to set fullscreen anymore
Works for me, I believe this is already fixed. ** Changed in: qemu Status: New = Fix Committed ** Changed in: qemu Status: Fix Committed = New -- Unable to set fullscreen anymore https://bugs.launchpad.net/bugs/450522 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: New Bug description: Description: When pressing Ctrl-Alt-f over the QEmu window, it won't go fullscreen anymore. Additional info: * package version: 0.11.0 * window managers: xfwm, metacity Steps to reproduce: Start QEmu with a command similar to this: /usr/bin/qemu-system-x86_64 -boot d -m 2047 -hda 'linux.qemu' -cdrom 'linux.iso' -net nic,vlan=0 -net user,vlan=0 -localtime -enable-kvm Hold Ctrl+Alt keys and press the f key with the QEmu window focused. What was expected: It should go fullscreen. If I press the combo and later maximize the window from the window button at some point it will go fullscreen but I can't go back, it behaves pretty strange and I can't reproduce the rest on request.
[Qemu-devel] [Bug 491345] Re: remote migration fails with message load of migration failed
Why did this bug get the windows tag? -- remote migration fails with message load of migration failed https://bugs.launchpad.net/bugs/491345 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: New Bug description: Remote migration fails with message load of migration failed on the destination after migration Steps to recreate: 1) install qemu frm git clone git://git.savannah.nongnu.org/qemu.git 2)The VM image was shared using nfs 3)qemu cmdline used: Source: /usr/local/bin/qemu-system-x86_64 -name 'vm1' -drive file=win2k3sp2-32.qcow2 -m 6144 --enable-kvm-usbdevice tablet -vnc :0 -monitor stdio Destination: /usr/local/bin/qemu-system-x86_64 -name 'vm1' -drive file=win2k3sp2-32.qcow2 -m 6144 --enable-kvm-usbdevice tablet -vnc :0 -monitor stdio --incoming tcp:0: 5)migrate tcp:destination: uname -a Linux 2.6.30.9-96.fc11.x86_64 #1 SMP Wed Nov 4 00:02:04 EST 2009 x86_64 x86_64 x86_64 GNU/Linux Distro: fedora 11 Thx yogi
[Qemu-devel] [Bug 450522] Re: Unable to set fullscreen anymore
** Changed in: qemu Status: New = Fix Committed -- Unable to set fullscreen anymore https://bugs.launchpad.net/bugs/450522 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: Fix Committed Bug description: Description: When pressing Ctrl-Alt-f over the QEmu window, it won't go fullscreen anymore. Additional info: * package version: 0.11.0 * window managers: xfwm, metacity Steps to reproduce: Start QEmu with a command similar to this: /usr/bin/qemu-system-x86_64 -boot d -m 2047 -hda 'linux.qemu' -cdrom 'linux.iso' -net nic,vlan=0 -net user,vlan=0 -localtime -enable-kvm Hold Ctrl+Alt keys and press the f key with the QEmu window focused. What was expected: It should go fullscreen. If I press the combo and later maximize the window from the window button at some point it will go fullscreen but I can't go back, it behaves pretty strange and I can't reproduce the rest on request.
[Qemu-devel] [PATCH] give some useful error messages when tap open
From: Michael Tokarev m...@tls.msk.ru
In net/tap-linux.c, when manipulation of /dev/net/tun fails, it prints
(with fprintf) something like this:
warning: could not open /dev/net/tun: no virtual network emulation
this has 2 issues:
1) it is not a warning really, it's a fatal error (kvm exits after
that),
2) there's no indication as of what's actually wrong: printing errno there
is helpful.
The patch below removes the warning prefix, uses %m (since it's linux,
%m is available as format modifier), and changes fprintf() to %qemu_error().
Now it prints something like this instead:
could not configure /dev/net/tun: Device or resource busy
(there are 2 messages like that in the same function)
This fixes Debian bug #578154, see
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=578154
Signed-off-by: Michael Tokarev m...@tls.msk.ru
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
IMPORTANT: this an old fix that got forgotten, probably because it was
submitted in the middle of thread. I've just compiled tested it.
net/tap-linux.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 03b8301..c92983c 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -33,14 +33,16 @@
#include qemu-common.h
#include qemu-error.h
+#define PATH_NET_TUN /dev/net/tun
+
int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int
vnet_hdr_required)
{
struct ifreq ifr;
int fd, ret;
-TFR(fd = open(/dev/net/tun, O_RDWR));
+TFR(fd = open(PATH_NET_TUN, O_RDWR));
if (fd 0) {
-fprintf(stderr, warning: could not open /dev/net/tun: no virtual
network emulation\n);
+error_report(could not open %s: %m, PATH_NET_TUN);
return -1;
}
memset(ifr, 0, sizeof(ifr));
@@ -71,7 +73,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
int vnet_hdr_required
pstrcpy(ifr.ifr_name, IFNAMSIZ, tap%d);
ret = ioctl(fd, TUNSETIFF, (void *) ifr);
if (ret != 0) {
-fprintf(stderr, warning: could not configure /dev/net/tun: no virtual
network emulation\n);
+error_report(could not configure %s (%s): %m, PATH_NET_TUN,
ifr.ifr_name);
close(fd);
return -1;
}
--
1.7.1.231.gd0b16
[Qemu-devel] [Bug 584153] Re: no useful error message when tap device open fails
Michael's fix is good, but it got forgotten, probably because he has submitted it in the middle of a thread. I'm going resubmit it again. ** Changed in: qemu Status: Incomplete = In Progress -- no useful error message when tap device open fails https://bugs.launchpad.net/bugs/584153 You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. Status in QEMU: In Progress Status in “qemu-kvm” package in Debian: Fix Released Bug description: When using tap network devices and it fails, qemu gives no information about what the problem is (permission denied, device busy or other), making debugging of such situations, especially for newbies, very difficult. The proposed patch just adds strerror() around the place, making it more friendly. See also Debian bug#578154, http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=578154 and a discussion on qemu-devel at http://marc.info/?t=12719287523 .
