Re: [PATCH v8 06/11] pkt-line: add packet_write_gently()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 26, 2016 at 09:21:10PM +0200, Lars Schneider wrote:

> On 25 Sep 2016, at 13:26, Jakub Narębski <jnareb@xxxxxxxxx> wrote:
> 
> > W dniu 20.09.2016 o 21:02, larsxschneider@xxxxxxxxx pisze:
> >> From: Lars Schneider <larsxschneider@xxxxxxxxx>
> >> ...
> >> 
> >> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> > 
> > I'm not sure what naming convention the rest of Git uses, but isn't
> > it more like '*data' rather than '*buf' here?
> 
> pkt-line seems to use 'buf' or 'buffer' for everything else.

I do not think we have definite rules, but I would generally expect to
see "data" as an opaque thing (e.g., passing "void *data" to callbacks).
"buf" or "buffer" makes sense here, but I don't think it really matters
that much either way.

> >> +	static char packet_write_buffer[LARGE_PACKET_MAX];
> > 
> > I think there should be warning (as a comment before function
> > declaration, or before function definition), that packet_write_gently()
> > is not thread-safe (nor reentrant, but the latter does not matter here,
> > I think).
> > 
> > Thread-safe vs reentrant: http://stackoverflow.com/a/33445858/46058
> > 
> > This is not something terribly important; I guess git code has tons
> > of functions not marked as thread-unsafe...
> 
> I agree that the function is not thread-safe. However, I can't find 
> an example in the Git source that marks a function as not thread-safe.
> Unless is it explicitly stated in the coding guidelines I would prefer
> not to start way to mark functions.

I'd agree. A large number of functions in git are not reentrant, and I
would not want to give the impression that those missing a warning are
safe to use.

> >> +	if (size > sizeof(packet_write_buffer) - 4) {
> > 
> > First, wouldn't the following be more readable:
> > 
> >  +	if (size + 4 > LARGE_PACKET_MAX) {
> 
> Peff suggested that here:
> http://public-inbox.org/git/20160810132814.gqnipsdwyzjmuqjy@xxxxxxxxxxxxxxxxxxxxx/

There is a good reason to do size checks as a subtraction from a known
quantity: you can be sure that you are not introducing an overflow
(e.g., Jakub's suggestion does the wrong thing when "size" is within 4
bytes of its maximum value). That's unlikely in this case, but then so
is the size exceeding LARGE_PACKET_MAX in the first place (arguably this
should be a die("BUG"), because it is the caller's responsibility to
split their packets.

-Peff



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]