Re: [Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs

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

On 12/07/2010 07:38 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
>> This implements a simple state machine to manage client/server rpc
>> jobs being multiplexed over a single channel.
>>
>> A client job consists of sending an rpc request, reading an
>> rpc response, then making the appropriate callbacks. We allow one
>> client job to be processed at a time, which will make the following
>> state transitions:
>>
>> VA_CLIENT_IDLE ->  VA_CLIENT_SEND (job queued, send channel open)
>> VA_CLIENT_SEND ->  VA_CLIENT_WAIT (request sent, awaiting response)
>> VA_CLIENT_WAIT ->  VA_CLIENT_IDLE (response recieved, callbacks made)
>>
>> A server job consists of recieving an rpc request, generating a
>> response, then sending the response. We expect to receive one server
>> request at a time due to the 1 at a time restriction for client jobs.
>> Server jobs make the following transitions:
>>
>> VA_SERVER_IDLE ->  VA_SERVER_WAIT (recieved/executed request, send
>> channel busy, response deferred)
>> VA_SERVER_IDLE ->  VA_SERVER_SEND (recieved/executed request, send
>> channel open, sending response)
>> VA_SERVER_WAIT ->  VA_SERVER_SEND (send channel now open, sending
>> response)
>> VA_SERVER_SEND ->  VA_SERVER_IDLE (response sent)
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>
> As mentioned before, I really don't understand why this is part of QEMU,
> the guest agent really should be able to run totally outside of QEMU.
>
>> +
>> +#define DEBUG_VA
>> +
>> +#ifdef DEBUG_VA
>> +#define TRACE(msg, ...) do { \
>> +    fprintf(stderr, "%s:%s():L%d: " msg "\n", \
>> +            __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
>> +} while(0)
>> +#else
>> +#define TRACE(msg, ...) \
>> +    do { } while (0)
>> +#endif
>> +
>> +#define LOG(msg, ...) do { \
>> +    fprintf(stderr, "%s:%s(): " msg "\n", \
>> +            __FILE__, __FUNCTION__, ## __VA_ARGS__); \
>> +} while(0)
>
> This must be like the 217th copy of these functions, could you please
> use some of the code that is already in the tree, and make it generic if
> needed.
>
>> +
>> +#define VERSION "1.0"
>> +#define EOL "\r\n"
>> +
>> +#define VA_HDR_LEN_MAX 4096 /* http header limit */
>> +#define VA_CONTENT_LEN_MAX 2*1024*1024 /* rpc/http send limit */
>> +#define VA_CLIENT_JOBS_MAX 5 /* max client rpcs we can queue */
>> +#define VA_SERVER_JOBS_MAX 1 /* max server rpcs we can queue */
>
> As mentioned last time, please make this stuff configurable and not hard
> coded.
>

Yup, definitely on the TODO. Should be in the next round.

> Cheers,
> Jes
>