[Qemu-devel] Re: [RFC][PATCH v6 07/23] virtagent: base server definitions

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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