[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC][PATCH v4 04/18] virtagent: base RPC client defini
From: |
Jes Sorensen |
Subject: |
Re: [Qemu-devel] [RFC][PATCH v4 04/18] virtagent: base RPC client definitions |
Date: |
Thu, 18 Nov 2010 15:10:20 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) Gecko/20101103 Fedora/1.0-0.33.b2pre.fc14 Lightning/1.0b3pre Thunderbird/3.1.6 |
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
- [Qemu-devel] [RFC][PATCH v4 00/18] virtagent: host/guest RPC communication agent, Michael Roth, 2010/11/16
- [Qemu-devel] [RFC][PATCH v4 02/18] virtagent: base definitions for host/guest RPC server, Michael Roth, 2010/11/16
- [Qemu-devel] [RFC][PATCH v4 01/18] virtagent: add common rpc transport defs, Michael Roth, 2010/11/16
- [Qemu-devel] [RFC][PATCH v4 03/18] virtagent: qemu-vp, integrate virtagent server, Michael Roth, 2010/11/16
- [Qemu-devel] [RFC][PATCH v4 04/18] virtagent: base RPC client definitions, Michael Roth, 2010/11/16
- Re: [Qemu-devel] [RFC][PATCH v4 04/18] virtagent: base RPC client definitions,
Jes Sorensen <=
- [Qemu-devel] [RFC][PATCH v4 05/18] virtagent: add getfile RPC, Michael Roth, 2010/11/16
- [Qemu-devel] [RFC][PATCH v4 06/18] virtagent: add agent_viewfile command, Michael Roth, 2010/11/16
- [Qemu-devel] [RFC][PATCH v4 07/18] virtagent: add getdmesg RPC, Michael Roth, 2010/11/16
- [Qemu-devel] [RFC][PATCH v4 08/18] virtagent: add agent_viewdmesg command, Michael Roth, 2010/11/16
- [Qemu-devel] [RFC][PATCH v4 09/18] virtagent: add va_shutdown RPC, Michael Roth, 2010/11/16
- [Qemu-devel] [RFC][PATCH v4 12/18] virtagent: add agent_ping monitor command, Michael Roth, 2010/11/16