On Tue, Apr 13, 2010 at 02:36:48PM -0400, Chris Lalancette wrote:
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- 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@redhat.com>
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@redhat.com> + */ + +%#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 :|