Re: [Qemu-devel] [RFC][PATCH v4 04/18] virtagent: base RPC client definitions

[Jes Sorensen <Jes.Sorensen@redhat.com>]

On 11/16/10 17:01, Michael Roth wrote:
> diff --git a/monitor.c b/monitor.c
> index 8cee35d..cb81cd7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -42,6 +42,7 @@
>  #include "audio/audio.h"
>  #include "disas.h"
>  #include "balloon.h"
> +#include "virtagent.h"
>  #include "qemu-timer.h"
>  #include "migration.h"
>  #include "kvm.h"

You are adding an include here without modifying any code to actually
use something from this file.

> +static int va_client_ready(void)
> +{
> +    if (client_state != NULL && client_state->vp != NULL
> +        && client_state->socket_path != NULL) {

Please put the && up on the previous line, makes it easier for a reader
to notice that the next line is part of the first portion.

> +        return 0;
> +    }
> +
> +    return -1;
> +}

-EAGAIN? -EBUSY?

> +static void va_set_capabilities(QList *qlist)
> +{
> +    TRACE("called");
> +
> +    if (client_state == NULL) {
> +        LOG("client is uninitialized, unable to set capabilities");
> +        return;
> +    }

Why no error value returned here?

> +int va_client_init(VPDriver *vp_drv, bool is_host)
> +{
> +    const char *service_id, *path;
> +    QemuOpts *opts;
> +    int fd, ret;
> +
> +    if (client_state) {
> +        LOG("virtagent client already initialized");
> +        return -1;
> +    }

-1 again :(

> +    /* setup listening socket to forward connections over */
> +    opts = qemu_opts_create(qemu_find_opts("net"), "va_client_opts", 0);
> +    qemu_opt_set(opts, "path", path);
> +    fd = unix_listen_opts(opts);
> +    qemu_opts_del(opts);
> +    if (fd < 0) {
> +        LOG("error setting up listening socket");
> +        goto out_bad;
> +    }
> +
> +    /* tell virtproxy to forward connections to this socket to
> +     * virtagent service on other end
> +     */
> +    ret = vp_set_oforward(vp_drv, fd, service_id);
> +    if (ret < 0) {
> +        LOG("error setting up virtproxy iforward");
> +        goto out_bad;
> +    }
> +
> +    return 0;
> +out_bad:
> +    qemu_free(client_state);
> +    client_state = NULL;
> +    return -1;
> +}

You know why you ended up here, please pass that information up the stack.

> +static int rpc_has_error(xmlrpc_env *env)
> +{
> +    if (env->fault_occurred) {
> +        LOG("An RPC error has occurred (%i): %s\n", env->fault_code, env->fault_string);
> +        //qerror_report(QERR_RPC_FAILED, env->fault_code, env->fault_string);
> +        return -1;
> +    }
> +    return 0;
> +}

-1 again

> +/*
> + * Get a connected socket that can be used to make an RPC call
> + * This interface will eventually return the connected virtproxy socket for the
> + * virt-agent channel
> + */
> +static int get_transport_fd(void)
> +{
> +    /* TODO: eventually this will need a path that is unique to other
> +     * instances of qemu-vp/qemu. for the integrated qemu-vp we should
> +     * explore the possiblity of not requiring a unix socket under the
> +     * covers, as well as having client init code set up the oforward
> +     * for the service rather than qemu-vp
> +     */
> +    int ret;
> +    int fd = unix_connect(client_state->socket_path);
> +    if (fd < 0) {
> +        LOG("failed to connect to virtagent service");
> +    }
> +    ret = fcntl(fd, F_GETFL);
> +    ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
> +    return fd;
> +}

You are forgetting to check the return from fcntl() for errors.

> +static int rpc_execute(xmlrpc_env *const env, const char *function,
> +                       xmlrpc_value *params, VARPCData *rpc_data)
> +{
> +    xmlrpc_mem_block *call_xml;
> +    int fd, ret;
> +
> +    ret = va_client_ready();
> +    if (ret < 0) {
> +        LOG("client in uninitialized state, unable to execute RPC");
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    if (!va_has_capability(function)) {
> +        LOG("guest agent does not have required capability");
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    fd = get_transport_fd();
> +    if (fd < 0) {
> +        LOG("invalid fd");
> +        ret = -1;
> +        goto out;
> +    }

You just got a proper error value back, why replace it with -1? It is
all over this function.

> diff --git a/virtagent.h b/virtagent.h
> new file mode 100644
> index 0000000..53efa29
> --- /dev/null
> +++ b/virtagent.h
> +#define GUEST_AGENT_PATH_CLIENT "/tmp/virtagent-guest-client.sock"
> +#define HOST_AGENT_PATH_CLIENT "/tmp/virtagent-host-client.sock"
> +#define VA_MAX_CHUNK_SIZE 4096 /* max bytes at a time for get/send file */

Config file please!

Jes