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

Re: [libvirt] [RFC PATCH 3/5] Qemu remote protocol.



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 :|


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