support streaming snapshot over fd #39
Conversation
xsnap/sources/xsnap-worker.c
Outdated
| int writeError = fxWriteOkay(toParent, meterIndex, machine, "", 0); | ||
| // Allows us to format up to 999,999,999,999 bytes (1TiB - 1) | ||
| char fsize[13]; | ||
| int fsizeLength = snprintf(fsize, sizeof(fsize) - 1, "%d", stream.size); |
There was a problem hiding this comment.
Why sizeof(fsize) - 1? snprintf won't NUL-terminate the buffer if it hits this limit, so you have 1 byte of initialized stack in it. In that case, you might actually mean:
| int fsizeLength = snprintf(fsize, sizeof(fsize) - 1, "%d", stream.size); | |
| int fsizeLength = snprintf(fsize, sizeof(fsize) - 1, "%d", stream.size); | |
| fsize[fsizeLength] = '\0'; |
Or on the other hand, if you don't need fsize to be NUL-terminated, maybe you mean:
| int fsizeLength = snprintf(fsize, sizeof(fsize) - 1, "%d", stream.size); | |
| int fsizeLength = snprintf(fsize, sizeof(fsize), "%d", stream.size); |
There was a problem hiding this comment.
I'm guessing this was copied from the snprintf() in fxWriteOkay(), around line 813, which is probably mine. My read of the snprintf man page (on my Mac):
The snprintf() and vsnprintf() functions will write at most size-1 of the characters printed into the output string (the size'th character then gets the terminating ‘\0’); if the return value is greater than or equal to the size argument, the string was too short and some of the printed characters were discarded. The output is always null-terminated, unless size is 0.
is that using sizeof(fsize)-1 means fsize will always be NUL-terminated, even if the argument is too big (the string will be truncated in that case, but it will still be NUL-terminated). I remember worrying about clobbering the stack, and wanted to make sure that even if I miscalculated how large the buffer needed to be, the worst case would be corrupted meter data and not crashing or corrupting the execution.
There was a problem hiding this comment.
Another man quote:
The functions snprintf() and vsnprintf() write at most size bytes (including the terminating null byte ('\0')) to str.
So I assume that yes this is being overly cautious. It does also mean it will only write 11 chars right now, aka 100GiB not 1TiB...
I'll fix both.
There was a problem hiding this comment.
Ah, looks like C99 defines the always-terminate behaviour. I wrote C where not all platforms were C99, so had to be more defensive against the various footguns.
| SnapshotStream stream; | ||
| if (path[0] == '@') { | ||
| int fd = atoi(path + 1); | ||
| int tmpfd = dup(fd); |
There was a problem hiding this comment.
What is the dup() for? Does it allow us to fclose(stream.file) but then re-use the original fd later? Do we have a use for that, or is this a generic pattern that we do need for the writing side?
There was a problem hiding this comment.
We do need it for the writing side, and I figured in the future we may want the ability to reuse the load snapshot stream.
xsnap/sources/xsnap-worker.c
Outdated
| int writeError = fxWriteOkay(toParent, meterIndex, machine, "", 0); | ||
| // Allows us to format up to 999,999,999,999 bytes (1TiB - 1) | ||
| char fsize[13]; | ||
| int fsizeLength = snprintf(fsize, sizeof(fsize) - 1, "%d", stream.size); |
There was a problem hiding this comment.
I'm guessing this was copied from the snprintf() in fxWriteOkay(), around line 813, which is probably mine. My read of the snprintf man page (on my Mac):
The snprintf() and vsnprintf() functions will write at most size-1 of the characters printed into the output string (the size'th character then gets the terminating ‘\0’); if the return value is greater than or equal to the size argument, the string was too short and some of the printed characters were discarded. The output is always null-terminated, unless size is 0.
is that using sizeof(fsize)-1 means fsize will always be NUL-terminated, even if the argument is too big (the string will be truncated in that case, but it will still be NUL-terminated). I remember worrying about clobbering the stack, and wanted to make sure that even if I miscalculated how large the buffer needed to be, the worst case would be corrupted meter data and not crashing or corrupting the execution.
xsnap/sources/xsnap-worker.c
Outdated
| static int fxSnapshotRead(void* stream, void* address, size_t size) | ||
| { | ||
| return (fread(address, size, 1, stream) == 1) ? 0 : errno; | ||
| SnapshotStream* snapshotStream = stream; | ||
| if (snapshotStream->size >= 0 && (signed)(snapshotStream->size - size) < 0) |
There was a problem hiding this comment.
FWIW, I believe the snapshot format is self-delimiting: unless something is confused or corrupted, the fxSnapshotRead() call should exactly consume the last byte of the snapshot, and it will never read beyond the end.
There was a problem hiding this comment.
BTW the (signed) cast is a little bit unnerving. I think the logic is correct, but it might be more obvious if you used a separate boolean flag to mean "we know a size", use size_t remaining instead of int size, and then do purely unsigned comparisons like if (snapshotStream->hasSize && size > snapshotStream->remaining) return EIO;
There was a problem hiding this comment.
Yeah since we're not using this "EOF" style check, I might rip it out entirely.
Introduce a snapshot stream struct to store the size of the data written Respond to snapshot write with written size
mhofman
force-pushed
the
mhofman/stream-snapshot
branch
from
9e78492 to
9d408aa
Compare
Refs: Agoric/agoric-sdk#6363
This introduces support for using an existing fd to read and write the snapshot, allowing streaming to/from the parent process.
A filename starting with
@will denote a fd to use.This also changes the internal structure used when reading/writing the snapshot to keep track of the bytes read/written if applicable. (read limits not currently used as it hasn't been necessary).