net: lib: ftp_client: Add FTP client library

[rlubos]

Add FTP client library, based on Nordic's FTP client library from NCS, along with FTP shell module and a minimal sample.

[pdgendt]

Very nice!

[pdgendt]

TBH, I don't see the benefit of using FTP_CODE_XXX instead of using XXX directly. I'd prefer to have some actual code names.

[rlubos]

I can update the names, something like FTP_CODE_150_FILE_STATUS_OK etc. would work, or do you think we should drop the number from the name entirely?

[pdgendt]

I'd like to have descriptive names, both with or without number is fine for me.

[sylvioalves]

@rlubos do you plan to fix those sonarqube warnings like Access of the field 'ctrl_buf' at an overflowing index, while it holds only 0 'unsigned char' elements? and Refactor this function to reduce its Cognitive Complexity from 27 to the 25 allowed.?

[rlubos]

@rlubos do you plan to fix those sonarqube warnings like Access of the field 'ctrl_buf' at an overflowing index, while it holds only 0 'unsigned char' elements? and Refactor this function to reduce its Cognitive Complexity from 27 to the 25 allowed.?

Yes, I have them fixed locally already (at least I hope so), will push soon. However, I must admit I am pretty confused with the Access of the field 'ctrl_buf' at an overflowing index, while it holds only 0 'unsigned char error, either I miss something or it's a false positive, however, I've moved the out-of-bound check into the loop to see if it silences the sonarqube.

[sylvioalves]

@rlubos do you plan to fix those sonarqube warnings like Access of the field 'ctrl_buf' at an overflowing index, while it holds only 0 'unsigned char' elements? and Refactor this function to reduce its Cognitive Complexity from 27 to the 25 allowed.?

Yes, I have them fixed locally already (at least I hope so), will push soon. However, I must admit I am pretty confused with the Access of the field 'ctrl_buf' at an overflowing index, while it holds only 0 'unsigned char error, either I miss something or it's a false positive, however, I've moved the out-of-bound check into the loop to see if it silences the sonarqube.

The issue lies here:

#if defined(CONFIG_FTP_CLIENT)
#define FTP_BUFFER_SIZE CONFIG_FTP_CLIENT_BUF_SIZE
#else
#define FTP_BUFFER_SIZE 0
#endif

So ctrl_buf[0] is set when CONFIG_FTP_CLIENT is not defined (weird anyway).

[rlubos]

@rlubos do you plan to fix those sonarqube warnings like Access of the field 'ctrl_buf' at an overflowing index, while it holds only 0 'unsigned char' elements? and Refactor this function to reduce its Cognitive Complexity from 27 to the 25 allowed.?

Yes, I have them fixed locally already (at least I hope so), will push soon. However, I must admit I am pretty confused with the Access of the field 'ctrl_buf' at an overflowing index, while it holds only 0 'unsigned char error, either I miss something or it's a false positive, however, I've moved the out-of-bound check into the loop to see if it silences the sonarqube.

The issue lies here:

#if defined(CONFIG_FTP_CLIENT)
#define FTP_BUFFER_SIZE CONFIG_FTP_CLIENT_BUF_SIZE
#else
#define FTP_BUFFER_SIZE 0
#endif

So ctrl_buf[0] is set when CONFIG_FTP_CLIENT is not defined (weird anyway).

Ah, thanks for the tip! Maybe just changing 0 to 1 will be enough then. I'll try that

[rlubos]

In latest push:

• All review feedback should've been addressed,
• sonarqube issues should be fixed (we'll see that later),
• some extra changes regarding input validation and PASV message parsing based on internal feedback

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sylvioalves]

Can you also handle this line in the case where CONFIG_FTP_CLIENT is not set? Also, if CONFIG_FTP_CLIENT is not set, why is this being tested/included? Sounds unreal scenario.

[rlubos]

But this whole file won't even be compiled if CONFIG_FTP_CLIENT is unset. Other thing is the header file, which may be included unconditionally (like in the shell module introduced in other commit), so we need those safeguards for undefined symbols.

I guess sonarqube just don't understand CMake/Kconfig dependencies, hence it assumes the file could be compiled anyway...

[sylvioalves]

@rlubos I forgot to ask. Should it have some kind of test case or only the sample would be enough?

[rlubos]

@rlubos I forgot to ask. Should it have some kind of test case or only the sample would be enough?

For the initial release I've only considered the sample, as verifying FTP in ztest would be tricky w/o a FTP server to test against.