gcoap: Add file server #14397

chrysn · 2020-06-30T13:12:52Z

Contribution description

This ties in the Gcoap module with the VFS module in the style of a static web server.

This is quite minimal (no write support, no blockwise, no ETag), but already fully documented. The example commit is marked WIP as it's not sure yet where to best put the example.

Testing procedure

Currently: Run the gcoap example, and GET /vfs/const/ or GET /vfs/const/hello-world, which should show a directory listing or the file content, respectively.

chrysn · 2020-06-30T19:20:21Z

There is now support for ETag and block-wise transfer. (Not on directory listings yet, though).

Example-wise, I'll abandon piggy-backing on gcoap and instead add a few lines (sic: 2 lines header, 17 lines static structs, 2 lines code to main) to the filesystem example, conditionally included. Creating good example directories and mouting/formatting is way too much hassle otherwise.

cgundogan · 2020-07-08T08:06:48Z

There is now support for ETag

👍

I'd like to see the ETag mechanism done in gcoap, so it can be reused for different scenarios (not only serving files). We can generalize it later, if you want. I'll take a deeper look at the provided implementation here.

benpicco · 2020-07-27T16:28:21Z

Is this still a draft?

benpicco · 2020-07-27T16:28:40Z

(Sorry, wrong button)

chrysn · 2020-07-28T08:31:32Z

Is this still a draft?

Yes; one of the reasons is that it'd be a bit stricter on the file system API than that caters for -- for example, it struggles with the different file systems's directory listing responses of one returning "/entry" and the other "entry". URIs need a bit more precision there than paths: "//entry" may work on the file system, but means something vastly different in a URI. (Also, I've been on vacation for the last two-and-a-half weeks and at the IETF meeting right now, so I'll really only be back at RIOT issues around the end of the week).

benpicco · 2020-07-28T08:38:31Z

it struggles with the different file systems's directory listing responses of one returning "/entry" and the other "entry".

That sounds like a bug in the FS abstraction. What file systems are affected?

chrysn · 2020-07-28T08:53:41Z

That sounds like a bug in the FS abstraction. What file systems are affected?

In the file system example on master, I see (with fatfs configured and a prepared image): ls /const /hello-world /hello-riot total 2 files mount ls /sda FOO EMPTY FOLDER GARBAGE total 4 files while on the default littlefs config, I see: ls /const /hello-world /hello-riot total 2 files format mount tee /sda/foo hello ls /sda /. /.. /foo total 3 files (and carelessly destroyed my fatfs image :-/ )

This is starting point for a file server. It works but does not do block-wise (truncating large files or directory listings), ETag or write support, and fails to handle read errors (after a successful open) properly.

The note on partially cached versions is not applicable yet (for lack of block-wise support), but will become relevant.

The filesystem example is ideal to demonstrate the fileserver as it has a usable default set of demo files, and different file systems can be mounted for experimentation. The impact on the filesystem example outside the coapfileserver context is minimal: Only a few lines of includes (2), static configuration (17 including comments and whitespace) and startup code (2) are added under ifdef guards that clearly mark them as optional and Gcoap related. They are only enabled if NETWORK=1 is set.

Workaround-For: RIOT-OS#14635

With the "resolved agains the anchor" reading obsolete, link targets can easily be expressed relatively.

leandrolanzieri

Nice contribution! I made some initial comments, I'll be testing this shortly.

leandrolanzieri · 2020-10-26T08:50:37Z

sys/net/application_layer/coapfileserver/coapfileserver.c

+
+    /* Normalizing fields whose value can change without affecting the ETag */
+    stattag->stat.st_nlink = 0;
+    memset(&stattag->stat.st_atim, 0, sizeof(stattag->stat.st_atim));


I'm getting errors on this member access when compiling for cortex.

Suggested change

memset(&stattag->stat.st_atim, 0, sizeof(stattag->stat.st_atim));

memset(&stattag->stat.st_atime, 0, sizeof(stattag->stat.st_atime));

leandrolanzieri · 2020-10-26T08:51:02Z

sys/net/application_layer/coapfileserver/coapfileserver.c

+#include <net/gcoap.h>
+#include <vfs.h>
+#include <fcntl.h>
+#include <error.h>


Also getting errors with this inclusion, is it needed?

leandrolanzieri · 2020-10-26T08:51:13Z

sys/net/application_layer/coapfileserver/coapfileserver.c

+#include <fcntl.h>
+#include <error.h>
+
+#define ENABLE_DEBUG 1


Suggested change

#define ENABLE_DEBUG 1

#define ENABLE_DEBUG 0

leandrolanzieri · 2020-10-26T08:52:49Z

sys/net/application_layer/coapfileserver/Makefile

+
+SRC = coapfileserver.c
+


Not needed, the file should be included automatically:

Suggested change

SRC = coapfileserver.c

leandrolanzieri · 2020-10-26T09:01:12Z

sys/net/application_layer/coapfileserver/coapfileserver.c

+#include "debug.h"
+
+/** Maximum length of an expressible path, including the trailing 0 character. */
+#define COAPFILESERVER_PATH_MAX (64)


Could this value be configured?

Could be made, but I'm hoping to get rid of the need to assemble any paths by using something openat-like (issue or PR TBD).

leandrolanzieri · 2020-10-26T09:10:43Z

sys/net/application_layer/coapfileserver/doc.txt

+ * As usual, GET operations are used to read files<!-- WRITESUPPORT, and PUT writes to files.
+ * In the current implementation, PUTs are expressed as random-access, meaning
+ * that files are not updated atomically -->.


Why are there some parts commented out?

Because I had write support planned to go in from the start, had the documentation already written in the first step, and don't want to have to write it all again later on. It'll better if I move them over to a nascent write-support-for-fileserver branch instead.

leandrolanzieri · 2020-10-26T09:15:40Z

sys/net/application_layer/coapfileserver/doc.txt

+ *   ```
+ *
+ *   The allowed methods dictate whether it's read-only (``COAP_GET``) or (in the
+ *   future<!-- WRITESUPPORT -->) read-write (``1COAP_GET | COAP_PUT | COAP_DELETE``).


Suggested change

* future) read-write (``1COAP_GET | COAP_PUT | COAP_DELETE``).

* future) read-write (``COAP_GET | COAP_PUT | COAP_DELETE``).

leandrolanzieri · 2020-10-26T09:16:16Z

sys/net/application_layer/coapfileserver/doc.txt

+ * # About
+ * 
+ * This maps files in the local file system onto a resources in CoAP. In that,
+ * is is what is called a static web server in the unconstrained web.


Suggested change

* is is what is called a static web server in the unconstrained web.

* it is what is called a static web server in the unconstrained web.

leandrolanzieri · 2020-10-26T12:44:14Z

examples/filesystem/main.c

+static const struct coapfileserver_entry vfs = { "", 1 };
+
+/* CoAP resources. Must be sorted by path (ASCII order). */
+static const coap_resource_t _resources[] = {
+    { "/vfs", COAP_GET | COAP_MATCH_SUBTREE, coapfileserver_handler, (void*)&vfs },
+};
+
+static gcoap_listener_t _listener = {
+    &_resources[0],
+    ARRAY_SIZE(_resources),
+    NULL,
+    NULL
+};
+


Would it make sense to move this to an auto-init function? Is there a reason why this needs to be registered from the application?

Only if it were to be made configurable as to the file system entry point and to the coap entry point -- we really shouldn't pick resource names on users' CoAP servers. (And there'd still need to allow manual management for things like ACLs, or when different mount points need different treatment on the CoAP side, eg. w/rt writability).

AFAIK we currently don't do any automatically inserted CoAP resources (other than /.well-known/core). Doing that efficiently would mean putting sorted entries into an array, AFAICT that can't be done even with XFA (#15002) and'd thus need pre-sorting by wherethroughever the configured paths are set. (For example, if coapfileserver did this and so did a hypothetical resourcedirectory, they'd need to be put as ["/.well-known/core", "/coapfileserver", "/rd"] with the one set of names but as ["/.well-known/core", "/cord-server", "/files"] with others).

I'll give it a try, but it may result in an "I'd rather do this as a next step".

Only if it were to be made configurable as to the file system entry point and to the coap entry point -- we really shouldn't pick resource names on users' CoAP servers. (And there'd still need to allow manual management for things like ACLs, or when different mount points need different treatment on the CoAP side, eg. w/rt writability).

Agreed, I was thinking as well on making the entry point configurable.

Doing that efficiently would mean putting sorted entries into an array

Yeah, that's true. Maybe if we could adapt the path matching function on nanocoap we could get rid of the alphabetical ordering constraint? In any case, if that's needed it would be work for a follow up PR.

Maybe if we could adapt the path matching function on nanocoap

Sorry, meant the gcoap matching, that's the one that enforces this.

chrysn · 2020-10-26T14:23:43Z

Thanks for the review; most points can be trivially addressed with the next batch (which may take a few days though).

The st_atim vs. st_atime and include parts are a bit icky as the otherwise RIOT-style vfs calls fan out to whatever the currently used libc decides to do about POSIX stat properties, but I'll manage and try building for some more platforms.

stale · 2021-06-02T16:37:05Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

fjmolinas · 2022-04-04T11:11:24Z

Is this one something that could be revived @chrysn?

chrysn · 2022-04-04T12:18:12Z

Hm, there is now a file server in the rust-gcoap example. Is there much purpose in duplicating that? (There are some features in here, which are not in there yet; these should be ported over.)

benpicco · 2022-04-04T15:32:54Z

Is the rust server usable from C applications? How big is the overhead compared to the C implementation?

fjmolinas · 2022-04-04T15:39:27Z

Hm, there is now a file server in the rust-gcoap example. Is there much purpose in duplicating that? (There are some features in here, which are not in there yet; these should be ported over.)

For me this is still useful mainly because don't want to depend on rust: I still have not picked up rust, and I work with people that do not code in rust. But I can understand if you have no interest in pushing this.

chrysn · 2022-04-04T17:32:40Z

Is the rust server usable from C applications?

It will be once #16785 (similar to the above #14397 (comment)) and #16833 are done (maybe even only the latter, if it registers somewhere in-tree).

How big is the overhead compared to the C implementation?

That is one reason I wrote the Rust one, but I'm not at feature parity (and then will have to strip down the Rust one again as it can already do some things the C one can't).

Also, it'll be a nice comparison for gcoap and nanocoap; I didn't wrap nanocoap yet, but it's on my rough agenda. (Plus there's a Rust CoAP implementation competing).

I still have not picked up rust, and I work with people that do not code in rust.

That shouldn't be a problem once the above things are closed (any more than not speaking C++ is if a pkg uses that). However, platform support for MIPS, AVR and the pre-RISC-V ESPs.

But I can understand if you have no interest in pushing this.

It should be usable as it is already, just not at the quality level I expect from RIOT components.

chrysn · 2022-04-13T11:20:21Z

This allows quick revalidation: "I have a representation, but its Max-Age (default 60s) expires. It had tag 1234, so send anything new, or tell me it's still good." -- "2.03 Valid, ETag 1234 is good (for the next 60s)"

miri64 · 2022-04-13T12:01:55Z

This allows quick revalidation: "I have a representation, but its Max-Age (default 60s) expires. It had tag 1234, so send anything new, or tell me it's still good." -- "2.03 Valid, ETag 1234 is good (for the next 60s)"

See also #17888 for the client-side :-)

benpicco · 2022-04-15T13:34:22Z

I rebased this and applied the suggested fixes to my branch but I noticed something off:

$ aiocoap-client coap://[fe80::58b0:8ff:fe67:ad82%tapbr0]/vfs/            
4.04 Not Found

$ aiocoap-client coap://[fe80::58b0:8ff:fe67:ad82%tapbr0]/vfs/const/
application/link-format content was re-formatted
<hello-world>,
<hello-riot>

$ aiocoap-client coap://[fe80::58b0:8ff:fe67:ad82%tapbr0]/vfs/nvm0/
4.04 Not Found

sys/net/application_layer/coapfileserver/coapfileserver.c

chrysn · 2022-04-15T14:00:27Z

$ aiocoap-client coap://[fe80::58b0:8ff:fe67:ad82%tapbr0]/vfs/
4.04 Not Found

That is fine -- at least that was theresolution of #15291. The Rust based server, on directory access, looks both at the directory and at the mount point list, and serves entries from either. Of course, that's more complex on the client side than if just VFS added mount points magically, but then again, the code here is only used for relatively high-level applications (special purpose applications, even when using a file server, might not span mount points in the first place). Moreover, it's kind of practical in the file server to treat mount points special, as they can have df style metadata that regular directories don't have.

(The nvm0 is something different, you found that already).

benpicco · 2022-04-15T15:10:07Z

sys/net/application_layer/coapfileserver/coapfileserver.c

+                request.namebuf[namelength] = '/';
+                memcpy(&request.namebuf[namelength] + 1, value, optlen);
+                namelength = newlength;


Is there really no way in GCoap to provide the handler with the request URL, so that the handler doesn't have to parse the coap packet itself?

chrysn · 2022-04-15T15:34:05Z

The request URL is a very complex beast, why would you want that? You can't pluck the URL (or its path component) into the FS any better than the options, plus it's a lot of string processing. If anything it'd make sense to access it as a CRI (cf <#13827>), but that'd be very similar in the end to what is here now.

benpicco · 2022-04-15T15:43:09Z

I was thinking of something like

char uri[CONFIG_NANOCOAP_URI_MAX];
coap_get_uri_path(pkt, uri);
[…]
char *sub_path = &uri[strlen(resource->path)]);
resource->handler(pkt, resp_buf, resp_buf_len, sub_path, resource->context);

That would allow us to get rid of the uneconomic strip_path parameter.

chrysn · 2022-04-15T15:49:05Z

Dealing in URIs would mean a great lot of reserved character and percent encoding shenanigans, I wouldn't want to have that where it's perfectly avoidable. A pointer to CRI components starting at the first unprocessed one sounds more practical. (Note that the server may still need access to the processed ones too, eg if it is to serve link-format listings, as these practically don't have <./file> style local references, only </path/deep/file> ones.

benpicco · 2022-04-15T15:51:12Z

Hm wouldn't it still help if the handler had knowledge of the resource path that it's supposed to handle?

So

resource->handler(pkt, resp_buf, resp_buf_len, resource->path, resource->context);

maybe?

chrysn · 2022-04-15T15:52:28Z

That's what I mean by the last unprocessed element. [edit: Wrong reading, see next comment]

chrysn · 2022-04-15T19:12:06Z

Ah, sorry, I misunderstood your comment.

Yes, passing the entry to the handler as a whole seems like a viable option; might even just pass resource in rather than ->path and ->context. (For example, that'd allow the handler to tell whether or not it is configured for POST and DELETE, if it'd want to know whether or not to advertise these featuers).

chrysn added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: CoAP Area: Constrained Application Protocol implementations labels Jun 30, 2020

chrysn requested review from cgundogan, haukepetersen, miri64 and PeterKietzmann as code owners June 30, 2020 13:12

chrysn self-assigned this Jun 30, 2020

chrysn marked this pull request as draft June 30, 2020 13:13

benpicco closed this Jul 27, 2020

benpicco reopened this Jul 27, 2020

chrysn mentioned this pull request Jul 28, 2020

File systems report names with leading slashes #14635

Closed

2 tasks

chrysn added 11 commits October 24, 2020 01:10

coapfileserver: New module to serve VFS via CoAP

2bc74a6

This is starting point for a file server. It works but does not do block-wise (truncating large files or directory listings), ETag or write support, and fails to handle read errors (after a successful open) properly.

coap: Add ETag code from RFC7252

abe369f

coapfileserver: Add support for ETag

4ae2c60

The note on partially cached versions is not applicable yet (for lack of block-wise support), but will become relevant.

coapfileserver: Serve files blockwise

5ac9898

coapfileserver: Error handling

7d02c94

coapfileserver: Work around different slash conventions

73b2ccf

Workaround-For: RIOT-OS#14635

coapfileserver: Hide . and ..

1c3f63f

coapfileserver: Work around missing stat in fatfs

604dc91

coapfileserver: Server links with correct trailing slashes

527d37a

coapfileserver: Adopt different reading of RFC6690

893874c

With the "resolved agains the anchor" reading obsolete, link targets can easily be expressed relatively.

leandrolanzieri reviewed Oct 26, 2020

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jun 2, 2021

stale bot closed this Jul 8, 2021

fjmolinas pushed a commit to fjmolinas/RIOT that referenced this pull request Apr 14, 2022

MERGECOMMIT: gcoap: Add file server RIOT-OS#14397

c1291b6

fjmolinas pushed a commit to thingsat/RIOT that referenced this pull request Apr 15, 2022

MERGECOMMIT: gcoap: Add file server RIOT-OS#14397

7249cbe

fjmolinas pushed a commit to thingsat/RIOT that referenced this pull request Apr 15, 2022

MERGECOMMIT: gcoap: Add file server RIOT-OS#14397

03fe5e6

fjmolinas pushed a commit to thingsat/RIOT that referenced this pull request Apr 15, 2022

MERGECOMMIT: gcoap: Add file server RIOT-OS#14397

ba1ad62

benpicco reviewed Apr 15, 2022

View reviewed changes

sys/net/application_layer/coapfileserver/coapfileserver.c Show resolved Hide resolved

benpicco reviewed Apr 15, 2022

View reviewed changes

benpicco mentioned this pull request Apr 15, 2022

gcoap: Add file server #17956

Merged

chrysn mentioned this pull request Apr 15, 2022

Align uri_parser's output with CRIs #13827

Open

This was referenced Apr 15, 2022

nanocoap: define and use coap_request_ctx_t for request handlers #17957

Merged

vfs: fall back to fstat() if stat() is not implemented #17966

Closed

	memset(&stattag->stat.st_atim, 0, sizeof(stattag->stat.st_atim));
	memset(&stattag->stat.st_atime, 0, sizeof(stattag->stat.st_atime));

	* future<!-- WRITESUPPORT -->) read-write (``1COAP_GET \| COAP_PUT \| COAP_DELETE``).
	* future<!-- WRITESUPPORT -->) read-write (``COAP_GET \| COAP_PUT \| COAP_DELETE``).

	* is is what is called a static web server in the unconstrained web.
	* it is what is called a static web server in the unconstrained web.

gcoap: Add file server #14397

gcoap: Add file server #14397

Conversation

chrysn commented Jun 30, 2020

Contribution description

Testing procedure

chrysn commented Jun 30, 2020

cgundogan commented Jul 8, 2020

benpicco commented Jul 27, 2020

benpicco commented Jul 27, 2020

chrysn commented Jul 28, 2020 via email

benpicco commented Jul 28, 2020

chrysn commented Jul 28, 2020 via email

leandrolanzieri 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrysn commented Oct 26, 2020

stale bot commented Jun 2, 2021

fjmolinas commented Apr 4, 2022

chrysn commented Apr 4, 2022 via email

benpicco commented Apr 4, 2022

fjmolinas commented Apr 4, 2022

chrysn commented Apr 4, 2022

chrysn commented Apr 13, 2022 via email

miri64 commented Apr 13, 2022

benpicco commented Apr 15, 2022 • edited Loading

chrysn commented Apr 15, 2022

Choose a reason for hiding this comment

chrysn commented Apr 15, 2022 via email

benpicco commented Apr 15, 2022 • edited Loading

chrysn commented Apr 15, 2022 via email

benpicco commented Apr 15, 2022 • edited Loading

chrysn commented Apr 15, 2022 via email • edited Loading

chrysn commented Apr 15, 2022

benpicco commented Apr 15, 2022 •

edited

Loading

benpicco commented Apr 15, 2022 •

edited

Loading

benpicco commented Apr 15, 2022 •

edited

Loading

chrysn commented Apr 15, 2022 via email •

edited

Loading