[genivi-dlt] [PATCH] All dlt_user functions return DLT_RETURN_BUFFER_FULL when, buffer full

Helwing, Lutz Lutz_Helwing at mentor.com
Fri Mar 18 13:19:53 EDT 2016


Hello Gunnar,

thanks for your remarks. Don't worry: of course i'm not offended :)

Some points:

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

One reason for things not being caught in unit tests is obvious: no one 
contributes any.

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.

Kind regards
Lutz

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-
>> size+length+sizeof(uint32_t)+sizeof(uint16_t))>DLT_USER_BUF_MAX_SIZE)
> ...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
> things.
>
> (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 [1] 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
> why?
>
>  From me it would be a -1 and please resubmit separate patches.
> No offense.
>
> Best Regards
> - Gunnar
> [1] http://git.projects.genivi.org/?p=dlt-daemon.git;a=commitdiff;h=d319ebf4
> d2d449d70befae0eebbdd82c19c5315f;hp=858e87c83bc527a546ee7ec3617f2a306a5e45fc
>
>
>
> 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
>> problems:
>>
>> 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
>> unsigned).
>>
>> 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
>> -- 
>>
>> 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
>> http://www.mentor.com
>>   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
>> https://lists.genivi.org/mailman/listinfo/genivi-diagnostic-log-and-trace



More information about the genivi-diagnostic-log-and-trace mailing list