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@xxxxxxxxxx>
>
> @@ -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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list