Re: [Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks

[Michael Roth]

On 12/07/2010 07:44 AM, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
> > +static void va_server_read_cb(const char *content, size_t content_len)
> > +{
> > +    xmlrpc_mem_block *resp_xml;
> > +    VAServerData *server_data =&va_state->server_data;
> > +    int ret;
> > +
> > +    TRACE("called");
> > +    resp_xml = xmlrpc_registry_process_call(&server_data->env,
> > +                                            server_data->registry,
> > +                                            NULL, content, content_len);
> > +    if (resp_xml == NULL) {
> > +        LOG("error processing RPC request");
> > +        goto out_bad;
> > +    }
> > +
> > +    ret = va_server_job_add(resp_xml);
> > +    if (ret != 0) {
> > +        LOG("error adding server job: %s", strerror(ret));
> > +    }
> > +
> > +    return;
> > +out_bad:
> > +    /* TODO: should reset state here */
> > +    return;
>
> Looks like some missing error handling is needed here?
>
> > +static void va_rpc_parse_hdr(VAHTState *s)
> > +{
> > +    int i, line_pos = 0;
> > +    bool first_line = true;
> > +    char line_buf[4096];
>
> In 03/21 you defined VA_HDR_LEN_MAX to 4096, here you hard code the
> value .... sounds like something begging to go wrong.
>
> > +static int va_end_of_header(char *buf, int end_pos)
> > +{
> > +    return !strncmp(buf+(end_pos-2), "\n\r\n", 3);
> > +}
>
> Maybe I am missing something here, but it looks like you do a strncmp to
> a char that is one past the end of the buffer, or? If this is
> intentional, please document it.

buf+end_pos points to the last char we read (rather than being an offset 
to the current position). So it stops comparing when it reaches 
buf+end_pos (buf=0 + end_pos=2 implies 3 characters)

For some reason this confused the hell out of me when I looked over it 
again as well. Alternatively I can do:

static int va_end_of_header(char *buf, int end_pos)
{
    return !strncmp(buf+(end_pos-2), "\n\r\n", 3);
}
...
va_end_of_header(s->hdr, s->hdr_pos - 1)

->

static int va_end_of_header(char *buf, int cur_pos)
{
    return !strncmp(buf+(cur_pos-3), "\n\r\n", 3);
}
...
va_end_of_header(s->hdr, s->hdr_pos);

It does seem easier to parse...

> All this http parsing code leaves the question open why you do it
> manually, instead of relying on a library?

Something like libcurl? At some point we didn't attempt to use libraries 
provide by xmlrpc-c (which uses libcurl for http transport) for the 
client and server. The problem there is that libcurl really wants and 
tcp socket read and write from, whereas we need to support tcp/unix 
sockets on the host side and isa/virtio serial ports on the guest side.

Even assuming we could hook in wrappers for these other types of 
sockets/channels, there's also the added complexity since dropping 
virtproxy of multiplexing HTTP/RPCs using a single stream, whereas 
something like libcurl would, understandably, assume it has a dedicated 
stream to read/write from. So we wouldn't really save any work or code, 
unfortunately.

> Cheers,
> Jes