[patch #9611] tftp client/dhcp issue

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client/dhcp issue

Simon Goldschmidt
URL:
  <http://savannah.nongnu.org/patch/?9611>

                 Summary: tftp client/dhcp issue
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: bwijen
            Submitted on: Sat 31 Mar 2018 09:03:18 AM UTC
                Category: None
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None

    _______________________________________________________

Details:

Hi,

I'm working on an tftp client implementation.
I added two patches for a first review.
Please let me know if this is something you are interested in.

Also:
While using my implementation, I think I also found
a bug in the DHCP implementation (when LWIP_DHCP_BOOTP_FILE is set)
Could you please confirm?

FYI:
The patches are on top of the STABLE-2.0.3_RELEASE tag.


Thank you,

Ben...



    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Sat 31 Mar 2018 09:03:18 AM UTC  Name:
0001-tftp-Add-helper-functions.patch  Size: 5KiB   By: bwijen

<http://savannah.nongnu.org/patch/download.php?file_id=43738>
-------------------------------------------------------
Date: Sat 31 Mar 2018 09:03:18 AM UTC  Name:
0003-dhcp-Fix-BOOTP_FILE-bug.patch  Size: 2KiB   By: bwijen

<http://savannah.nongnu.org/patch/download.php?file_id=43739>
-------------------------------------------------------
Date: Sat 31 Mar 2018 09:03:18 AM UTC  Name:
0002-tftp-Add-client-functionality.patch  Size: 3KiB   By: bwijen

<http://savannah.nongnu.org/patch/download.php?file_id=43740>

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

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


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Update of patch #9611 (project lwip):

                 Summary:  tftp client/dhcp issue => tftp client            

    _______________________________________________________

Follow-up Comment #1:

Yes, having a TFTP client integrated with the server would be welcome. But
please:
- rename or add files to reflect their contents (server vs. client)
- name functions to reflect their usage (server vs. client; tftp_init() is a
really bad example here)
- keep one patch for one issue (tftp client) and don't mix it with a bugfix
(dhcp issue) - I've change the summary for this.

On the DHCP issue, you're probably right. However, after your change, it seems
like with "parse_sname_as_options != 0", the file name could be copied
multiple times?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

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


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Follow-up Comment #2, patch #9611 (project lwip):

Hi Simon,

Thanks for your reply.
Regarding your comments:
* I have created bug #53574: "DHCP issue when LWIP_DHCP_BOOTP_FILE is set"
* I will make sure to squash any future patches...

Regarding the split:
The tftp code is over 90% both server *and* client.
Would you agree renaming 'tftp_server' to just 'tftp'?
tftp_init would then mean both client and server.

Or would you prefer the more drastic approach: renaming the current
implementation tftp and added tftp_client and tftp_server wrappers?


Thank you,

Ben...

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

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


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Follow-up Comment #3, patch #9611 (project lwip):

Renaming the file after adding a tftp client is fine. However, having one
'init' function for both client and server is a not good idea. I can clean
that up once your client patches are acceptable though.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

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


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Follow-up Comment #4, patch #9611 (project lwip):

Since this patch does not contain things worth to keep (up to now), can I
close it or do you have more input/progress to share?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

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


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Follow-up Comment #5, patch #9611 (project lwip):

Sorry for the delay.
Please see the 0004 path.

Do you require more than squashing and renaming?
If so, let me know.

(file #44044)
    _______________________________________________________

Additional Item Attachment:

File name: 0004-tftp-Add-client-functionality.patch Size:15 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Follow-up Comment #6, patch #9611 (project lwip):

Sorry for the delay. Could you describe what's in your patch, please? Is it a
fully working tftp client? Or just yet another example like the last patch?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Follow-up Comment #7, patch #9611 (project lwip):

Ping?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Follow-up Comment #8, patch #9611 (project lwip):

I'll have to add that the patch doesn't apply and that's why I ask.

If there's no reaction, I think I'll close this.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Follow-up Comment #9, patch #9611 (project lwip):

Hi Simon,

Sorry for the delay.
The 0004-patch is a squashed version of 0001 and 0002 with some files renamed,
as you suggested. (Still based on the 2.0.3 branch.)
If you want, I can rebase on master.

Please note, that the tftp-client functionality works (at least for me)

If you require something more, please let me know, although my time is scarce
I'm willing to take this to the end.

Ben...

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Additional Item Attachment, patch #9611 (project lwip):

File name: 0005-tftp-Add-client-functionality.patch Size:14 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9611>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9611] tftp client

Simon Goldschmidt
Update of patch #9611 (project lwip):

                  Status:                    None => Done                  
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #10:

Applied, thanks for your contribution!

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9611>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel