jimmyliu86 / btstack

Automatically exported from code.google.com/p/btstack
0 stars 0 forks source link

Please remove malloc from SDP server #147

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
There is a slight 'issue' (not really a bug) with the way the client generates 
a SDPrecord and submits it to the system, which in turn mallocs the space, 
adjusts the ServiceRecordHandle and copies (?) the record data.

In some embedded applications it is best to avoid mallocs, so it might be 
possible have the client application handle the allocation of space (malloc or 
static) and then the SDP server could just reference the existing data?

Maybe this can #defined with HAVE_MALLOC.

Simon

Original issue reported on code.google.com by mungew...@gmail.com on 28 Apr 2011 at 3:53

GoogleCodeExporter commented 8 years ago
Well, I don't like the malloc either. The problem here was that I need the SDP 
server to set the ServiceRecordHandle but cannot know if the client uses one. 
Anyway, in the POSIX version, we use a single buffer, so it needs to be copied 
somewhere.

For use in an embedded system, I assume that the client/server part isn't used 
at all. In this case, there should be a way to register an SDP record with a 
fixed record handle without an additional malloc/copy. 

Original comment by matthias.ringwald@gmail.com on 28 Apr 2011 at 5:21

GoogleCodeExporter commented 8 years ago
> I need the SDP server to set the ServiceRecordHandle but cannot know if the 
client uses one.

If we state that they MUST provided a full record (as I've been doing in my 
testing) then they/the client will do that... ;-)

We can scan the supplied record and reject it if it does not contain an 
SDPRecordHandle (valid or 0 for auto allocate). In terms of 'reporting back' we 
can just set the memory location of the handle to the value which is auto 
allocated, as well as in the HCI response.

If POSIX requires a copy of the SDPrecord to be taken then the malloc will be 
needed in that case, but when we're putting a stack on a PIC (or other small 
micro) we shouldn't be limited by this requirement.
Simon

Original comment by mungew...@gmail.com on 28 Apr 2011 at 5:52

GoogleCodeExporter commented 8 years ago
I got this implemented, but feel like a fool for chasing a non-bug for a few 
hours.

When used with socket-server the same/last record will be output multiple 
times. For embedded applications the library should be statically linked.

Original comment by mungew...@gmail.com on 29 Apr 2011 at 4:49

Attachments:

GoogleCodeExporter commented 8 years ago
thanks for the patches. I'll respond to your email later, dumping some thoughts 
on BTstack for embedded systems.

What about: "when used with socket-server the same/last record will be output 
multiple times". An SDP query matches records by UUID, so a query for "all 
L2CAP using services" will also list the SPP record e.g. (not sure if that's 
related) 

Original comment by matthias.ringwald@gmail.com on 29 Apr 2011 at 8:37

GoogleCodeExporter commented 8 years ago
This was the non-bug I spent (cough cough) 4 hours chasing. I have adjusted the 
link list so that it adds a pointer to the record data supplied by the 
'client', the client must maintain this data block.

In the client/model I was see that the record data was valid at time of 
registration, but when more than 1 record was set the last record would be 
repeated multiple time when a remote system did a browse.

In the end I figured that the data block is being duplicated on the 
client/server call and thus is good for registration. However the link list now 
points to the 'copy' so this is changed when a new record is register.... we 
now have multiple pointers hitting the same memory location - hence the 
'repeated last record' on browse.

This only affects the client/server setup. In embedded the pointers in the 
linked list hit the 'real' locations, and all is well with the world.
Simon.

Original comment by mungew...@gmail.com on 29 Apr 2011 at 3:07

GoogleCodeExporter commented 8 years ago
Did you get a chance to look at this yet?

If you think that this is an OK way of approaching the problem I can do similar 
for HCI, L2CAP and (now) RFCOMM.

Simon.

Original comment by mungew...@gmail.com on 4 May 2011 at 3:10

GoogleCodeExporter commented 8 years ago
well, I had a quick look. I think this would create too much clutter with many 
#ifdefs. otherwise, I didn't really look into the problem. in your project, 
could you use a custom malloc which uses fixed allocated memory?

Original comment by matthias.ringwald@gmail.com on 4 May 2011 at 3:25

GoogleCodeExporter commented 8 years ago
I don't know whether I would consider 4 * '#if' too many, but this would apply 
across all files with mallocs and it is your code after all... obviously this 
is a source issue and does not apply to the runtime binary.

The software devs are very keen that there be no dynamic allocation after init 
is complete, I think that they are scared of memory fragmention/depelition 
resulting in a system crash - which would be bad in an embedded solution.

Do any others have comments to add?
Simon

Original comment by mungew...@gmail.com on 4 May 2011 at 3:47

GoogleCodeExporter commented 8 years ago
patches as mentioned on mailing list.
Simon,

Original comment by mungew...@gmail.com on 6 May 2011 at 3:33

Attachments:

GoogleCodeExporter commented 8 years ago
"processed patches" :) - didn't compile or test though..

Original comment by matthias.ringwald@gmail.com on 12 May 2011 at 8:05