On 04/21/2010 10:01 AM, Chris Lalancette wrote:
Since we are adding a new "per-hypervisor" protocol, we make it so that the qemu remote protocol uses a new PROTOCOL and PROGRAM number. This allows us to easily distinguish it from the normal REMOTE protocol.
This necessitates changing the proc in remote_message_header from a "remote_procedure" to an "unsigned", which should be the same size (and thus preserve the on-wire protocol).
REMOTE_PROTOCOL = $(top_srcdir)/src/remote/remote_protocol.x +QEMU_PROTOCOL = $(top_srcdir)/src/remote/qemu_protocol.x
remote_dispatch_prototypes.h: $(srcdir)/remote_generate_stubs.pl $(REMOTE_PROTOCOL) - $(AM_V_GEN)perl -w $(srcdir)/remote_generate_stubs.pl -p $(REMOTE_PROTOCOL) > $@ + $(AM_V_GEN)perl -w $(srcdir)/remote_generate_stubs.pl -c -p remote $(REMOTE_PROTOCOL) > $@
remote_dispatch_table.h: $(srcdir)/remote_generate_stubs.pl $(REMOTE_PROTOCOL) - $(AM_V_GEN)perl -w $(srcdir)/remote_generate_stubs.pl -t $(REMOTE_PROTOCOL) > $@ + $(AM_V_GEN)perl -w $(srcdir)/remote_generate_stubs.pl -c -t remote $(REMOTE_PROTOCOL) > $@
remote_dispatch_args.h: $(srcdir)/remote_generate_stubs.pl $(REMOTE_PROTOCOL) - $(AM_V_GEN)perl -w $(srcdir)/remote_generate_stubs.pl -a $(REMOTE_PROTOCOL) > $@ + $(AM_V_GEN)perl -w $(srcdir)/remote_generate_stubs.pl -c -a remote $(REMOTE_PROTOCOL) > $@
remote_dispatch_ret.h: $(srcdir)/remote_generate_stubs.pl $(REMOTE_PROTOCOL) - $(AM_V_GEN)perl -w $(srcdir)/remote_generate_stubs.pl -r $(REMOTE_PROTOCOL) > $@ + $(AM_V_GEN)perl -w $(srcdir)/remote_generate_stubs.pl -c -r remote $(REMOTE_PROTOCOL) > $@ + +qemu_dispatch_prototypes.h: $(srcdir)/remote_generate_stubs.pl $(QEMU_PROTOCOL) + $(AM_V_GEN)perl -w $(srcdir)/remote_generate_stubs.pl -p qemu $(QEMU_PROTOCOL) > $@
Reading through the patch in linear order, it was not clear at this point why remote needs the addition of the new -c option, but not qemu.
+remoteDispatchClientRequest(struct qemud_server *server, + struct qemud_client *client, + struct qemud_client_message *msg) { int ret; remote_error rerr; + int qemu_call;
Based on your use, this is better as bool...
- if (msg->hdr.vers != REMOTE_PROTOCOL_VERSION) { + + if (!qemu_call && msg->hdr.vers != REMOTE_PROTOCOL_VERSION) { remoteDispatchFormatError (&rerr, _("version mismatch (actual %x, expected %x)"), msg->hdr.vers, REMOTE_PROTOCOL_VERSION); goto error; } + else if (qemu_call && msg->hdr.vers != QEMU_PROTOCOL_VERSION) { + remoteDispatchFormatError (&rerr, + _("version mismatch (actual %x, expected %x)"), + msg->hdr.vers, QEMU_PROTOCOL_VERSION); + goto error; + }
switch (msg->hdr.type) { case REMOTE_CALL: - return remoteDispatchClientCall(server, client, msg); + return remoteDispatchClientCall(server, client, msg, qemu_call);
...which affects the prototype of remoteDispatchClientCall.
@@ -6390,6 +6406,35 @@ remoteDispatchNumOfNwfilters (struct qemud_server *server ATTRIBUTE_UNUSED, return 0; }
+static int +qemuDispatchMonitorCommand (struct qemud_server *server ATTRIBUTE_UNUSED,
Given that earlier in the patch, you removed the space for remoteDispatchClientRequest, it seems odd to see the space here.
+++ b/daemon/remote_generate_stubs.pl @@ -1,7 +1,16 @@ #!/usr/bin/perl -w # -# This script parses remote_protocol.x and produces lots of boilerplate -# code for both ends of the remote connection. +# This script parses remote_protocol.x or qemu_protocol.x and produces lots of +# boilerplate code for both ends of the remote connection. +# +# The first non-option argument specifies the prefix to be searched for, and +# output to, the boilerplate code. The second non-option argument is the +# file you want to operate on. For instance, to generate the dispatch table +# for both remote_protocol.x and qemu_protocol.x, you would run the +# following: +# +# remote_generate_stubs.pl -c -t remote ../src/remote/remote_protocol.x +# remote_generate_stubs.pl -c -t qemu ../src/remote/qemu_protocol.x
This comment doesn't match the makefile.
# # By Richard Jones <rjones@redhat.com>
@@ -10,8 +19,12 @@ use strict; use Getopt::Std;
# Command line options. -our ($opt_p, $opt_t, $opt_a, $opt_r, $opt_d); -getopts ('ptard'); +our ($opt_p, $opt_t, $opt_a, $opt_r, $opt_d, $opt_c); +getopts ('ptardc'); + +my $structprefix = $ARGV[0]; +my $procprefix = uc $structprefix; +shift;
# Convert name_of_call to NameOfCall. sub name_to_ProcName { @@ -25,47 +38,50 @@ sub name_to_ProcName { # opinion about the name, args and return type of each RPC. my ($name, $ProcName, $id, %calls, @calls);
-# REMOTE_PROC_CLOSE has no args or ret. -$calls{close} = { - name => "close", - ProcName => "Close", - UC_NAME => "CLOSE", - args => "void", - ret => "void", -}; +# only generate a close method if -c was passed
Ah, now I see why only one of the two generators needed -c.
+if ($opt_c) { + # REMOTE_PROC_CLOSE has no args or ret. + $calls{close} = { + name => "close", + ProcName => "Close", + UC_NAME => "CLOSE", + args => "void", + ret => "void", + }; +}
@@ -88,7 +104,7 @@ while (<>) { # Output
print <<__EOF__; -/* Automatically generated by remote_generate_stubs.pl. +/* Automatically generated by ${structprefix}_generate_stubs.pl.
One search-and-replace too many. The script's name is fixed, so half your output files now have a bogus comment.
+/* Define the program number, protocol version and procedure numbers here. */ +const QEMU_PROGRAM = 0x20008087; +const QEMU_PROTOCOL_VERSION = 1; + +enum qemu_procedure { + QEMU_PROC_MONITOR_COMMAND = 1
Do we want the cute comment here about grouping command ids by 10, or save that until we actually have more commands? Overall, the patch is large, but looks decent. I don't know if it could have been subdivided any further. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org