patchlwIP - A Lightweight TCP/IP stack - Patches: patch #8994, dhcp: add support for vendor...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #8994: dhcp: add support for vendor specific options 43 and 60

Submitter:  Jan Breuer <jan_breuer>
Submitted:  Sat 07 May 2016 09:56:23 AM UTC
   
 
Category:  DHCP Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Tue 25 Apr 2017 06:21:20 PM UTC, comment #11: 

In that case... I've just pushed it. Thanks for the fast response!

-> Done (although in a different way than origiginally proposed here)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 25 Apr 2017 03:53:57 PM UTC, comment #10: 

It seems to be nice and universal solution for all issues like this. I'm working on another project now, so I can't test it soon.

Jan Breuer <jan_breuer>
Tue 25 Apr 2017 02:15:30 PM UTC, comment #9: 

Jan, I've attached file #40502 to task #14270. Would you mind checking if this does what you need?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 12 Aug 2016 06:29:38 PM UTC, comment #8: 

Coming back on this:
the current implementation to get the boot file is as bad: it stores the bootfile in an 128-byte array per netif instead of passing this information zero-copy in a pbuf when it is received.
This should be fixed, too, in the same way as the vendor specific options (i.e. pbuf, offset, length ?).

Simon Goldschmidt <goldsimon>
Group administrator
Tue 24 May 2016 10:48:34 AM UTC, comment #7: 


> So the only real fix would be to statically allocate a maximum-sized buffer, I guess...


It can be hunderds of bytes. Thats a waste of RAM. There is no need to keep the vendor specific data buffer in memory after processing.

> Hmm, that's not a good reason to do it in lwIP. The problem is that a) you reserve the buffer for all netifseven if not used (also the loopback netif or PPP) and b) you need the buffer for every netif, even if you probably request the same vendor id on all netifs


I forget loopback, PPP and multi-netif! So it needs to be done differently. It should be buffer managed by the user and all DHCPs should have const ptr to it.

Jan Breuer <jan_breuer>
Tue 24 May 2016 10:33:31 AM UTC, comment #6: 


> Yes, there is no retry and it will be lost.


So the only real fix would be to statically allocate a maximum-sized buffer, I guess...

> So, this is just my egoistic need to have one field for both.


Hmm, that's not a good reason to do it in lwIP. The problem is that a) you reserve the buffer for all netifseven if not used (also the loopback netif or PPP) and b) you need the buffer for every netif, even if you probably request the same vendor id on all netifs

Simon Goldschmidt <goldsimon>
Group administrator
Tue 24 May 2016 09:42:27 AM UTC, comment #5: 

Yes, there is no retry and it will be lost.
Problem is, that passing just pbuf in callback is not much user friendly outside lwip core but still ok. Passing also offset and length is something, I would like to avoid. Somebody can also call pbuf_coalesce in the callback and I think that it will break dhcp code. It forces dhcp to pass it as const pbuf.

Calling mem_calloc is also not good solution.

Is there any other possibility to pass larger buffer to user callback. Is there something already in LwIP so I can take inspiration?


Problem of shared vendor_class_identifier is, that we already have in-house proprietary dhcpv6. I can't push it upstream (sad). So, this is just my egoistic need to have one field for both.

Jan Breuer <jan_breuer>
Mon 23 May 2016 08:25:18 PM UTC, comment #4: 

What happens if mem_calloc fails when parsing the server's response? There's no retry handling, so the information is lost. We should find a better way to handle the case where the response bytes are split among multiple pbufs. Maybe we can just pass the RX pbuf, start offset and length?

Also, I'd really prefer to keep the request buffer in struct dhcp. Let's care for dhcp6 when it is implemented...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 23 May 2016 10:48:27 AM UTC, comment #3: 

Something like

netif_set_dhcp_vendor_class_identifier(netif, identifier)
netif_get_dhcp_vendor_class_identifier(netif)

?

If so, it is implemented in next patch.

I'm using this field intentionally in netif struct and not in dhcp struct, because it can be shared between dhcpv4 and dhcpv6 implementations. If you think that it is not good idea, I can rearrange it little bit.

(file #37235)

Jan Breuer <jan_breuer>
Mon 23 May 2016 08:07:51 AM UTC, comment #2: 

The patch looks OK, but the lwIP API design should provide functions (or function-like macros) to access struct members. Applications should not access struct members directly. Although this cannot really be checked in C (unless you hide the actual struct definition, which produces bad code), could you add such a function or macro for the new struct dhcp member?

Simon Goldschmidt <goldsimon>
Group administrator
Sat 07 May 2016 10:12:18 AM UTC, comment #1: 

File #37096 is the correct patch. I forget to add some changes in previous one.

Jan Breuer <jan_breuer>
Sat 07 May 2016 09:56:23 AM UTC, original submission:  

Add feature LWIP_DHCP_VENDOR_SPECIFIC

LWIP_DHCP_VENDOR_SPECIFIC==1:
Request vendor specific information by identifer in

netif->vendor_class_identifier

For each response packet, a callback is called, which has to be provided by the port:

void dhcp_handle_vendor_specific(const char * data, size_t len);

Data must be handled immediately or copied, because they are freed after return from this callback.

Jan Breuer <jan_breuer>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jan_breuer (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-04-25 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2016-05-23 jan_breuer Attached File- Added 0002-dhcp-vendor-specific-identifier-getter-setter.patch, #37235
    2016-05-07 jan_breuer Attached File- Added 0001-dhcp-add-support-for-vendor-specific-options-43-and-.patch, #37096
    2016-05-07 jan_breuer Attached File- Added 0001-dhcp-add-support-for-vendor-specific-options-43-and-.patch, #37095

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code