[vlc-devel] [PATCH] network: implement an API for HTTP requests.

[Felix Abecassis]

2014-02-05 Denis Charmet <typx at dinauz.org>:
> Hi,
>
> Since it's quite redundant with access/http, it would be good to avoid
> code duplication.
My goal is to implement a new API in the core and afterwards rewrite
the access/http module to use this API for sending requests and
receiving responses.
Sounds good?

>
> Le lundi 03 février 2014 à 07:07:48, Felix Abecassis a écrit :
>> +/* Read a line (ended by \r\n) from an HTTP response. */
>> +static char* http_readline( vlc_object_t *p_this, int fd )
> why not net_Get?
Right, thanks. You mean net_Gets?

>> +{
>> +    size_t size = 64;
>> +    char *str = malloc(sizeof(*str) * size);
> no check
Right, thanks, but not needed if I use net_Gets :)

> Never trust a source if there is no ':' the following dereference is likely to segfault.
Yes, thanks. I think it's more than likely to segfault :)

>> +    while (*value_begin == ' ')
> You may also skip potential '\t'
Right, thanks.

> is it worth to call strncmp when you could test just line[0] == '\r' &&
> line[1] == '\n' if there are 2 chars
Isn't that some unnecessary optimization? :)

> You shouldn't use realloc like that. It can return NULL but
> header->dictionary will still be allocated and will be leaked.
> Why not using the array macros already available in vlc_arrays.h
Thanks, I was not aware of these macros.

>> +
>> +    /* Try to find Content-Length entry in the header */
>> +    for (size_t i = 0; i < header->size; ++i)
>> +    {
>> +        const char *key = header->dictionary[i].key;
>> +        const char *value = header->dictionary[i].value;
>> +        if (!strncasecmp(key, "Content-Length", 14))
> This could be done in the previous loop
I've modified my code since this patch, I now define a list of
callbacks for the headers with a (const char*, callback) array.
It avoids the cascading strncasecmp for all possible headers.

> Why strdup("") when you could asprintf("%s %s HTTP/1.%d",...)
> And you didn't check for result.
Indeed, but actually Inow  realize it's not necessarily to build the
full string of headers, we can instead call net_Printf several times.

>
>> +    for (size_t i = 0; i < request_header->size; ++i)
>> +    {
>> +        const char *key = request_header->dictionary[i].key;
>> +        const char *value = request_header->dictionary[i].value;
>> +        /* Discard unauthorized headers */
>> +        if (!strncasecmp(key, "Content-Length", 14)
>> +            || !strncasecmp(key, "Connection", 10))
> Discard "\r\n" :)
Now handled by my callback list described above :)

>> +        if (!(header_string = http_add_header(header_string, "Expect", "100-continue")))
> Maybe make Expect a forbidden header like "Content-Length" and
> "Connection"
Good idea.

> Suddenly I wonder what happens when you have "Connection: close" and
> "Expect: 100-continue"
It would be surprising if the server where to close the connection
before sending the final status code.

Thanks for your feedbacks

-- 
Félix Abecassis
http://felix.abecassis.me