Patchwork [RFC,v5,09/21] virtagent: add va.getdmesg RPC

login
register
mail settings
Submitter Michael Roth
Date Dec. 3, 2010, 6:03 p.m.
Message ID <1291399402-20366-10-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/74200/
State New
Headers show

Comments

Michael Roth - Dec. 3, 2010, 6:03 p.m.
Add RPC to view guest dmesg output.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtagent-server.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)
Adam Litke - Dec. 6, 2010, 10:25 p.m.
On Fri, 2010-12-03 at 12:03 -0600, Michael Roth wrote:
> +/* va_getdmesg(): return dmesg output
> + * rpc return values:
> + *   - dmesg output as a string
> + */
> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
> +                              xmlrpc_value *param,
> +                              void *user_data)
> +{
> +    char *dmesg_buf = NULL, cmd[256];
> +    int ret;
> +    xmlrpc_value *result = NULL;
> +    FILE *pipe;
> +
> +    SLOG("va_getdmesg()");
> +
> +    dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
> +    sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
> +
> +    pipe = popen(cmd, "r");
> +    if (pipe == NULL) {
> +        LOG("popen failed: %s", strerror(errno));
> +        xmlrpc_faultf(env, "popen failed: %s", strerror(errno));
> +        goto EXIT_NOCLOSE;
> +    }
> +
> +    ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
> +    if (!ferror(pipe)) {
> +        dmesg_buf[ret] = '\0';
> +        TRACE("dmesg:\n%s", dmesg_buf);
> +        result = xmlrpc_build_value(env, "s", dmesg_buf);
> +    } else {
> +        LOG("fread failed");
> +        xmlrpc_faultf(env, "popen failed: %s", strerror(errno));
> +    }
> +
> +    pclose(pipe);
> +EXIT_NOCLOSE:

I think goto labels are supposed to be lower-case.
Jes Sorensen - Dec. 7, 2010, 2:37 p.m.
On 12/03/10 19:03, Michael Roth wrote:
> Add RPC to view guest dmesg output.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtagent-server.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 46 insertions(+), 0 deletions(-)
> 
> diff --git a/virtagent-server.c b/virtagent-server.c
> index a430b58..aac8f70 100644
> --- a/virtagent-server.c
> +++ b/virtagent-server.c
> @@ -83,6 +83,50 @@ EXIT_CLOSE_BAD:
>      return result;
>  }
>  
> +/* va_getdmesg(): return dmesg output
> + * rpc return values:
> + *   - dmesg output as a string
> + */
> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
> +                              xmlrpc_value *param,
> +                              void *user_data)
> +{
> +    char *dmesg_buf = NULL, cmd[256];
> +    int ret;
> +    xmlrpc_value *result = NULL;
> +    FILE *pipe;
> +
> +    SLOG("va_getdmesg()");
> +
> +    dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
> +    sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);

What happens if the guest's dmesg buffer is larger than your hardcoded
value?

Jes
Michael Roth - Dec. 7, 2010, 5:32 p.m.
On 12/07/2010 08:37 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
>> Add RPC to view guest dmesg output.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   virtagent-server.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 46 insertions(+), 0 deletions(-)
>>
>> diff --git a/virtagent-server.c b/virtagent-server.c
>> index a430b58..aac8f70 100644
>> --- a/virtagent-server.c
>> +++ b/virtagent-server.c
>> @@ -83,6 +83,50 @@ EXIT_CLOSE_BAD:
>>       return result;
>>   }
>>
>> +/* va_getdmesg(): return dmesg output
>> + * rpc return values:
>> + *   - dmesg output as a string
>> + */
>> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
>> +                              xmlrpc_value *param,
>> +                              void *user_data)
>> +{
>> +    char *dmesg_buf = NULL, cmd[256];
>> +    int ret;
>> +    xmlrpc_value *result = NULL;
>> +    FILE *pipe;
>> +
>> +    SLOG("va_getdmesg()");
>> +
>> +    dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
>> +    sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
>
> What happens if the guest's dmesg buffer is larger than your hardcoded
> value?

It'll end up getting truncated by the fread() later:

ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);

That's where the dmesg -s VA_DMESG_LEN comes into play, it should size 
things such that we can buffer up till the end of the dmesg output.

This param is kind of quirky though, size doesn't seem to have an affect 
for anything below 4KB, but if we stick with VA_DMESG_LEN >= 4KB this 
should cover us, unless it's a distro-specific. But it should blow 
anything up, at least.

>
> Jes
>
>
Jes Sorensen - Dec. 8, 2010, 7:22 p.m.
On 12/07/10 18:32, Michael Roth wrote:
> On 12/07/2010 08:37 AM, Jes Sorensen wrote:
>> On 12/03/10 19:03, Michael Roth wrote:
>>> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
>>> +                              xmlrpc_value *param,
>>> +                              void *user_data)
>>> +{
>>> +    char *dmesg_buf = NULL, cmd[256];
>>> +    int ret;
>>> +    xmlrpc_value *result = NULL;
>>> +    FILE *pipe;
>>> +
>>> +    SLOG("va_getdmesg()");
>>> +
>>> +    dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
>>> +    sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
>>
>> What happens if the guest's dmesg buffer is larger than your hardcoded
>> value?
> 
> It'll end up getting truncated by the fread() later:
> 
> ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
> 
> That's where the dmesg -s VA_DMESG_LEN comes into play, it should size
> things such that we can buffer up till the end of the dmesg output.
> 
> This param is kind of quirky though, size doesn't seem to have an affect
> for anything below 4KB, but if we stick with VA_DMESG_LEN >= 4KB this
> should cover us, unless it's a distro-specific. But it should blow
> anything up, at least.

I am wary of these hard coded constants. Isn't there a way to set the
kernel's dmesg buffer size, or is that only a compile time option?

Cheers,
Jes
Michael Roth - Dec. 9, 2010, 9:15 p.m.
On 12/08/2010 01:22 PM, Jes Sorensen wrote:
> On 12/07/10 18:32, Michael Roth wrote:
>> On 12/07/2010 08:37 AM, Jes Sorensen wrote:
>>> On 12/03/10 19:03, Michael Roth wrote:
>>>> +static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
>>>> +                              xmlrpc_value *param,
>>>> +                              void *user_data)
>>>> +{
>>>> +    char *dmesg_buf = NULL, cmd[256];
>>>> +    int ret;
>>>> +    xmlrpc_value *result = NULL;
>>>> +    FILE *pipe;
>>>> +
>>>> +    SLOG("va_getdmesg()");
>>>> +
>>>> +    dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
>>>> +    sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
>>>
>>> What happens if the guest's dmesg buffer is larger than your hardcoded
>>> value?
>>
>> It'll end up getting truncated by the fread() later:
>>
>> ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
>>
>> That's where the dmesg -s VA_DMESG_LEN comes into play, it should size
>> things such that we can buffer up till the end of the dmesg output.
>>
>> This param is kind of quirky though, size doesn't seem to have an affect
>> for anything below 4KB, but if we stick with VA_DMESG_LEN>= 4KB this
>> should cover us, unless it's a distro-specific. But it should blow
>> anything up, at least.
>
> I am wary of these hard coded constants. Isn't there a way to set the
> kernel's dmesg buffer size, or is that only a compile time option?
>

 From what I can tell it's a compile-time option. I originally had 
dmesg_len as a param the host could pass to the guest, but it has no 
effect if the buffer is smaller, which might cause unnecessary confusion.

> Cheers,
> Jes
Jes Sorensen - Dec. 10, 2010, 6:46 a.m.
On 12/09/10 22:15, Michael Roth wrote:
> On 12/08/2010 01:22 PM, Jes Sorensen wrote:
>>> This param is kind of quirky though, size doesn't seem to have an affect
>>> for anything below 4KB, but if we stick with VA_DMESG_LEN>= 4KB this
>>> should cover us, unless it's a distro-specific. But it should blow
>>> anything up, at least.
>>
>> I am wary of these hard coded constants. Isn't there a way to set the
>> kernel's dmesg buffer size, or is that only a compile time option?
>>
> 
> From what I can tell it's a compile-time option. I originally had
> dmesg_len as a param the host could pass to the guest, but it has no
> effect if the buffer is smaller, which might cause unnecessary confusion.

You are correct! I checked, there were people talking about a
configurable option but so far it is not in place.

I would still rather have this go via a file transfer to the host, and
put the output in a tmpfile.

I am still against adding viewfile commands to the monitor though. You
get a bad control character in the string and your console is messed up.
Hit CTRL-c and you lost your guest.

Cheers,
Jes

Patch

diff --git a/virtagent-server.c b/virtagent-server.c
index a430b58..aac8f70 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -83,6 +83,50 @@  EXIT_CLOSE_BAD:
     return result;
 }
 
+/* va_getdmesg(): return dmesg output
+ * rpc return values:
+ *   - dmesg output as a string
+ */
+static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
+                              xmlrpc_value *param,
+                              void *user_data)
+{
+    char *dmesg_buf = NULL, cmd[256];
+    int ret;
+    xmlrpc_value *result = NULL;
+    FILE *pipe;
+
+    SLOG("va_getdmesg()");
+
+    dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
+    sprintf(cmd, "dmesg -s %d", VA_DMESG_LEN);
+
+    pipe = popen(cmd, "r");
+    if (pipe == NULL) {
+        LOG("popen failed: %s", strerror(errno));
+        xmlrpc_faultf(env, "popen failed: %s", strerror(errno));
+        goto EXIT_NOCLOSE;
+    }
+
+    ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
+    if (!ferror(pipe)) {
+        dmesg_buf[ret] = '\0';
+        TRACE("dmesg:\n%s", dmesg_buf);
+        result = xmlrpc_build_value(env, "s", dmesg_buf);
+    } else {
+        LOG("fread failed");
+        xmlrpc_faultf(env, "popen failed: %s", strerror(errno));
+    }
+
+    pclose(pipe);
+EXIT_NOCLOSE:
+    if (dmesg_buf) {
+        qemu_free(dmesg_buf);
+    }
+
+    return result;
+}
+
 typedef struct RPCFunction {
     xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
     const char *func_name;
@@ -91,6 +135,8 @@  typedef struct RPCFunction {
 static RPCFunction guest_functions[] = {
     { .func = va_getfile,
       .func_name = "va.getfile" },
+    { .func = va_getdmesg,
+      .func_name = "va.getdmesg" },
     { NULL, NULL }
 };
 static RPCFunction host_functions[] = {