[lwip-devel] [bug #52763] mdns: move data off the stack and dynamically allocate data.

[Douglas]

Follow-up Comment #9, bug #52763 (project lwip):

Thank you for the feedback, some of the issues caught are good to have fixed.
The rebased patch combines all the fixes, even the reverted support for
delaying responses. I doubt there are the resources to slice this all up
sorry, so could we try to move forward.

> mdns_service_free() has a copy-paste error: 

Good catch, glad that one was fixed, thanks.

> I don't really see the point of keeping the host name in a separately
allocated block. Its lifetime is the same as the surrounding struct, and it
only makes a few bytes difference. Also, the name might have to be adjusted so
it is good to know the amount of space available. This also applies to the
char arrays in the service struct. 

It uses less ram storage space in most cases. This patch is largely about
moving from all the statically allocate maximum sized storage blocks to
variable length allocations. For a variable size it can not be a part of a
statically sized C struct, so it's a C string pointer. I understand the change
takes an almost opposite style of allocation, and the original code style is
probably better if ram is plentiful, but this patch is about optimizing for
low ram usage rather than performance and code size.

> You cannot count on any string functions being available, especially strndup
which does an allocation. lwIP must not have any dependencies outside of what
the port is expected to provide. 

strndup is a standard C library string function so it seems reasonable to
expect it to be provided. If some port does not have it seems better for them
to just add it in port specific code rather than requiring all ports to
duplicate a standard function. Looking at that code again there was a lack of
error checking on these which has been added.

> Please update the function comments in the cases where you change the method
signature.

Done, forget those, thanks.

> The late declaration of err_t res will not compile on stricter systems.

Yes, good point, fixed. Seem to keep making that mistake.

> The old mdnsapi_* functions are still in the header file.

Opps, fixed.

> Please split out the LWIP_MDNS_RESPONDER_QUEUE_ANNOUNCEMENTS changes into a
separate patch.

There seems to be interactions between a lot of these changes, so it seems
best to consider how it all fits together. The patch combines fixes for all
the issues raised. It's a lot of work to start breaking things up. I
understand it's a bigger job to review together, but my time is also limited.

> Does this function still compile without the int declaration?
> mdns_resp_remove_netif(struct netif *netif)
> {
> - int i;

I don't see that in the current patch.

> The dns_hdr struct is 12 bytes, that should be small enough to keep it on
the stack.

This patch is about moving data off the stack. I have other reasons for
wanting a struct or array off the stack, for checking accesses. Only trivial
data types remain on the stack.

The updated patch also adds a random delay for announcements, computing them
after the delay. It also adds some lock assertions.

(file #42805)
    _______________________________________________________

Additional Item Attachment:

File name: 0001-mdns-move-data-off-the-stack-and-dynamically-allocat.patch
Size:78 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?52763>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/