[genivi-dlt] [PATCH] All dlt_user functions return DLT_RETURN_BUFFER_FULL when, buffer full
Lutz_Helwing at mentor.com
Fri Mar 18 13:19:53 EDT 2016
thanks for your remarks. Don't worry: of course i'm not offended :)
The behaviour of the interface can and should be discussed, that's
right. If anyone did other checks for return values than ret !=
DLT_RETURN_OK there would be an impact through this patch. The truncate
"fix" also meant a change in behaviour of DLT which no one seemed to
have a problem with. The quick fix changes behaviour again.
The whitespace mess came from reverting to the old state, i'm sorry, i
should have been more selective in reverting. The weird long lines can
be seen all over the file in the parts i did not change. All in all,
frankly speaking, the DLT code is a huge mess when looking at
formatting. The coding style even changes within the same file and
sometimes even within the same function. I cannot see compliance to any
coding guidelines at all. Where can the "DLT coding standard" be found?
Would be interesting to have a look at it. For the Windows line endings
i must say that i don't see any in the patch file i have on my computer
and it would be strange because i don't use Windows. I will cross-check
One reason for things not being caught in unit tests is obvious: no one
All in all it would be a very worthwhile project to raise the DLT code
format quality to some acceptable level and to massively work on
improvement of testing. If DLT quality really is pretty important to a
lot of companies it should be a doable effort.
I will check the patch and re-submit after splitting the commits.
Thanks again for looking at the patch.
On 18/03/16 17:34, Andersson, Gunnar wrote:
> Hello Lutz
> First time reviewing these patches so a lot of spontaneous
> reactions, no offense, but DLT quality is pretty important
> to a lot of companies.
> To be direct, the patch looks like a big mess. Can you separate
> the following changes, all of which seem to be included at the
> same time here:
> 1. Quick fix needed for crashing(?) bug that causes buffer
> overflow as described below.
> 2. Changing the return value behavior of public interfaces...
> There is a lot of DLT code out there nowadays depending on those
> interfaces. I realize this is development and not a tagged
> release yet, but I'd be a whole lot more careful about changes
> like this - besides you state yourself it may not be the right
> solution yet. So, it sounds like it needs more discussion then?
> 3. Whitespace changes all over the place. I don't know if this
> long line with no spaces between any tokens is according to
> DLT code standard or not...
>> +if ((log-
> ...but if it is then still put the many many such changes in a
> separate commit, please.
> You can't expect people to review the critical parts of this when
> the patch submission is this long and such a big mix of unrelated
> (Also, did you mean to have Windows line feeds on the whole patch?
> I guess I can't say anything about the cross-platform dlt-viewer but
> in dlt-daemon I think it has no place.)
> The previous faulty commit  was smaller but it suffered from
> similar issues (whitespace changes and stuff), which makes it more
> difficult to nail down where the mistake was really made and I think
> also more difficult to just revert to a working solution instead of
> what here seems like another stressed solution (possibly creating
> more problems?)
> I would recommend some analysis of why this was not caught in unit
> tests. A test was commented out in last commit - did you explain
> From me it would be a -1 and please resubmit separate patches.
> No offense.
> Best Regards
> - Gunnar
>  http://git.projects.genivi.org/?p=dlt-daemon.git;a=commitdiff;h=d319ebf4
> On Fri, 2016-03-18 at 15:12 +0000, Helwing, Lutz wrote:
>> Dear DLT community,
>> we have found an issue with truncation of messages which do not fit into
>> buffer or when buffer is full. Thus we would like to propose a patch that
>> will undo the change in some way:
>> Commit d319ebf4d2d449d70befae0eebbdd82c19c5315f introduced new handling of
>> the situation when writing data to the user buffer would exceed its size.
>> This was achieved by truncating the data to fit. This solution had two
>> 1. It contained a bug which could lead to a buffer overflow when the
>> updated arg_size gets negative (actually undefined value because it is
>> 2. Possibly useful data get lost with truncation.
>> Thus the patch changes the behaviour of the DLT library functions to
>> return DLT_RETURN_BUFFER_FULL instead of truncating. This enables the user
>> to trigger a send of the buffer and then trying to write the same data to
>> the now empty buffer again so nothing is lost.
>> Also some tests have been implemented to test if the correct value is
>> returned when too much data is tried to be written.
>> Perhaps this way of handling the "buffer full" situation is also not that
>> brilliant. It would be nice to discuss a way to handle this more more
>> reasonably. In the best case by the library itself?
>> Kind regards
>> Lutz Helwing
>> Senior Engineer
>> Telefon +49 (89) 57096 - 297
>> Fax +49 (89) 57096 - 400
>> lutz_helwing at mentor.com
>> Mentor Graphics Development (Deutschland) GmbH
>> Arnulfstrasse 201
>> D-80634 München
>> Mentor Graphics Development (Deutschland) GmbH
>> Geschäftsführung: Dean Freed, Walter Vermijs, Shannon Wetzel
>> Handelsregister: Amtsgericht Freiburg i.Br., HRB 705237
>> Sitz der Gesellschaft: Peterzeller Straße 8, 78048 Villingen-Schwenningen
>> USt-Id Nummer: DE268786819
>> Confidentiality Notice: This e-mail message, including any attachments,
>> is for the sole use of the intended recipient(s) and may contain
>> confidential and privileged information. Any unauthorized review, use,
>> disclosure or distribution is prohibited. If you are not the intended
>> recipient, please contact the sender by return e-mail and destroy all
>> copies of the original message.
>> genivi-diagnostic-log-and-trace mailing list
>> genivi-diagnostic-log-and-trace at lists.genivi.org
More information about the genivi-diagnostic-log-and-trace