validate max_allowed_packet on the client side #84
Conversation
| { | ||
| if (builder->packet_length > max_length) { | ||
| return TRILOGY_MAX_PACKET_EXCEEDED; | ||
| } |
inc/trilogy/builder.h
Outdated
| * max_length - The new maximum packet length to set | ||
| * | ||
| * Return values: | ||
| * TRILOGY_OK - The maximum packet length was set. |
There was a problem hiding this comment.
Very minor nit that you don't really have to do, but would you mind indenting from - to line up with the one below, like the rest of the comments? I never used to do stuff like this, but gofmt changed my life for better or worse 😆
I guess the rest of the comments above where TRILOGY_MAX_PACKET_EXCEEDED was added could use it as well so everything lines up nicely. But again, no big deal if that's annoying ;)
There was a problem hiding this comment.
Oh, yeah, this is a bit of an ugly floater. I deliberately didn't want to give up that much horizontal space by aligning everything to the new very-long name, but now this line is only aligned with the separate preceding blocks. 😕
I see I was also needlessly careless on the wrapped indentation below -- I copied across the misalignment from buffer.h, without noticing this is the first instance in this file.
I think we just need to do a bit of a general sweep... I think the alignment was a lot more consistent before one of the renames.
There was a problem hiding this comment.
I fixed this one, and tidied the wrapping on the others, but couldn't bring myself to drag the other descriptions over that far. 🤷🏻♂️
Makefile
Outdated
| ifeq ($(UNAME_S), Darwin) | ||
| CFLAGS += "-I$(shell brew --prefix openssl@1.1)/include" | ||
| LDFLAGS += "-L$(shell brew --prefix openssl@1.1)/lib" | ||
| else |
There was a problem hiding this comment.
In a perfect world this could be in its own pull, but I was getting sick of adding these myself locally when compiling so I'm personally all good with it going in with this.
There was a problem hiding this comment.
Yeah this had started life on its own branch; rebased out now and I'll push it separately.
|
@brianmario thanks for checking this out! While I've got you -- what do you think of Per the discussion in #79 (and my repeated confusion in figuring this out 😅), while those chunks certainly meet a reasonable definition of a packet, I feel like we might be better off using a different term internally. From a quick search, I think even other instances within the codebase are already using "packet" to refer to the potentially-multi-part total message. |
|
@matthewd I'd definitely be in favor of renaming that constant. At the time, the difference between a "packet" and a "frame" (or whatever other term might be used for the segments of data) didn't matter as much. But I think it makes sense to rename either as part of this pull or a follow up before a new release is tagged 👍 |
Sending an over-limit packet will cause the server to drop the connection; by enforcing the limit on the client, we can fail in a way that keeps the connection usable. Co-authored-by: Kamil Gwóźdź <kamil-gwozdz@github.com>
We don't really care what's in there, but this is more hygienic, and will likely keep Valgrind happier.
Co-authored-by: Kamil Gwóźdź <kamil-gwozdz@github.com>
This is another take on #79, incorporating lessons learned while working on #81.