nanocoap_vfs: add nanocoap_vfs_get() #17937

benpicco · 2022-04-13T22:11:04Z

Contribution description

This adds a convenience function nanocoap_vfs_get() to download a file from a CoAP server and store it on the local filesystem.

In addition a shell command ncget is provided that makes use of the function.

Testing procedure

Run the tests/nanocoap_cli example. If you are on native you want to enable the vfs_auto_format module to automatically create a file system on the auto-generated virtual storage.

Run aiocoap-fileserver . in e.g. the RIOT root directory.

main(): This is RIOT! (Version: 2022.07-devel-20-g7d76a-nanocoap_vfs)
nanocoap test app
All up, running the shell now
> ls /nvm0
./
../
total 0 files

> ncget coap://[fe80::90a7:a6ff:fe4b:2e32%5]/
/.git
/CODE_OF_CONDUCT.md
/LICENSE
/.github/
/boards/
/dist/
/examples/
/sys/
/build/
/bootloaders/
/core/
/cpu/
/doc/
/drivers/
/makefiles/
/pkg/
/tests/
/.bandit
/.circleci/
/.drone.yml
/.gitattributes
/.gitignore
/.mailmap
/CITATION.cff
/CODEOWNERS
/CODING_CONVENTIONS.md
/CODING_CONVENTIONS_C++.md
/CONTRIBUTING.md
/Kconfig
/LOSTANDFOUND.md
/Makefile
/Makefile.base
/Makefile.dep
/README.md
/SECURITY.md
/Vagrantfile
/doc.txt
/fuzzing/
/kconfigs/
/release-notes.txt
/uncrustify-riot.cfg
/MAINTAINING.md
/keys/
/.murdock
/Makefile.features
/Makefile.include
/coaproot/

> ncget coap://[fe80::90a7:a6ff:fe4b:2e32%5]/LICENSE
> ls /nvm0
./
../
LICENSE	24310 B
total 1 files

> vfs r /nvm0/LICENSE 256
00000000: 2020 2020 2020 2020 2020 2020 2020 2020                  
00000010: 2020 474e 5520 4c45 5353 4552 2047 454e    GNU LESSER GEN
00000020: 4552 414c 2050 5542 4c49 4320 4c49 4345  ERAL PUBLIC LICE
00000030: 4e53 450a 2020 2020 2020 2020 2020 2020  NSE.            
00000040: 2020 2020 2020 2020 2020 2056 6572 7369             Versi
00000050: 6f6e 2032 2e31 2c20 4665 6272 7561 7279  on 2.1, February
00000060: 2031 3939 390a 0a20 436f 7079 7269 6768   1999.. Copyrigh
00000070: 7420 2843 2920 3139 3931 2c20 3139 3939  t (C) 1991, 1999
00000080: 2046 7265 6520 536f 6674 7761 7265 2046   Free Software F
00000090: 6f75 6e64 6174 696f 6e2c 2049 6e63 2e0a  oundation, Inc..
000000a0: 2035 3120 4672 616e 6b6c 696e 2053 7472   51 Franklin Str
000000b0: 6565 742c 2046 6966 7468 2046 6c6f 6f72  eet, Fifth Floor
000000c0: 2c20 426f 7374 6f6e 2c20 4d41 2020 3032  , Boston, MA  02
000000d0: 3131 302d 3133 3031 2020 5553 410a 2045  110-1301  USA. E
000000e0: 7665 7279 6f6e 6520 6973 2070 6572 6d69  veryone is permi
000000f0: 7474 6564 2074 6f20 636f 7079 2061 6e64  tted to copy and

Issues/PRs references

chrysn

Do you have any plans for nput (cvfs-put, coap upload?) as well?

sys/shell/commands/shell_commands.c

sys/shell/commands/sc_nanocoap_vfs.c

chrysn · 2022-04-15T12:51:58Z

sys/shell/commands/sc_nanocoap_vfs.c

+        return -EINVAL;
+    }
+
+    if (_is_dir(url)) {


I'm not sure if this is too much relying on the file system semantics -- or whether it's just a practical default, especially as the trailing slash gives no practical local file name anyway.

So what if someone tries to download coap://host/path/? If they give an output file name, that might be fully OK (given that host doesn't necessarily have file system semantics). If they do not given an output file name, strrchr(url, '/') later will find the trailing slash, set dst = "/default-data/", and vfs_open will fail to open that as a file for writing.

So I don't see a good reason for that check.

You mean if someone wanted to actually store the resource behind coap://host/path/ instead of doing a directory listing?

I'm not sure, is this really a use case for the shell utility? Maybe I have a narrow view because I only used few CoAP servers, but I thought <path>/ would always give a resource listing, not an actual resource.

That's only a directory listing if what is behind that is a file server. If it's a pubsub broker it could be a topic, or on an RD it could be part of lookup. CoAP by itself has no concepts of files and directories, and even when acting as a client that saves data to a file system, we shouldn't make assumptions on the server.

I bet that in every second CoAP implementation a trailing slash would not survive the encoding / parsing process. See https://datatracker.ietf.org/doc/html/rfc7252#section-6.4 for details.

My understanding is that a trailing / should result in an additional empty URI-Path option. But I have the feeling that many implementation will just "normalize" that out.

There have been some, but they have been fixed. Servers for HTTP are more lax in that area, but a) CoAP tries to avoid some mistakes HTTP made, and b) unlike in HTTP there is no simple way to redirect to the other form (and serving the same document on can easily cause incorrect outcomes, which is why even the lax HTTP servers redirect).

But would it make sense to store this on the file system?

What do you mean?

My proposal is to simply remove this one check. Then any URI could be ncget'ted. It may make sense to have a check for the (possibly implicit) output file name to not be empty (as the file system error is likely not helpful to the user). Just like wget http://riot-os.org/ -Owebsite.html makes sense, something like ncget coap://temperature.riot-os.org/temperatures/average/vienna/ latest-local.temp can happen.

Well right now if you do a ncget on a / path you get a parsed directory listing to stdout for the user's convenience:

> ncget coap://[fe80::90a7:a6ff:fe4b:2e32%5]/core/ /core/include/ /core/cond.c /core/doc.txt /core/mbox.c /core/msg_bus.c /core/mutex.c /core/thread_flags.c /core/Makefile /core/lib/ /core/msg.c /core/Kconfig /core/sched.c /core/thread.c

If you to a ncget on a 'file' it will be downloaded instead:

ncget coap://[fe80::90a7:a6ff:fe4b:2e32%5]/core/thread.c Saved as /nvm0/thread.c

unless you specify stdout as the destination

ncget coap://[fe80::90a7:a6ff:fe4b:2e32%5]/core/thread.c - /* * Copyright (C) 2013 Freie Universität Berlin * * This file is subject to the terms and conditions of the GNU Lesser * General Public License v2.1. See the file LICENSE in the top level * directory for more details. */ /** * @ingroup core_thread * @{ * * @file * @brief Threading implementation …

Well you're interacting with a file server -- but not every server is, and it's part of the flexibility of web protocols that you can. I don't regularly interact with IPSO servers, but IIRC almost all of their resources have trailing slashes.

I don't even object to the default destination of paths with trailing slashes being - (as is implied here), that's a very sensible thing to do. But if a user wanted to save it (eg. because some device makes its most frequently accessed resource available on the empty path), that should not be ruled out.

Ah then this is a simple change! - cfdd360

sys/shell/commands/sc_nanocoap_vfs.c

benpicco · 2022-04-15T13:29:51Z

Do you have any plans for nput (cvfs-put, coap upload?) as well?

Now that aiocoap-fileserver has write support it's the only logical conclusion 😄

maribu · 2022-05-03T08:36:59Z

@chrysn do you want to take another look? Otherwise I'd do some testing and (assuming positive outcome) give an ACK.

chrysn

Didn't do a full check (as maribu already mentioned the pending ACK), but didn't find any blockers; address comments as you deem adequate.

chrysn · 2022-05-03T09:34:31Z

sys/shell/commands/sc_nanocoap_vfs.c

+        return -EINVAL;
+    }
+
+    if (_is_dir(url)) {


So what if someone tries to download coap://host/path/? If they give an output file name, that might be fully OK (given that host doesn't necessarily have file system semantics). If they do not given an output file name, strrchr(url, '/') later will find the trailing slash, set dst = "/default-data/", and vfs_open will fail to open that as a file for writing.

So I don't see a good reason for that check.

chrysn · 2022-05-03T09:38:43Z

sys/include/net/nanocoap_vfs.h

+ * @returns     0 on success
+ * @returns     <0 on error
+ */
+int nanocoap_vfs_get(nanocoap_sock_t *sock, const char *path, const char *dst);


Whenever there is a pair like this (get vs. get_url), I wonder when to use which, if the use cases overlap or whether there are combinations that would be needed. (For example, can one use an established socket with a URL that has query components, or a Uri-Host component?)

Here it's probably motivated by the duality of nanocoap_sock_get_blockwise and nanocoap_get_blockwise_url, where I should have asked the question earlier.

I'd appreciate a note on when to use which, but maybe there are no good answers until #13827 is through.

nanocoap_vfs_get_url() is just there for convenience to avoid boilerplate in the common case.

The path component (and everything that goes with it) will be parsed the same way for both in nanocoap_sock_get_blockwise().

So the path argument is really more path and query? If so, that should be documented (dejavu, I think we've had this somewhere else already.)

Can I just call it query path?

maribu

I only reviewed partially, but since I often forget to complete a review and leave the already provided comments in limbo, I rather click submit now half-way-through.

sys/include/net/nanocoap_vfs.h

benpicco · 2022-05-23T20:34:16Z

Should I squash?

maribu · 2022-05-24T04:55:53Z

Please squash and rebase (:

maribu

ACK. Code looks good and I trust the provided testing.

maribu · 2022-05-24T11:45:55Z

sys/include/net/nanocoap_vfs.h

+ * @returns     0 on success
+ * @returns     <0 on error


Suggested change

* @returns 0 on success

* @returns <0 on error

* @retval 0 success

* @retval <0 error

benpicco requested review from smlng, leandrolanzieri, aabadie, MichelRottleuthner, fjmolinas, kaspar030, bergzand, miri64, haukepetersen, PeterKietzmann and cgundogan as code owners April 13, 2022 22:11

benpicco requested review from chrysn and maribu April 13, 2022 22:11

github-actions bot added Area: CoAP Area: network Area: OTA Area: sys Area: tests labels Apr 13, 2022

benpicco force-pushed the nanocoap_vfs branch from 34285bf to 2768161 Compare April 13, 2022 22:26

github-actions bot added the Area: examples label Apr 13, 2022

benpicco force-pushed the nanocoap_vfs branch from 2768161 to 414f73f Compare April 13, 2022 23:18

benpicco added CI: ready for build and removed Area: OTA labels Apr 13, 2022

benpicco force-pushed the nanocoap_vfs branch from 414f73f to bf5e191 Compare April 15, 2022 12:29

github-actions bot added the Area: OTA label Apr 15, 2022

benpicco force-pushed the nanocoap_vfs branch from bf5e191 to 6d5af19 Compare April 15, 2022 12:30

chrysn reviewed Apr 15, 2022

View reviewed changes

benpicco force-pushed the nanocoap_vfs branch from c2b09d6 to 597e8b9 Compare April 15, 2022 16:47

benpicco mentioned this pull request Apr 18, 2022

nanocoap_vfs: add nanocoap_vfs_put() #17962

Merged

benpicco mentioned this pull request Apr 29, 2022

nanocoap_sock: only abort nanocoap_sock_get_blockwise() on negative error #18037

Merged

benpicco removed the CI: ready for build label Apr 29, 2022

benpicco force-pushed the nanocoap_vfs branch from a41c3d3 to 5b8ea0d Compare May 1, 2022 23:05

benpicco added the CI: ready for build label May 1, 2022

maribu added Reviewed: 1-fundamentals Reviewed: 2-code-design Reviewed: 4-code-style Reviewed: 5-documentation labels May 3, 2022

chrysn reviewed May 3, 2022

View reviewed changes

maribu reviewed May 17, 2022

View reviewed changes

sys/include/net/nanocoap_vfs.h Show resolved Hide resolved

benpicco removed the CI: ready for build label May 17, 2022

benpicco added 4 commits May 24, 2022 09:32

nanocoap: define default block size

ad7d974

nanocoap_vfs: add nanocoap_vfs_get()

87359bb

sys/shell: add 'nget' shell command

4dd971f

tests/nanocoap_cli: enable nget command

5fde41e

benpicco force-pushed the nanocoap_vfs branch from b79f3e0 to 5fde41e Compare May 24, 2022 07:35

benpicco added the CI: ready for build label May 24, 2022

benpicco requested a review from fabian18 May 24, 2022 11:28

maribu approved these changes May 24, 2022

View reviewed changes

benpicco merged commit 85169fc into RIOT-OS:master May 24, 2022

benpicco deleted the nanocoap_vfs branch May 24, 2022 14:10

chrysn added this to the Release 2022.07 milestone Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nanocoap_vfs: add nanocoap_vfs_get() #17937

nanocoap_vfs: add nanocoap_vfs_get() #17937

benpicco commented Apr 13, 2022 •

edited

Loading

chrysn left a comment

chrysn Apr 15, 2022

chrysn May 3, 2022

benpicco May 3, 2022

chrysn May 3, 2022

maribu May 3, 2022 •

edited

Loading

chrysn May 3, 2022

chrysn May 3, 2022

benpicco May 3, 2022 •

edited

Loading

chrysn May 3, 2022

benpicco May 3, 2022 •

edited

Loading

benpicco commented Apr 15, 2022

maribu commented May 3, 2022

chrysn left a comment

chrysn May 3, 2022

chrysn May 3, 2022

benpicco May 3, 2022

chrysn May 3, 2022

benpicco May 3, 2022

maribu left a comment

benpicco commented May 23, 2022

maribu commented May 24, 2022

maribu left a comment

maribu May 24, 2022

nanocoap_vfs: add nanocoap_vfs_get() #17937

nanocoap_vfs: add nanocoap_vfs_get() #17937

Conversation

benpicco commented Apr 13, 2022 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

chrysn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribu May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco May 3, 2022 • edited Loading

Choose a reason for hiding this comment

benpicco commented Apr 15, 2022

maribu commented May 3, 2022

chrysn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribu left a comment

Choose a reason for hiding this comment

benpicco commented May 23, 2022

maribu commented May 24, 2022

maribu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco commented Apr 13, 2022 •

edited

Loading

maribu May 3, 2022 •

edited

Loading

benpicco May 3, 2022 •

edited

Loading

benpicco May 3, 2022 •

edited

Loading