[FFmpeg-devel] [PATCH] Replace old cookies with new cookies of the same name

[Nicolas George]

Le primidi 1er fructidor, an CCXXII, Micah Galizia a écrit :
> Yes & no. I agree its not an ideal implementation (it actually was
> mine to begin with) to just use a string full of cookies. But we can't
> pass around complex structures through avopts, which is where we would
> really see the benefit. As you & Mark pointed out, we need to maintain
> compatibility with the current format, so why why go through parsing
> the cookie list each time if 1 in 100 requests actually sends back an
> updated cookie, and even then you'd have to dump it back out to a
> string and parse it again to pass it between http requests.... all
> this considered, I'd say I'm definitely leaning to "no" unless there
> is something I've overlooked.

First, a small nitpick: if you read the option to get the updated values of
the cookies after the request, then you are abusing the API, as the option
does not have AV_OPT_FLAG_EXPORT. But for the sake of discussion we can
assume it should be there and always was.

But that does not mean that the string must be the authoritative value used
internally. A lot of your code is spent updating the middle of the string
while keeping the prefix and suffix constant. That makes a lot of tricky
code, with reallocations and possible integer overflows everywhere.

I am convinced that parsing the string as a whole, manipulating an internal
data structure and rebuilding the string as a whole would give much simpler
code. The parsing is needed for the actual Set-Cookie header; line-splitting
is relatively easy. As for the rebuilding of the string, we have utilities
that can take care of the reallocations and integer overflow for you; I
think especially of AVBPrint. Here is a snippet of code:

    av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
    for (i = 0; i < http->nb_cookies; i++) {
	Cookie *c = &http->cookie[i];
	if (i)
	    av_bprintf(&bp, "\n");
	av_bprintf(&bp, "%s=%s", c->name, c->value);
	if (c->domain)
	    av_bprintf(&bp, "; domain=%s", c->domain);
	if (c->path)
	    av_bprintf(&bp, "; path=%s", c->path);
	if (c->extra)
	    av_bprintf(&bp, "; %s", c->extra);
    }
    if (!av_bprint_is_complete(&bp))
	return AVERROR(ENOMEM);
    return av_bprint_finalize(&bp, &http->cookies);


Another point: I believe that having a callback on some options values may
also be quite useful. That could be done as AV_OPT_FLAG_CALLBACK with a
function pointer in AVClass. In this particular case, the callback could
parse the string on the fly and re-generate it only when necessary.

What do people think about that?

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140818/dd4f7d47/attachment.asc>