Patchwork [RFC,v6,07/23] virtagent: base server definitions

login
register
mail settings
Submitter Michael Roth
Date Jan. 17, 2011, 1:15 p.m.
Message ID <1295270117-24760-8-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/79167/
State New
Headers show

Comments

Michael Roth - Jan. 17, 2011, 1:15 p.m.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtagent-server.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtagent-server.h |   34 ++++++++++++++++
 2 files changed, 145 insertions(+), 0 deletions(-)
 create mode 100644 virtagent-server.c
 create mode 100644 virtagent-server.h
Jes Sorensen - Jan. 21, 2011, 4:38 p.m.
> diff --git a/virtagent-server.h b/virtagent-server.h
> new file mode 100644
> index 0000000..9f68921
> --- /dev/null
> +++ b/virtagent-server.h
> @@ -0,0 +1,34 @@
> +/*
> + * virt-agent - host/guest RPC daemon functions
> + *
> + * Copyright IBM Corp. 2010
> + *
> + * Authors:
> + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <xmlrpc-c/base.h>
> +#include <xmlrpc-c/server.h>
> +
> +#define GUEST_AGENT_SERVICE_ID "virtagent"
> +#define GUEST_AGENT_PATH "/tmp/virtagent-guest.sock"
> +#define HOST_AGENT_SERVICE_ID "virtagent-host"
> +#define HOST_AGENT_PATH "/tmp/virtagent-host.sock"
> +#define VA_GETFILE_MAX 1 << 30
> +#define VA_FILEBUF_LEN 16384
> +#define VA_DMESG_LEN 16384

I really don't like these hard coded constants - you you have a command
line interface allowing for the change of the sockets and file names?
Otherwise you'll hit problems on the host side with concurrent runs of qemu.

I really would like to see the dmesg stuff removed too for now as we
discussed earlier.

Cheers,
Jes
Michael Roth - Jan. 21, 2011, 5:55 p.m.
On 01/21/2011 10:38 AM, Jes Sorensen wrote:
>> diff --git a/virtagent-server.h b/virtagent-server.h
>> new file mode 100644
>> index 0000000..9f68921
>> --- /dev/null
>> +++ b/virtagent-server.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * virt-agent - host/guest RPC daemon functions
>> + *
>> + * Copyright IBM Corp. 2010
>> + *
>> + * Authors:
>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include<xmlrpc-c/base.h>
>> +#include<xmlrpc-c/server.h>
>> +
>> +#define GUEST_AGENT_SERVICE_ID "virtagent"
>> +#define GUEST_AGENT_PATH "/tmp/virtagent-guest.sock"
>> +#define HOST_AGENT_SERVICE_ID "virtagent-host"
>> +#define HOST_AGENT_PATH "/tmp/virtagent-host.sock"
>> +#define VA_GETFILE_MAX 1<<  30
>> +#define VA_FILEBUF_LEN 16384
>> +#define VA_DMESG_LEN 16384
>
> I really don't like these hard coded constants - you you have a command
> line interface allowing for the change of the sockets and file names?
> Otherwise you'll hit problems on the host side with concurrent runs of qemu.

Yup, that's one of the TODOs. In terms of configuration we can add 
parameters to the chardev to override these, but the goal here is sane 
defaults to avoid unnecessarily complicated invocations.

>
> I really would like to see the dmesg stuff removed too for now as we
> discussed earlier.

I think as a development/support tool it has a recently strong use case, 
even given it's limitations (which are not so bad....we retrieve up to a 
max of 16KB, possibly less depending on guest configuration, so it's not 
entirely predictable, but it's not dangerous. It's platform-specific, 
but that's handled by capabilities negotiation).

I just don't really see the downside to keeping it in.

>
> Cheers,
> Jes
Jes Sorensen - Jan. 24, 2011, 10:16 a.m.
On 01/21/11 18:55, Michael Roth wrote:
> On 01/21/2011 10:38 AM, Jes Sorensen wrote:
>>> +#include<xmlrpc-c/base.h>
>>> +#include<xmlrpc-c/server.h>
>>> +
>>> +#define GUEST_AGENT_SERVICE_ID "virtagent"
>>> +#define GUEST_AGENT_PATH "/tmp/virtagent-guest.sock"
>>> +#define HOST_AGENT_SERVICE_ID "virtagent-host"
>>> +#define HOST_AGENT_PATH "/tmp/virtagent-host.sock"
>>> +#define VA_GETFILE_MAX 1<<  30
>>> +#define VA_FILEBUF_LEN 16384
>>> +#define VA_DMESG_LEN 16384
>>
>> I really don't like these hard coded constants - you you have a command
>> line interface allowing for the change of the sockets and file names?
>> Otherwise you'll hit problems on the host side with concurrent runs of
>> qemu.
> 
> Yup, that's one of the TODOs. In terms of configuration we can add
> parameters to the chardev to override these, but the goal here is sane
> defaults to avoid unnecessarily complicated invocations.

As a sane default, using <name>.pid or something along those lines is
better. It is very common to run more than one qemu instance at a time.

>> I really would like to see the dmesg stuff removed too for now as we
>> discussed earlier.
> 
> I think as a development/support tool it has a recently strong use case,
> even given it's limitations (which are not so bad....we retrieve up to a
> max of 16KB, possibly less depending on guest configuration, so it's not
> entirely predictable, but it's not dangerous. It's platform-specific,
> but that's handled by capabilities negotiation).

There is plenty of good ways to do the same thing, copy file to host,
then view is just as easy and can be scripted, without the security
implications of having it inline.

> I just don't really see the downside to keeping it in.

It's obviously contentious, and it is not core functionality. In order
to get the patches adapted upstream it would easy the process to remove
it and keep it as a separate patch.

Cheers,
Jes
Michael Roth - Jan. 24, 2011, 4:51 p.m.
On 01/24/2011 04:16 AM, Jes Sorensen wrote:
> On 01/21/11 18:55, Michael Roth wrote:
>> On 01/21/2011 10:38 AM, Jes Sorensen wrote:
>>>> +#include<xmlrpc-c/base.h>
>>>> +#include<xmlrpc-c/server.h>
>>>> +
>>>> +#define GUEST_AGENT_SERVICE_ID "virtagent"
>>>> +#define GUEST_AGENT_PATH "/tmp/virtagent-guest.sock"
>>>> +#define HOST_AGENT_SERVICE_ID "virtagent-host"
>>>> +#define HOST_AGENT_PATH "/tmp/virtagent-host.sock"
>>>> +#define VA_GETFILE_MAX 1<<   30
>>>> +#define VA_FILEBUF_LEN 16384
>>>> +#define VA_DMESG_LEN 16384
>>>
>>> I really don't like these hard coded constants - you you have a command
>>> line interface allowing for the change of the sockets and file names?
>>> Otherwise you'll hit problems on the host side with concurrent runs of
>>> qemu.
>>
>> Yup, that's one of the TODOs. In terms of configuration we can add
>> parameters to the chardev to override these, but the goal here is sane
>> defaults to avoid unnecessarily complicated invocations.
>
> As a sane default, using<name>.pid or something along those lines is
> better. It is very common to run more than one qemu instance at a time.
>

Sorry, wasn't clear here. Using a pid by default to differentiate 
per-qemu instances of virtagent is a specific TODO, but it is currently 
configurable via the commandline as well:

qemu -chardev virtagent,path=/tmp/qemu-guest1-virtagent.sock,...

>>> I really would like to see the dmesg stuff removed too for now as we
>>> discussed earlier.
>>
>> I think as a development/support tool it has a recently strong use case,
>> even given it's limitations (which are not so bad....we retrieve up to a
>> max of 16KB, possibly less depending on guest configuration, so it's not
>> entirely predictable, but it's not dangerous. It's platform-specific,
>> but that's handled by capabilities negotiation).
>
> There is plenty of good ways to do the same thing, copy file to host,
> then view is just as easy and can be scripted, without the security
> implications of having it inline.
>
>> I just don't really see the downside to keeping it in.
>
> It's obviously contentious, and it is not core functionality. In order
> to get the patches adapted upstream it would easy the process to remove
> it and keep it as a separate patch.

Fair enough, the proposed copyfile replacement would be suitable as well.

My main concern is stripping away too much functionality for the initial 
merge, since guest-initiated shutdown is all we'd really have left 
lacking viewdmesg/viewfile.

Would it be better to get copyfile in for the initial set of patches, or 
as a subsequent set?

>
> Cheers,
> Jes
Jes Sorensen - Jan. 24, 2011, 5:04 p.m.
On 01/24/11 17:51, Michael Roth wrote:
> On 01/24/2011 04:16 AM, Jes Sorensen wrote:
>> It's obviously contentious, and it is not core functionality. In order
>> to get the patches adapted upstream it would easy the process to remove
>> it and keep it as a separate patch.
> 
> Fair enough, the proposed copyfile replacement would be suitable as well.
> 
> My main concern is stripping away too much functionality for the initial
> merge, since guest-initiated shutdown is all we'd really have left
> lacking viewdmesg/viewfile.
> 
> Would it be better to get copyfile in for the initial set of patches, or
> as a subsequent set?

Having copyfile would be good in an initial release too - however we
should probably review it in the light of Dan's suggestion of using
libguestfs.

I am working on freeze/thaw support which I hope to have ready within a
couple of days. It would be nice to get in, in an early release as well.

Cheers,
Jes

Patch

diff --git a/virtagent-server.c b/virtagent-server.c
new file mode 100644
index 0000000..c38a9e0
--- /dev/null
+++ b/virtagent-server.c
@@ -0,0 +1,111 @@ 
+/*
+ * virtagent - host/guest RPC server functions
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ *  Adam Litke        <aglitke@linux.vnet.ibm.com>
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include <syslog.h>
+#include "qemu_socket.h"
+#include "virtagent-common.h"
+
+static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
+
+#define SLOG(msg, ...) do { \
+    char msg_buf[1024]; \
+    if (!va_enable_syslog) { \
+        break; \
+    } \
+    snprintf(msg_buf, 1024, msg, ## __VA_ARGS__); \
+    syslog(LOG_INFO, "virtagent, %s", msg_buf); \
+} while(0)
+
+static VAServerData *va_server_data;
+
+static bool va_server_is_enabled(void)
+{
+    return va_server_data && va_server_data->enabled;
+}
+
+int va_do_server_rpc(const char *content, size_t content_len, const char *tag)
+{
+    xmlrpc_mem_block *resp_xml;
+    int ret;
+
+    TRACE("called");
+
+    if (!va_server_is_enabled()) {
+        ret = -EBUSY;
+        goto out;
+    }
+    resp_xml = xmlrpc_registry_process_call(&va_server_data->env,
+                                            va_server_data->registry,
+                                            NULL, content, content_len);
+    if (resp_xml == NULL) {
+        LOG("error processing RPC request");
+        ret = -EINVAL;
+        goto out;
+    }
+
+    ret = va_server_job_add(resp_xml, tag);
+    if (ret != 0) {
+        LOG("error adding server job: %s", strerror(ret));
+    }
+
+out:
+    return ret;
+}
+
+typedef struct RPCFunction {
+    xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
+    const char *func_name;
+} RPCFunction;
+
+static RPCFunction guest_functions[] = {
+    { NULL, NULL }
+};
+static RPCFunction host_functions[] = {
+    { NULL, NULL }
+};
+
+static void va_register_functions(xmlrpc_env *env, xmlrpc_registry *registry,
+                                  RPCFunction *list)
+{
+    int i;
+    for (i = 0; list[i].func != NULL; ++i) {
+        TRACE("adding func: %s", list[i].func_name);
+        xmlrpc_registry_add_method(env, registry, NULL, list[i].func_name,
+                                   list[i].func, NULL);
+    }
+}
+
+int va_server_init(VAServerData *server_data, bool is_host)
+{
+    RPCFunction *func_list = is_host ? host_functions : guest_functions;
+
+    va_enable_syslog = !is_host; /* enable logging for guest agent */
+    xmlrpc_env_init(&server_data->env);
+    server_data->registry = xmlrpc_registry_new(&server_data->env);
+    va_register_functions(&server_data->env, server_data->registry, func_list);
+    server_data->enabled = true;
+    server_data->is_host = true;
+    va_server_data = server_data;
+
+    return 0;
+}
+
+int va_server_close(void)
+{
+    if (va_server_data != NULL) {
+        xmlrpc_registry_free(va_server_data->registry);
+        xmlrpc_env_clean(&va_server_data->env);
+        va_server_data = NULL;
+    }
+    return 0;
+}
diff --git a/virtagent-server.h b/virtagent-server.h
new file mode 100644
index 0000000..9f68921
--- /dev/null
+++ b/virtagent-server.h
@@ -0,0 +1,34 @@ 
+/*
+ * virt-agent - host/guest RPC daemon functions
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <xmlrpc-c/base.h>
+#include <xmlrpc-c/server.h>
+
+#define GUEST_AGENT_SERVICE_ID "virtagent"
+#define GUEST_AGENT_PATH "/tmp/virtagent-guest.sock"
+#define HOST_AGENT_SERVICE_ID "virtagent-host"
+#define HOST_AGENT_PATH "/tmp/virtagent-host.sock"
+#define VA_GETFILE_MAX 1 << 30
+#define VA_FILEBUF_LEN 16384
+#define VA_DMESG_LEN 16384
+
+typedef struct VAServerData {
+    xmlrpc_env env;
+    xmlrpc_registry *registry;
+    bool enabled;
+    bool is_host;
+} VAServerData;
+
+int va_server_init(VAServerData *server_data, bool is_host);
+int va_server_close(void);
+int va_do_server_rpc(const char *content, size_t content_len, const char tag[64]);