daemon: check payload length before cast to struct

[clipka]

Check if size of control request structure fits into actual payload datasize. In case it does not fit, return with error message.

[gunnarx]

The fix looks good to me, but too much repetition for my taste. I understand there is repetition in the original code, but this just extends it. Every function (except one) seems to do the same checks and very similar error reporting. Refactor?

[clipka]

What do you think about doing the check inside a define and just call it later to reduce the duplication:

/* checks if received size is big enough for expected data. */
/* checks if received size is big enough for expected data */ #define CHECK_DATA_SIZE(received, required) \ ({ \ int _ret = (int)received - (int)required; \ if (_ret < 0) { \ dlt_vlog(LOG_WARNING, "%s: Received data not complete\n", __func__); \ _ret = DLT_RETURN_ERROR; \ } \ else { \ _ret = DLT_RETURN_OK; \ } \ _ret; \ })
call it like this;
if (CHECK_DATA_SIZE(msg->datasize, sizeof(DltServiceGetLogInfoRequest)) < 0) { return; }

[gunnarx]

Sorry for late response.

Yes I was thinking something like that exactly, but:

• Personally I'm usually OK with macros in C if they reduce repetition and complexity. But an inline function might be a type safe alternative, and more accepted by other complainers who feel macros are hacks.
  UPDATE: I see your use of _func_ might need to use a macro to be correct.

• You rewrote your own check which is good** but looking at the whole I'm thinking we might want to reduce the repetition of the previous checks also. So rather than a macro named "check data size" I would try to extend the helper function/macro to the bigger question we are actually trying to answer : "is input (in)valid?".

**Rather than complain about your bugfix :-) good practice is to do one thing at a time so I'd still propose we take your bugfix in one commit, and then refactor in the next commit.

Do you need the bugfix urgently? In that case we should recommend mering your fix in first.

Let me take a deeper look. Maybe I'll make a proposal based on top of your bugfix if you don't mind - taking the opportunity to do some programming for a change :-)

[clipka]

Hi,
ok, then we should take the commit. But I cannot approve my own request :-/

[gunnarx]

Adding refs to email discussions.
1 - @clipka patch announcement, November 2016
2 - @gunnarx generalized discussion, December 2016
3 - continued, January 2017

[gunnarx]

Well here's a +1 from me at least, but @GeniviDLTmaintainer should weigh in I guess (or is that you nowadays ;-)

+1 because I figure we could refactor and align error management after this merge