Reimplement readv to deal with short reads

[yamt]

Summary

The original implementation seems to have an assumption that the file is a regular file.

This commit re-implements it so that it can deal with tty, sockets, etc.

Impact

Testing

[acassis]

@yamt nice! Is there some example showing the failure? Should it be added to apps/testing/ ?

Another question: did you run ostest and LTS locally to confirm there is not known side effects?

[yamt]

@yamt nice! Is there some example that showing the failure? Should it be added to apps/testing/ ?

micropython repl (with https://github.com/micropython/micropython/pull/13676) on toywasm and wamr.

Another question: did you run ostest and LTS locally to confirm there is not known side effects?

no.

[yamt]

The original implementation seems to have an assumption that the file is a regular file.

well, the original implementation was actually broken for regular files as well wrt read/write atomicity.

[yamt]

can we make a decision?

[yamt]

can we make a decision?

ping

[xiaoxiang781216]

can we make a decision?

I would prefer to just read the first memory block, which conform the spec and don't need malloc.

[yamt]

can we make a decision?

I would prefer to just read the first memory block, which conform the spec and don't need malloc.

it doesn't conform the spec.

[xiaoxiang781216]

can we make a decision?

I would prefer to just read the first memory block, which conform the spec and don't need malloc.

it doesn't conform the spec.

Why?

[yamt]

can we make a decision?

I would prefer to just read the first memory block, which conform the spec and don't need malloc.

it doesn't conform the spec.

Why?

consider:

• file types which are usually not interruptible. eg. regular files
• file types which are supposed to preserve data boundaries. eg. udp sockets
• file types which are supposed to preserve read/write atomicity. eg. regular files

[xiaoxiang781216]

can we make a decision?

I would prefer to just read the first memory block, which conform the spec and don't need malloc.

it doesn't conform the spec.

Why?

consider:

• file types which are usually not interruptible. eg. regular files

if spec enforce the implementation must read all available data, please point out the statement.

• file types which are supposed to preserve data boundaries. eg. udp sockets

since udp already support sendmsg, the right fix is mapping writev to sendmsg by checking the fd type is socket.

• file types which are supposed to preserve read/write atomicity. eg. regular files

it's caller responsibility to ensure the atomicity if readv/writev just can finish the partial job.

[xiaoxiang781216]

It's better to do the different action base on the fd type before we add readv/writev to file_operation:

1. forward to sendmsg/recvmsg for socket type
2. read/write as much as possible for regular file
3. read/write the first block for all other type

of course, it may need to move readv/writev from libc to fs/vfs to do this type of dispatch. BTW, to simplify readv/writev dispatch, we can add readv/writev callback to file_operation and provide the default readv/writev implementation if the driver or file system doesn't provide one.

[yamt]

can we make a decision?

I would prefer to just read the first memory block, which conform the spec and don't need malloc.

it doesn't conform the spec.

Why?

consider:

• file types which are usually not interruptible. eg. regular files

if spec enforce the implementation must read all available data, please point out the statement.

i don't know where/if it's explicitly specified in the spec. it's the traditional common behavior which many applications rely on.

• file types which are supposed to preserve data boundaries. eg. udp sockets

since udp already support sendmsg, the right fix is mapping writev to sendmsg by checking the fd type is socket.

udp is just an example. eg. there can be such character devices.

• file types which are supposed to preserve read/write atomicity. eg. regular files

it's caller responsibility to ensure the atomicity if readv/writev just can finish the partial job.

no. search "atomic" in https://pubs.opengroup.org/onlinepubs/009604599/functions/read.html.

[yamt]

It's better to do the different action base on the fd type before we add readv/writev to file_operation:

1. forward to sendmsg/recvmsg for socket type

2. read/write as much as possible for regular file

3. read/write the first block for all other type

of course, it may need to move readv/writev from libc to fs/vfs to do this type of dispatch. BTW, to simplify readv/writev dispatch, we can add readv/writev callback to file_operation and provide the default readv/writev implementation if the driver or file system doesn't provide one.

see https://github.com/apache/nuttx/pull/12674#discussion_r1678639259

[yamt]

i don't think it's a good idea to block an obvious fix like this for months just saying "there can be a better solution" w/o actually providing any better solutions.

if this PR was merged two months ago, i might even have implemented the ideal iov-based solution at least partly in the two months.

[xiaoxiang781216]

can we make a decision?

I would prefer to just read the first memory block, which conform the spec and don't need malloc.

it doesn't conform the spec.

Why?

consider:

• file types which are usually not interruptible. eg. regular files

if spec enforce the implementation must read all available data, please point out the statement.

i don't know where/if it's explicitly specified in the spec. it's the traditional common behavior which many applications rely on.

But the spec explicitly allows return the short length. Caller always need handle the short return correctly.

i don't think it's a good idea to block an obvious fix like this for months just saying "there can be a better solution" w/o actually providing any better solutions.

if this PR was merged two months ago, i might even have implemented the ideal iov-based solution at least partly in the two months.

Since this solution isn't perfect, especially the allocation happens in the read/write path, I hesitate to merge this change, but other maintainers could merge it if they think it's OK.

[yamt]

• file types which are usually not interruptible. eg. regular files

if spec enforce the implementation must read all available data, please point out the statement.

i don't know where/if it's explicitly specified in the spec. it's the traditional common behavior which many applications rely on.

But the spec explicitly allows return the short length. Caller always need handle the short return correctly.

whatever the standard allows, i don't think it's a good idea to break the semantics which real applications have been relying on for decades.

[xiaoxiang781216]

since the better approach is merged(https://github.com/apache/nuttx/pull/13498), let's close this pr.

[yamt]

since the better approach is merged(#13498), let's close this pr.

https://github.com/apache/nuttx/pull/13498 itself doesn't fix the problem i wanted to fix with this PR.

https://github.com/apache/nuttx/pull/14898 does. but it's still open.

[xiaoxiang781216]

@yamt could you fix the conflict? so we can merge this patch.

[xiaoxiang781216]

should we close this pr without merging? @yamt

[yamt]

should we close this pr without merging? @yamt

what this PR does is still valid. however, at this point i'm more interested in finishing https://github.com/apache/nuttx/pull/14898. i will mark this a draft for now.

[yamt]

i'm closing this as https://github.com/apache/nuttx/pull/14898, which fixes the issue for the specific driver i care right now, was merged.