Skip to content

net: refactor tftp library #36579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 29, 2021
Merged

net: refactor tftp library #36579

merged 1 commit into from
Jun 29, 2021

Conversation

@mnkp
Copy link
Member

@mnkp mnkp commented Jun 27, 2021 •
edited
Loading

  • bugfix: Accept initial TFTP server reply from a port different than
    the one used to establish the connection (typically 69) as mandated
    by RFC 1350. Previous implementation was not standard compliant.
  • bugfix: close socket in case of error or timeout.
  • bugfix: Reset retransmit counter after receipt of a good packet.
  • bugfix: Use CONFIG_TFTP_LOG_LEVEL to set log level.
  • api: upon successful receipt of the file set client.user_buf_size
    to the size of the file received.
  • Restructure the code, comments.
  • Limit usage of global variables.
  • Limit usage of goto.

This PR, apart from one necessary exception, does not attempt to modify TFTP API. Consequently not all issues are fixed:

  • usage of a large, private static buffer was not removed
  • buffer passed as a parameter to tftp_get function call has to be large enough to fit the full file, i.e. to receive a file of size up to 64 kB user is forced to reserve 64 kB of RAM.

Fixes #34131

@mnkp mnkp added bug The issue is a bug, or the PR is fixing a bug area: Networking labels Jun 27, 2021
@mnkp
Copy link
Member Author

mnkp commented Jun 27, 2021

@olsky, @bwasim

@mnkp mnkp force-pushed the tftp_lib_refactor branch from 697af5e to c7444dd Compare June 27, 2021 20:45
jukkar
jukkar reviewed Jun 28, 2021
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fixing any issues mentioned in #34131?

subsys/net/lib/tftp/tftp_client.c Outdated
Copy link
Member

@jukkar jukkar Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra () around sizeof are not needed.

subsys/net/lib/tftp/tftp_client.c Outdated
Copy link
Member

@jukkar jukkar Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better would be set the values using the .field = value, notation, then there are no errors if the fields are reorganized in the struct

@mnkp
Copy link
Member Author

mnkp commented Jun 28, 2021

Is this fixing any issues mentioned in #34131?

Sorry, I missed the issue. Yes, this PR is fixing the problem described in #34131 including

Noticed resource leak in tftp_get(), the sock is not closed in the case of error or timeout. Also tftp_lock is not relased in the case of error. We should goto to req_done: label in the case of errors (except the socket() failing)

I will wait a bit for additional reviews before cleaning up this PR.

rlubos
rlubos reviewed Jun 28, 2021
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, not much to comment.

mnkp
mnkp commented Jun 28, 2021
View reviewed changes
@mnkp
net: refactor tftp library
4d712a5 
- bugfix: Accept initial tftp server reply from a port different than
  the one used to establish the connection (typically 69) as mandated
  by RFC 1350. Previous implementation was not standard compliant.
- bugfix: close socket in case of error or timeout.
- bugfix: Reset retransmit counter after receipt of a good packet.
- bugfix: Use CONFIG_TFTP_LOG_LEVEL to set log level.
- api: upon successful receipt of the file set `client.user_buf_size`
  to the size of the file received.
- Restructure the code, comments.
- Limit usage of global variables.
- Limit usage of `goto`.

Signed-off-by: Piotr Mienkowski <[email protected]>
@mnkp mnkp force-pushed the tftp_lib_refactor branch from c7444dd to 4d712a5 Compare June 28, 2021 18:30
@github-actions github-actions bot added the area: API Changes to public APIs label Jun 28, 2021
mnkp
mnkp commented Jun 28, 2021
View reviewed changes
Copy link
Member Author

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All pending issues were fixed. I've updated include/net/tftp.h to mention that upon successful transfer client->user_buf_size will contain received file size.

rlubos
rlubos approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nashif nashif merged commit ccc0999 into zephyrproject-rtos:main Jun 29, 2021
@mnkp mnkp deleted the tftp_lib_refactor branch June 29, 2021 13:39
brudley added a commit to recogni/zephyr that referenced this pull request Jul 2, 2021
@brudley
Update tftp lib wit latest upstream mods
2e05f26 
sha 4d712a5
net: refactor tftp library zephyrproject-rtos#36579
@brudley brudley mentioned this pull request Jul 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jukkar jukkar jukkar approved these changes

@rlubos rlubos rlubos approved these changes

@pfalcon pfalcon Awaiting requested review from pfalcon

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

Assignees

No one assigned

Labels

area: API Changes to public APIs area: Networking bug The issue is a bug, or the PR is fixing a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TFTP client ignores incoming data packets

4 participants

@mnkp @jukkar @rlubos @nashif