Page MenuHomeFreeBSD

ggatec: dynamic buffers
Needs ReviewPublic

Authored by jo_bruelltuete.com on Aug 30 2021, 1:22 AM.

Details

Reviewers
asomers
peterj
Summary

Dynamically size buffers in ggatec.
Instead of static size on the stack.

Test Plan

Tested it on my machine. I'm still on 12-stable.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sbin/ggate/ggatec/ggatec.c
155

don't need this anymore now that recv thread will resize its buffer.

sbin/ggate/ggatec/ggatec.c
117

there's diff context missing here.
but there's a switch statement that has ENOMEM ifdef'd off. but without handling ENOMEM ggatec is almost totally useless tbh. i'm constantly running into that condition just trying some simple stuff...
i've got an updated patch for it.

Handle oversized write buffer coming from zfs

In general, this is a definite improvement over fixed size buffers (that are actually liable to subtle overruns).

sbin/ggate/ggatec/ggatec.c
97

Why ssize_t? The value can never be negative and it's only used to assign to an off_t. I would suggest this be size_t.

131

I think this only warrants LOG_DEBUG, not LOG_NOTICE. Particularly with ZFS, I/Os can be arbitrary sized so it's quite normal for them to exceed MAXPHYS. That's likely to make this noisy, unless there's a particularly write early on.

132

style(9) requires that all variable declarations come at the top of the function.

208

As per the comment above, I don't think ssize_t is the most appropriate type here.

245

Variable declarations should be at the top of the function.

sbin/ggate/ggatec/ggatec.c
97

copy pasta, size_t is good.

131

Yep noticed quite a bit of log spam myself.

132

If we ever update style I think that'd be a good rule to get rid of... i'm a big fan of declare & define & init on first use, esp since it has safety implications.
btw is the compiler supposed to warn/complain about this? (it didn't)

sbin/ggate/ggatec/ggatec.c
208

ah, ggio.gctl_length is off_t and at line 244 we have a comparison to buf_capacity.
with ssize_t we can avoid casting and its visual clutter.

jo_bruelltuete.com added inline comments.
sbin/ggate/ggated/ggated.c
753

this seems prudent.
otherwise unsupported requests (like maybe flush or trim from a newer version of ggatec) would get EIO, but in a not so obvious way.