Re: write() _will_ fail on Mac OS X/XNU if nbytes > INT_MAX

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2013, 02:28 UTC
Message-ID: <7vbo8j600q.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAEDE8504Pa_hRcHBLt4S9CL74noqrX3fWGCSf+x45zCr_43+5A@mail.gmail.com>

Filipe Cabecinhas <filcab@gmail.com> writes:

> Sorry for the delay. I've updated the patch to work as you suggested (I think).
> It's attached.

A few comments.

First, the formalities. Please see Documentation/SubmittingPatches, notably:

   - Please do not send a patch as an attachment.

   - The attached patch does not seem to have any proposed commit
     log message. For this particular change, I expect it would not
     be more than a paragraph of several lines. Please have one.

   - Please sign-off your patch.

Look at patches from other high-value contributors to learn the style and the tone of log message. For instance,

    http://article.gmane.org/gmane.comp.version-control.git/223406

is a good example (taken at random).

Show 11 quoted lines

> diff --git a/compat/write.c b/compat/write.c
> new file mode 100644
> index 0000000..1e890aa
> --- /dev/null
> +++ b/compat/write.c
> @@ -0,0 +1,11 @@
> +#include <limits.h>
> +#include <unistd.h>
> +
> +/* Version of write that will write at most INT_MAX bytes.
> + * Workaround a xnu bug on Mac OS X */

	/*
         * We format our multi-line comments like this.
         *
         * Nothing other than slash-asterisk/asterisk-slash
         * on the first and the last line of the comment block.
         */

Show 6 quoted lines

> +ssize_t clipped_write(int fildes, const void *buf, size_t nbyte) {
> +  if (nbyte > INT_MAX)
> +    return write(fildes, buf, INT_MAX);
> +  else
> +    return write(fildes, buf, nbyte);
> +}

Style:

 - opening and closing parentheses of the function body sit on
   lines on their own.

 - one level of indent is 8 places, using a tab.

Perhaps it would look more like this (my MUA may clobber the tabs, though, before it gets to you):

	ssize_t clipped_write(int fildes, const void *buf, size_t nbyte)
	{
		if (nbyte > INT_MAX)
			nbyte = INT_MAX;
		return write(fildes, buf, nbyte);
	}

I do not want to see this ffile called "write.c"; we will encounter a platform whose write(2) behaves a way that we do not expect but is different from this "clipped to INT_MAX" in the future. The way we will deal with such a platform is to add a workaround similar to this patch somewhere in the compat/ directory, but if you squat on "compat/write.c", it would be a problem for that platform, as it cannot call its version "compat/write.c".

Perhaps call it "compat/clipped-write.c" or something.

By the way, does your underlying write(2) correctly write INT_MAX bytes, or did you mean to clip at (INT_MAX-1)? Just double-checking.

Other than that, the patch looks sensible to me.

Thanks.