-
Notifications
You must be signed in to change notification settings - Fork 240
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
Document why there is no path_filestat_set_size #180
Conversation
The fact that POSIX lacks a Also, IIUC we are explicitly not following POSIX's design so the comment seems a little strange. |
phases/ephemeral/witx/typenames.witx
Outdated
;;; If `path_open` is set, includes the right to invoke `path_open` with `oflags::trunc`. | ||
;;; Note: there is no `path_filestat_set_size`. This follows POSIX design, which only has | ||
;;; `ftruncate` and does not provide `ftruncateat`. |
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 found this confusing to read because there clearly is a thing path_filestat_set_size
called on the line below. I guess you are referring to the fact that there is no addition separate function with this name?
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.
Yes, the intended meaning is that there is no function named __wasi_path_filestat_set_size
.
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.
Could you change the text to add the word "function" here, so that it's clear you're talking about a function by that name (which indeed doesn't exist) rather than a right (which does)?
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.
Done!
@sbc100 On Unix the only possible implementation seems to be Exposing |
;;; If `path_open` is set, includes the right to invoke `path_open` with `oflags::trunc`. | ||
;;; Note: there is no function named `path_filestat_set_size`. This follows POSIX design, | ||
;;; which only has `ftruncate` and does not provide `ftruncateat`. |
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 see. This makes more sense to me now.
Perhaps you could rephrase. This comment makes it sound like we don't have this function because we are following POSIX, which is not that case. How about something like this:
Note: there is no function named `path_filestat_set_size` because on many existing
systems there is no way to efficiently implement this. For example, POSIX has no
`ftruncateat` function.
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.
Sorry for the delay. I rephrased it in a slightly different way, is it okay for you now?
phases/ephemeral/docs.md
Outdated
Note: there is no function named `path_filestat_set_size`. This follows POSIX design, | ||
which only has `ftruncate` and does not provide `ftruncateat`. While such function | ||
would definitely be useful, it would be difficult to implement `path_filestat_set_size` | ||
efficiently on most existing systems. |
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.
Would it be difficult to implement efficiently? It seems we could implement this by just opening the file and doing ftruncate
, which would be reasonable. My understanding is that the main reason we don't have path_filestat_set_size
is that we don't currently have any use cases for it, since no code written for POSIX systems would use it.
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.
Here to implement efficiently is intended to mean to implement using one syscall only, as we consider the cost of other calls negligible.
How about
While such function would be desirable from the API design perspective, there are virtually no use cases for it since no code written for POSIX systems would use it. Moreover, implementing it would require multiple syscalls, leading to inferior performance.
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.
@sunfishcode bump
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.
Thanks for the bump! That wording sounds good to me.
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 pushed the new wording.
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.
Looks good!
As suggested by @sunfishcode in bytecodealliance/wasmtime#273