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