[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 08/10] Qemu remote protocol.



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]