On Tue, Jun 23, 2020 at 01:55:33PM -0400, Denton Liu wrote:
> void packet_flush(int fd)
> {
> - packet_trace("0000", 4, 1);
> - if (write_in_full(fd, "0000", 4) < 0)
> - die_errno(_("unable to write flush packet"));
> + control_packet_write(fd, "0000", _("unable to write flush packet"));
> }
I like this kind of abstraction much more than what was going on in the
previous patch.
> +#define control_packet_write(fd, s, errstr) \
> + do { \
> + (void)s"is a string constant"; \
This is a neat trick. It might be nice to wrap in
BUILD_ASSERT_IS_STRING_LITERAL() or similar. (Though I wonder a bit if
we even really need to assert that; wouldn't it be OK to use this
function without it? We're using strlen(), after all, and not sizeof).
Would it also be useful to assert that the length of the control packet
is 4? And likewise that it's less than 4? That seems much more
interesting to me (as we'd be violating the protocol otherwise). And
that would be easy to do if the we passed the packet number as an
integer and formatted it ourselves.
But the result gets kind of ugly:
void control_packet_write(int fd, unsigned packet_id, const char *errstr)
{
char buf[4 + 1];
assert(packet_id < 4); /* >= 4 are actual data packets */
vsnprintf(buf, sizeof(buf), "%04u", packet_id);
if (write_in_full(fd, buf, 4))
...;
}
There are only 3 of these, and their whole point is to hide not just
this magic "4", but the whole idea of "this is what a control packet
looks like". I kind of wonder if the abstractions we need to reduce the
3 lines to 1 is really making anything more readable.
-Peff
