Skip to content
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.

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

gcoap: Add file server #17956

Merged
merged 2 commits into from
May 23, 2022
Merged

gcoap: Add file server #17956

merged 2 commits into from
May 23, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Apr 15, 2022

Contribution description

This is a continuation of @chrysn's GCoAP fileserver implementation with only minimal changes.
Files are served block-wise, directory listings are now too.

My use case is to provide a CoAP fileserver to serve SUIT update files from SD card.

Testing procedure

Instead of extending the already cluttered examples/filesystem, I added a new examples/gcoap_fileserver that leverages vfs_default and is thus much cleaner.

To get some data on the (virtual) file system you can use #17937 to download some files from aiocoap-fileserver, then if you are on native copy MEMORY.bin to examples/gcoap_fileserver.

You should now be able to request the files via aiocoap-client.

main(): This is RIOT! (Version: 2022.07-devel-107-ge39b5-coapfileserver)
> ls /nvm0
ls /nvm0
./
../
LICENSE	24310 B
hello.txt	15 B
total 2 files
benpicco@T430s ~/dev/RIOT (git)-[coapfileserver] % aiocoap-client coap://[fe80::748f:e6ff:fed7:426d%tapbr0]/vfs/
</vfs/LICENSE>,</vfs/hello.txt>
benpicco@T430s ~/dev/RIOT (git)-[coapfileserver] % aiocoap-client coap://[fe80::748f:e6ff:fed7:426d%tapbr0]/vfs/hello.txt
Hello from RIOT
benpicco@T430s ~/dev/RIOT (git)-[coapfileserver] % aiocoap-client coap://[fe80::748f:e6ff:fed7:426d%tapbr0]/vfs/LICENSE > /tmp/LICENSE
benpicco@T430s ~/dev/RIOT (git)-[coapfileserver] % md5sum LICENSE /tmp/LICENSE 
a4ce26a8dcc018faa99a6cd08cbcd1c3  LICENSE
a4ce26a8dcc018faa99a6cd08cbcd1c3  /tmp/LICENSE

Issues/PRs references

taken over from #14397

@benpicco benpicco requested a review from chrysn April 15, 2022 16:36
@github-actions github-actions bot added Area: doc Area: Documentation Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System labels Apr 15, 2022
@benpicco benpicco force-pushed the coapfileserver branch 2 times, most recently from 87b0f7a to d0e3732 Compare April 15, 2022 16:40
@benpicco benpicco requested a review from fjmolinas April 15, 2022 16:41
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

some feedback

sys/Makefile Outdated Show resolved Hide resolved
sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
sys/include/net/coapfileserver.h Outdated Show resolved Hide resolved
sys/include/net/coapfileserver.h Outdated Show resolved Hide resolved
sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

some more feedback

sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/coapfileserver/coapfileserver.c Outdated Show resolved Hide resolved
@fjmolinas
Copy link
Contributor

My use case is to provide a CoAP fileserver to serve SUIT update files from SD card.

Nice! I had the same usecase!

@chrysn
Copy link
Member

chrysn commented Apr 20, 2022 via email

@github-actions github-actions bot added Area: build system Area: Build system Area: CoAP Area: Constrained Application Protocol implementations and removed Area: doc Area: Documentation labels Apr 22, 2022
@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Apr 22, 2022
sys/include/net/gcoap.h Show resolved Hide resolved
sys/include/net/gcoap/fileserver.h Outdated Show resolved Hide resolved
sys/net/application_layer/gcoap/fileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/gcoap/fileserver.c Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the coapfileserver branch 2 times, most recently from bfb31ab to 6c66b40 Compare May 10, 2022 09:15
sys/net/application_layer/gcoap/fileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/gcoap/fileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/gcoap/fileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/gcoap/fileserver.c Outdated Show resolved Hide resolved
sys/net/application_layer/gcoap/fileserver.c Outdated Show resolved Hide resolved
@fabian18
Copy link
Contributor

Is there already a WIP branch which implements file/directory creation?
The usecase for #17962 is to make an already existing file available as a CoAP resource right?

@benpicco
Copy link
Contributor Author

Is there already a WIP branch which implements file/directory creation?

No, my use case was just serving firmware updates, so I don't have any plans to implement this right now.

The usecase for #17962 is to make an already existing file available as a CoAP resource right?

That's to upload a file to a CoAP server via PUT, the CoAP server does not have to run RIOT (e.g. could be aiocoap-fileserver).

@fabian18
Copy link
Contributor

Ok, I want to transfer a file to the VFS using coap. So I guess my use case is not covered so far. But no problem, I think I am going to try to add this.

@benpicco
Copy link
Contributor Author

I want to transfer a file to the VFS using coap.

You can use #17937 for that

@fabian18
Copy link
Contributor

Thanks for the hint. This also kind of solves the problem but I would prefer to push the files to the nodes.

dist/tools/doccheck/exclude_patterns Outdated Show resolved Hide resolved
sys/include/net/coap.h Outdated Show resolved Hide resolved
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

Just another round of testing.

  • blockwise transfer works
  • file retrieval works
  • directory retrieval works
  • Etag validation works
> vfs ls /nvm0
2022-05-21 13:00:29,497 # vfs ls /nvm0
2022-05-21 13:00:29,504 # ./
2022-05-21 13:00:29,504 # ../
2022-05-21 13:00:29,526 # anotherdir/
2022-05-21 13:00:29,553 # cacert.der    608 B
2022-05-21 13:00:29,555 # total 1 files
$ coap-client -m get -a fe80::3478:9157:9fd5:81c3%eth0 coap://[fe80::4c49:6fff:fe54:0]/vfs/cacert.der
$ coap-client -m get -O 4,0x60829555  -a fe80::3478:9157:9fd5:81c3%eth0 coap://[fe80::4c49:6fff:fe54:0]/vfs/cacert.der
$ coap-client -m get -O 23,0x00 -a fe80::3478:9157:9fd5:81c3%eth0 coap://[fe80::4c49:6fff:fe54:0]/vfs/

Note: tested with libcoap example client and additional module CFLAGS += -DCONFIG_GNRC_IPV6_NIB_ARSM=1 because I simply tested with link local addresses over ethernet.

Screenshot_20220521_132020

I think it can go in if murdock becomes green.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 21, 2022
@github-actions github-actions bot removed the Area: tools Area: Supplementary tools label May 21, 2022

/* Normalizing fields whose value can change without affecting the ETag */
stat.st_nlink = 0;
memset(&stat.st_atime, 0, sizeof(stat.st_atime));
Copy link
Contributor

Choose a reason for hiding this comment

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

cpu/avr8_common/avr_libc_extra/include/vendor/sys/stat.h does not #define st_atime st_atim.tv_sec and we should better memset() the whole struct timespec

Suggested change
memset(&stat.st_atime, 0, sizeof(stat.st_atime));
memset(&stat.st_atim, 0, sizeof(stat.st_atim));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh now esp* and pic32 don't know .st_atim but only .st_atime 😕

/* re-use path buffer, it is already COAPFILESERVER_PATH_MAX bytes long */
path[path_len] = '/';
path[path_len + 1] = 0;
strncat(path, name, COAPFILESERVER_PATH_MAX - 1);
Copy link
Member

Choose a reason for hiding this comment

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

Why not strncpy, since path_len already contains the current length of path?

Beware: I think that strncpy and strncat have a stupid API, as they may leave the destination path without terminating zero byte if the destination buffer is not large enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean

    strncpy(path + path_len, name, COAPFILESERVER_PATH_MAX - path_len - 1);

That looks a bit more fragile to me, but might be more efficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

strncpy(path + path_len + 1, name, COAPFILESERVER_PATH_MAX - path_len - 2);

+1 because of '/' and -2 also for the '/' and a '\0'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are proving my point 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with both. @maribu, choose readability over efficiency, or not ?

Copy link
Member

@maribu maribu May 23, 2022

Choose a reason for hiding this comment

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

I just checked: strncat indeed does always zero-terminate the string, even if it has to truncate. That makes it safer to use than strncpy, so I guess strncat has won ;)

Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

ACK

@benpicco
Copy link
Contributor Author

Thank you for the review!

@benpicco benpicco merged commit 08e430b into RIOT-OS:master May 23, 2022
@benpicco benpicco deleted the coapfileserver branch May 23, 2022 20:10
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: CoAP Area: Constrained Application Protocol implementations Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants