-
Notifications
You must be signed in to change notification settings - Fork 2k
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
gcoap: Add file server #17956
Conversation
87b0f7a
to
d0e3732
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more feedback
Nice! I had the same usecase! |
Tbh I didn't fully wrap my head around the whole etag thing and I don't quite see how useful it is, so it's high on my culling list.
The ETag is what stands between files being modified and a client silently taking a frankenfile for the response.
|
b635389
to
7d59900
Compare
bfb31ab
to
6c66b40
Compare
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.
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 |
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. |
You can use #17937 for that |
Thanks for the hint. This also kind of solves the problem but I would prefer to push the files to the nodes. |
There was a problem hiding this 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.
I think it can go in if murdock becomes green.
|
||
/* Normalizing fields whose value can change without affecting the ETag */ | ||
stat.st_nlink = 0; | ||
memset(&stat.st_atime, 0, sizeof(stat.st_atime)); |
There was a problem hiding this comment.
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
memset(&stat.st_atime, 0, sizeof(stat.st_atime)); | |
memset(&stat.st_atim, 0, sizeof(stat.st_atim)); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Thank you for the review! |
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 newexamples/gcoap_fileserver
that leveragesvfs_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 onnative
copyMEMORY.bin
toexamples/gcoap_fileserver
.You should now be able to request the files via aiocoap-client.
Issues/PRs references
taken over from #14397