On Tue, Apr 13, 2010 at 02:36:48PM -0400, Chris Lalancette wrote:
> Signed-off-by: Chris Lalancette <clalance@xxxxxxxxxx>
> ---
> daemon/Makefile.am | 25 +++++-
> daemon/dispatch.c | 171 +++++++++++++++++++------------------
> daemon/libvirtd.h | 1 +
> daemon/qemu_dispatch_args.h | 5 +
> daemon/qemu_dispatch_prototypes.h | 12 +++
> daemon/qemu_dispatch_ret.h | 5 +
> daemon/qemu_dispatch_table.h | 14 +++
> daemon/qemu_generate_stubs.pl | 170 ++++++++++++++++++++++++++++++++++++
> daemon/remote.c | 43 +++++++++
> daemon/remote.h | 8 ++
> src/Makefile.am | 37 ++++++++-
> src/remote/qemu_protocol.c | 60 +++++++++++++
> src/remote/qemu_protocol.h | 69 +++++++++++++++
> src/remote/qemu_protocol.x | 55 ++++++++++++
> src/remote/remote_driver.c | 105 +++++++++++++++++------
> 15 files changed, 665 insertions(+), 115 deletions(-)
> create mode 100644 daemon/qemu_dispatch_args.h
> create mode 100644 daemon/qemu_dispatch_prototypes.h
> create mode 100644 daemon/qemu_dispatch_ret.h
> create mode 100644 daemon/qemu_dispatch_table.h
> create mode 100755 daemon/qemu_generate_stubs.pl
> create mode 100644 src/remote/qemu_protocol.c
> create mode 100644 src/remote/qemu_protocol.h
> create mode 100644 src/remote/qemu_protocol.x
>
> diff --git a/daemon/dispatch.c b/daemon/dispatch.c
> index f024900..d36d1a1 100644
> --- a/daemon/dispatch.c
> +++ b/daemon/dispatch.c
> @@ -336,85 +336,6 @@ cleanup:
> }
>
>
> -int
> -remoteDispatchClientCall (struct qemud_server *server,
> - struct qemud_client *client,
> - struct qemud_client_message *msg);
> -
> -
> -/*
> - * @server: the unlocked server object
> - * @client: the locked client object
> - * @msg: the complete incoming message packet, with header already decoded
> - *
> - * This function gets called from qemud when it pulls a incoming
> - * remote protocol messsage off the dispatch queue for processing.
> - *
> - * The @msg parameter must have had its header decoded already by
> - * calling remoteDecodeClientMessageHeader
> - *
> - * Returns 0 if the message was dispatched, -1 upon fatal error
> - */
> -int
> -remoteDispatchClientRequest (struct qemud_server *server,
> - struct qemud_client *client,
> - struct qemud_client_message *msg)
> -{
> - int ret;
> - remote_error rerr;
> -
> - DEBUG("prog=%d ver=%d type=%d satus=%d serial=%d proc=%d",
> - msg->hdr.prog, msg->hdr.vers, msg->hdr.type,
> - msg->hdr.status, msg->hdr.serial, msg->hdr.proc);
> -
> - memset(&rerr, 0, sizeof rerr);
> -
> - /* Check version, etc. */
> - if (msg->hdr.prog != REMOTE_PROGRAM) {
> - remoteDispatchFormatError (&rerr,
> - _("program mismatch (actual %x, expected %x)"),
> - msg->hdr.prog, REMOTE_PROGRAM);
> - goto error;
> - }
> - if (msg->hdr.vers != REMOTE_PROTOCOL_VERSION) {
> - remoteDispatchFormatError (&rerr,
> - _("version mismatch (actual %x, expected %x)"),
> - msg->hdr.vers, REMOTE_PROTOCOL_VERSION);
> - goto error;
> - }
> -
> - switch (msg->hdr.type) {
> - case REMOTE_CALL:
> - return remoteDispatchClientCall(server, client, msg);
> -
> - case REMOTE_STREAM:
> - /* Since stream data is non-acked, async, we may continue to received
> - * stream packets after we closed down a stream. Just drop & ignore
> - * these.
> - */
> - VIR_INFO("Ignoring unexpected stream data serial=%d proc=%d status=%d",
> - msg->hdr.serial, msg->hdr.proc, msg->hdr.status);
> - qemudClientMessageRelease(client, msg);
> - break;
> -
> - default:
> - remoteDispatchFormatError (&rerr, _("type (%d) != REMOTE_CALL"),
> - (int) msg->hdr.type);
> - goto error;
> - }
> -
> - return 0;
> -
> -error:
> - ret = remoteSerializeReplyError(client, &rerr, &msg->hdr);
> -
> - if (ret >= 0)
> - VIR_FREE(msg);
> -
> - return ret;
> -}
> -
> -
> /*
> * @server: the unlocked server object
> * @client: the locked client object
> @@ -427,10 +348,11 @@ error:
> *
> * Returns 0 if the reply was sent, or -1 upon fatal error
> */
> -int
> +static int
> remoteDispatchClientCall (struct qemud_server *server,
> struct qemud_client *client,
> - struct qemud_client_message *msg)
> + struct qemud_client_message *msg,
> + int qemu_protocol)
> {
> XDR xdr;
> remote_error rerr;
> @@ -469,7 +391,10 @@ remoteDispatchClientCall (struct qemud_server *server,
> }
> }
>
> - data = remoteGetDispatchData(msg->hdr.proc);
> + if (qemu_protocol)
> + data = qemuGetDispatchData(msg->hdr.proc);
> + else
> + data = remoteGetDispatchData(msg->hdr.proc);
>
> if (!data) {
> remoteDispatchFormatError (&rerr, _("unknown procedure: %d"),
> @@ -584,6 +509,88 @@ fatal_error:
> return -1;
> }
>
> +/*
> + * @server: the unlocked server object
> + * @client: the locked client object
> + * @msg: the complete incoming message packet, with header already decoded
> + *
> + * This function gets called from qemud when it pulls a incoming
> + * remote protocol messsage off the dispatch queue for processing.
> + *
> + * The @msg parameter must have had its header decoded already by
> + * calling remoteDecodeClientMessageHeader
> + *
> + * Returns 0 if the message was dispatched, -1 upon fatal error
> + */
> +int remoteDispatchClientRequest(struct qemud_server *server,
> + struct qemud_client *client,
> + struct qemud_client_message *msg)
> +{
> + int ret;
> + remote_error rerr;
> + int qemu_call;
> +
> + DEBUG("prog=%d ver=%d type=%d satus=%d serial=%d proc=%d",
> + msg->hdr.prog, msg->hdr.vers, msg->hdr.type,
> + msg->hdr.status, msg->hdr.serial, msg->hdr.proc);
> +
> + memset(&rerr, 0, sizeof rerr);
> +
> + /* Check version, etc. */
> + if (msg->hdr.prog == REMOTE_PROGRAM)
> + qemu_call = 0;
> + else if (msg->hdr.prog == QEMU_PROGRAM)
> + qemu_call = 1;
> + else {
> + remoteDispatchFormatError (&rerr,
> + _("program mismatch (actual %x, expected %x or %x)"),
> + msg->hdr.prog, REMOTE_PROGRAM, QEMU_PROGRAM);
> + goto error;
> + }
> +
> + 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, qemu_call);
> +
> + case REMOTE_STREAM:
> + /* Since stream data is non-acked, async, we may continue to received
> + * stream packets after we closed down a stream. Just drop & ignore
> + * these.
> + */
> + VIR_INFO("Ignoring unexpected stream data serial=%d proc=%d status=%d",
> + msg->hdr.serial, msg->hdr.proc, msg->hdr.status);
> + qemudClientMessageRelease(client, msg);
> + break;
> +
> + default:
> + remoteDispatchFormatError (&rerr, _("type (%d) != REMOTE_CALL"),
> + (int) msg->hdr.type);
> + goto error;
> + }
> +
> + return 0;
> +
> +error:
> + ret = remoteSerializeReplyError(client, &rerr, &msg->hdr);
> +
> + if (ret >= 0)
> + VIR_FREE(msg);
> +
> + return ret;
> +}
Can we avoid moving this function down the file - there doesn't appear to
be any compile reason for moving it & I can't see from the diff whether
you changed any code in it
> --- a/daemon/libvirtd.h
> +++ b/daemon/libvirtd.h
> @@ -45,6 +45,7 @@
> # include <rpc/types.h>
> # include <rpc/xdr.h>
> # include "remote_protocol.h"
> +# include "qemu_protocol.h"
I'm thinking this is possibly redundant - I feel it should be possible
to just include it in either the remote.c or dispatch.c file where
it is needed. Indeed the same probably applies for remote_protocol.h
too
> diff --git a/daemon/qemu_generate_stubs.pl b/daemon/qemu_generate_stubs.pl
> new file mode 100755
> index 0000000..870b5c5
> --- /dev/null
> +++ b/daemon/qemu_generate_stubs.pl
> @@ -0,0 +1,170 @@
> +#!/usr/bin/perl -w
> +#
> +# This script parses remote_protocol.x and produces lots of boilerplate
> +# code for both ends of the remote connection.
> +#
> +# By Richard Jones <rjones@xxxxxxxxxx>
IIUC, the only difference between this script & remote_generate_stubs.pl
is the method name prefix they're matching on. Could we just pass in the
desired method name prfix from the Makefile.am rule, avoiding the need
to duplicate the generator code ?
> +#endif /* !_RP_QEMU_H_RPCGEN */
> diff --git a/src/remote/qemu_protocol.x b/src/remote/qemu_protocol.x
> new file mode 100644
> index 0000000..e0593a8
> --- /dev/null
> +++ b/src/remote/qemu_protocol.x
> @@ -0,0 +1,55 @@
> +/* -*- c -*-
> + * qemu_protocol.x: private protocol for communicating between
> + * remote_internal driver and libvirtd. This protocol is
> + * internal and may change at any time.
> + *
> + * Copyright (C) 2010 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Author: Chris Lalancette <clalance@xxxxxxxxxx>
> + */
> +
> +%#include "internal.h"
> +%#include "remote_protocol.h"
> +%#include <arpa/inet.h>
> +
> +/*----- Protocol. -----*/
> +struct qemu_monitor_command_args {
> + remote_nonnull_domain domain;
> + remote_nonnull_string cmd;
> + int flags;
> +};
> +
> +struct qemu_monitor_command_ret {
> + remote_nonnull_string result;
> +};
> +
> +/* 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
> +};
> +
> +struct qemu_message_header {
> + unsigned prog; /* QEMU_PROGRAM */
> + unsigned vers; /* QEMU_PROTOCOL_VERSION */
> + qemu_procedure proc; /* QEMU_PROC_x */
> + remote_message_type type;
> + unsigned serial; /* Serial number of message. */
> + remote_message_status status;
> +};
I don't think we need, or should have, a separate message header definition
for QEMU. The daemon has to read & look at the complete header before it
can determine that this is the QEMU protocol vs remote protocol. Thus they
must be identical, or bad stuff will happen.
I guess the reason was for the 'qemu_procedure' enum, but could we just
turn that field into a plain 'unsigned' (which is the same byte size).
Despite all the comments, this all looks architecturally sound to me.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list