http: have sol_http_param_add_xxx functions to help build params

[cmarcelo]

This is an API suggestion.

The current way to build a params is by calling sol_http_params_add() with a helper macro that creates literals for the struct sol_http_param_value.

sol_http_params_add(&params, SOL_HTTP_REQUEST_PARAM_POST_FIELD("occupied", "yes"))

the problem with this approach is: we have to "pay" for the "namespace tax" twice, first for the function name, then for the macro name. It also adds a bit of smoke when the user does something wrong.

The suggestion is replace the macros to create literals for specific functions (they could be defined in the headers to enable inlining).

sol_http_params_add_post_field(&params, "occupied", "yes")

Similar pattern would apply for other types of parameters.

Note: I couldn't find uses of the SOL_HTTP_REQUEST_PARAM_* macros that were not associated with adding, are there any?

[barbieri]

I feel your pain, but note that there are two variants, one that copies the strings and the other that just references it :-/

[barbieri]

ah, and in the past I've suggested a different approach since most of the time we add couple of fields that is to use an va_args. It would be bit less secure since one can always screw the order, but something like:

static inline int sol_http_params_many_add(struct sol_http_params *params, ...);

sol_http_params_many_add(&params,
    SOL_HTTP_REQUEST_PARAM_POST_FIELD("occupied", "yes"),
    SOL_HTTP_REQUEST_PARAM_POST_FIELD("x", "1"),
    SOL_HTTP_REQUEST_PARAM_SENTINEL /* 0 */);

or (less safe):

static inline int sol_http_params_many_add(struct sol_http_params *params, enum sol_http_param_type type, ...);

sol_http_params_many_add(&params, SOL_HTTP_PARAM_POST_FIELD,
   "occupied", "yes",
   "x", "1",
   NULL);

[cmarcelo]

Is params expected to outlive the call to sol_http_client_request?

• If no, why do we need the string copy variant?
• If yes, when it is safe to destroy the params (to get rid of the strings we copied)?

Side note: by far, most of my questions when reading the documentation were like the above. I wasn't sure whether was OK to create some parameter struct on the stack or not.

[barbieri]

The struct itself is not referenced, just the contents. They are kept around until the internals use it, as you can see, the API to add may be executed on one function that clears the stack and then the actual client post/get method is executed, this would use garbage memory.

However, many times the strings will be alive, like static const globals or pointers we keep alive elsewhere (like machine-id or some other specific context that lives longer than the connection start). Then always duplicating is bad.

For those we duplicate, we do keep an sol_arena to make the process easier.

@cabelitos can you take a look to improve the docs?

[cmarcelo]

I meant its content as well.

The sample code does something like:

• params init
• do_request_with_params()
• params clear

When I copy, the sol_arena inside params get a copy of the strings, but then we clear them out right after the call. If they are needed "after" (i.e. imagine something posted to the mainloop for later execution), the strings will be already invalid as well. In this case it seems I should clear the params only during response time?

It seems to me either the sample is "wrong" or the strings aren't needed after the do_request_with_params() returns, and we wouldn't need copy strings in the first place.

(*) I say wrong because the sample doesn't copy anything, so the strings will always be valid.

[barbieri]

Once you params_clear() the vector will be empty, so if you do a second do_request_with_params(), then it will find nothing.

OTOH, if you mean do_request_with_params() may delay its usage of params, then indeed, the correct behaviour is to either 1) duplicate params or 2) use params and not the internal members, so when you loop over it you'll find no entries in case of params_clear() (at least it won't segfault).

As for the actual implementation of sol_http_client_request(), either it makes use of params directly and writes them to the socket or an intermediate buffer that will go to that socket, or it must keep an internal copy itself. Usually these libraries keep a "write out" buffer to make logic of managing what is left to send easier (ie: send() or write() will return less bytes processed, you'd need to re-process to find where it was chopped).

As for the sample, you mean https://github.com/solettaproject/soletta/blob/master/src/samples/http/client.c? Looking at it, indeed, the copies shouldn't be needed.

[cmarcelo]

The example I was refering was the one in the docs

http://solettaproject.github.io/docs/c-api/group__HTTP__CLIENT.html#ga8cd4dc21acf75f83d251cf4865961e2a

but the same sequence happen in

https://github.com/solettaproject/soletta/blob/master/src/samples/http/client.c#L239

which is

• prepare params (possibly copying)
• sol_http_client_request()
• clear params (deleting the arena with the copies)

Given that description of the implementation, it seems having an arena inside params and the copy variants aren't very helpful.