Page MenuHomeFreeBSD

Add sponge(1)
ClosedPublic

Authored by eadler on Nov 1 2017, 7:55 PM.
Tags
None
Referenced Files
F80105967: D12900.id36169.diff
Thu, Mar 28, 12:05 AM
Unknown Object (File)
Thu, Mar 7, 1:26 PM
Unknown Object (File)
Feb 7 2024, 12:39 PM
Unknown Object (File)
Jan 27 2024, 10:41 PM
Unknown Object (File)
Jan 27 2024, 5:57 AM
Unknown Object (File)
Jan 26 2024, 9:18 AM
Unknown Object (File)
Jan 19 2024, 2:29 PM
Unknown Object (File)
Jan 18 2024, 7:55 PM

Details

Reviewers
None
Group Reviewers
manpages
Commits
rS326555: sponge(1) Minor commit for commit log
Summary

This adds a naive implementation of sponge(1) to base.

sponge(1) is useful in pipelines to write to files read earlier without
setting up a temporary file inline

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 13311
Build 13545: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I like it. This looks great, but I'm concerned about performance for larger files since realloc copies data and can wind up with a fragmented heap that might artificially limit what you can do. Simple lists of ranges would suffice to avoid these copies without greatly increasing the complexity of the code.

usr.bin/sponge/sponge.1
49

It might be useful to specify what happens when memory can't be allocated for the entire file.

usr.bin/sponge/sponge.c
21

so if we fail to allocate, no output is produced. If so, this is OK.

89

why *4? This artificially may limit the file size as we approach the VM limits. *2 will result in more allocation but will let us get much closer to the limit without going nuts.

Also, this strategy is great for small files, but sucks for large files since we call realloc, which could be copying a crapton of data to keep the output code simple.

111

Why mix raw access and stdio? Seems like we could easily use all one or the other.

118

There's no harm in always closing the output, we're about to exit anyway.

Please wait for the next update and then review

usr.bin/sponge/sponge.c
111

because I was using printf for debugging and Just Worked. replacing with writev to avoid the reallocf problem you mentioned

118

true, but I dislike it, and it makes debugging annoying.

Hi Eitan,

how about adding a small EXAMPLE section to the man page to show usage?

danfe added inline comments.
usr.bin/sponge/sponge.c
21

style(9) violation: should be static void *\nsafe_malloc.

28

return (ret);

31

Ditto.

38

Ditto.

48

Ditto.

52

Move { to the next line.

53

Usage is typically sent to stderr.

57

Ditto.

66

Local variables are utterly unsorted. See style(9).

eadler marked 13 inline comments as done.

Attempt style(9)

An alternative implementation would write to a temporary file, then rename onto the destination once stdin closes. This is how ksh93's >;pathname redirection works (this does not mean I think this definitely should be done like that, or that the shell is the best place to implement this feature).

usr.bin/sponge/sponge.1
27

Convention seems to be not to add the leading zero here.

54

We should perhaps add a warning that the file will be written even if earlier components of the pipeline failed.

usr.bin/sponge/sponge.c
16

whitespace style: static void *safe_malloc...

78

style: spaces

80

-h for help is an unusual convention. It is very common for -h to mean something else, such as affecting a symlink instead of its target or making output more human-readable.

117

This memory is leaked (never used).

133

You can use err(3) here and elsewhere to simplify the code a bit.

137

If there are too many iovecs (sysconf(_SC_IOV_MAX), 16 MB with 16 KB buffers on FreeBSD), writev() will do nothing and return the error [EINVAL]. To avoid this, the write should be split up.

If sysconf(_SC_IOV_MAX) returns -1 or if you want to simplify the code, use the constant _XOPEN_IOV_MAX. This may lead to more system calls, but that may not be a problem.

144

The error from close() should be checked.

Some more style(9) issues...

usr.bin/sponge/sponge.c
20

Per our style(9), * should be part of the variable, not the type. Also applies to prototypes above.

23

Local variables are typically separated from the rest of of the function with extra linefeed.

29

Still not done.

35

Ditto.

41

Again, return (ret);.

44

Again, should be static void * (note the space).

47

Missing linefeed.

53

Again, return (ret);.

99

Per style(9), infinite loops are spelled as for (;;) in FreeBSD.

eadler marked an inline comment as done.

almost there...

usr.bin/sponge/sponge.1
54

Its the same for all unix commands, but its likely worth adding a reminder here.

usr.bin/sponge/sponge.c
80

It is annoying to me when utilities use -h for anything other than help.

137

this is ridiculous. Will fix in the next iteration.

wblock added inline comments.
usr.bin/sponge/sponge.1
41

Redundant.

utility reads standard in until complete, then opens
​the output file and writes to it.
42

I would say "useful" instead of "valuable".

43
This makes it useful in pipelines that read a file and then write to it.
44

s/The following/These/

47

s/Opens/Open/

52

s/In the event that/If/
and this needs a comma and some other stuff. I doubt you are referring to a specific malloc, but rather to memory allocation in general. So:

If an attempt to allocate memory fails,
55

s/will be/is/

55

Seems like this ought to be in a BUGS section at the end.

56

s/have//

68

This should use double quotes. Too bad we don't have a macro for air quotes, because that's pretty much what this is.

69
.Cm sort file | sponge file
eadler marked 30 inline comments as done.

style(9)

usr.bin/sponge/sponge.1
55

this isn't a bug. Its statement on how shells work.

usr.bin/sponge/sponge.1
54

missing space in iswritten

usr.bin/sponge/sponge.c
137

Ok, but to be portable you still need something like

if (maxiovec == -1)
    maxiovec =  _XOPEN_IOV_MAX;

since I expect haters of fixed limits to return -1 from sysconf(_SC_IOV_MAX).

eadler marked an inline comment as done.

last iteration - I hope!

eadler marked an inline comment as done.

add unit test

This revision was automatically updated to reflect the committed changes.