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

[Rémi Denis-Courmont]

On Wed, 5 Feb 2014 14:21:08 +0100, Felix Abecassis
<felix.abecassis at gmail.com> wrote:
> 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?

I don't think the HTTP access belongs in the core. And worse, your API is
much too simplistic to support streaming.

>> 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?

net_Gets() is a blocking function and as it stands can only be used safely
from access or demux object. So that sounds like a Dead End.

>> 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.

No. VLC macros do not cope with errors and should be avoided IMHO.

>>> +
>>> +    /* 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.

No- Calling net_Print() several times would split the packet across
multiple TCP segments. This is inefficient from network point of view. And
to add pain to injury. net_Printf() is blocking and thus inadequate here.

And while I do not care personally, I will mention that some broken HTTP
servers insist on the request being sent in a single segment - even though
that makes no sense and violates the specification.

-- 
Rémi Denis-Courmont
Sent from my collocated server