Patch 5 seems spurious to me, regardless of what Apple's lawyers want.

There are more substantial patches coming.

On Tue, 2008-12-16 at 05:01 +0200, Timo Sirainen wrote:

...

Patch #3. Some versions of Mac OS X return near-duplicate kevents.

This probably won't affect other BSDs, so I think the duplicate removal should be around #ifdef __APPLE__?

Hmm. Actually I think when I was writing that code I noticed the same thing and tried to fix it with:

	/* there can be multiple events for a single io.
	   call the callback only once if that happens. */
	if (io->refcount == 2 && io->io.callback != NULL)
		io->io.callback(io->io.context);

But later (although inside the same commit..) I probably added the assert to:

for (i = 0; i < ret; i++) {
	io = (void *)events[i].udata;
	i_assert(io->refcount == 1);
	io->refcount++;
}

Which makes the refcount == 2 check pointless. I think it would work correctly simply if the assert was removed?

On Dec 16, 2008, at 6:47 PM, Mike Abbott wrote:

...

Hmm. Actually I think when I was writing that code I noticed the same thing and tried to fix it with:

  /* there can be multiple events for a single io.
     call the callback only once if that happens. */
  if (io->refcount == 2 && io->io.callback != NULL)
  	io->io.callback(io->io.context);

When there are near-duplicates in the returned events -- that is,
two or more entries in the events array having the same udata but
different fflags -- the refcount==2 check actually prevents the
callback from being called at all (assuming Apple's patch to this
area has not been applied, and the assert is removed). The first
loop increments the refcount twice (or more), from 1 to 3 (or
higher). Then the second loop skips the callback entirely because
the refcount does not equal 2. The callback is not even called once.

But it decreases the refcount every time. So if refcount starts at 3,
the second loop skips the first callback -> decreases refcount to 2 ->
calls callback -> decreases refcount to 1.

...

Patch #1. Some versions of Mac OS X have buggy CMSG_* macros.

Is the nopen() check really necessary?

It might detect buggy CMSG macros on non-Apple systems.

I'd like to know at least what kind of a bug it works around for
before applying it.

The 64-bit CMSG macros are broken in such a way that two descriptors
are sometimes passed (depending on the value in uninitialized memory,
often 0), not one. The nopen() check looks for that.

I will try to be more descriptive up front with the remaining
patches. This is why I started with some small ones. Thank you for
your cooperation.

...

Patch #2. Don't set "<username>=1" in the environment.

I put the fix to auth-client.c: http://hg.dovecot.org/dovecot-1.1/rev/3145be9f66ae

Two points:

1. Yes, if you see a better way to achieve the same goal for any of
   Apple's patches, please do adopt the better way.
2. However, in this case your change will prevent one of our future
   patches from working properly. It will need the user name in the
   extra_fields array in deliver.c. May I suggest that you take patch #2
   as-is?

...

Patch #3. Some versions of Mac OS X return near-duplicate kevents.

This probably won't affect other BSDs, so I think the duplicate
removal should be around #ifdef __APPLE__?

I have no data to support that conclusion.

...

Patch #4. Null renames are actually pretty slow on HFS (Apple's file system), so avoid them.

I've thought about adding a similar check before, but it removes an intentional side effect: It makes sure that the file exists. If it doesn't exist (flags had changed), its new name is looked up and the rename is tried again. Would it be faster to instead stat() to see if the file exists?

I wondered about that. Sure, a stat() would be faster.

...

Patch #5. Required by Apple's lawyers.

That's fine by me.

Phew!

On Thu, 2009-01-08 at 13:15 -0600, Mike Abbott wrote:

For all the changes you checked into 1.1 on our behalf, will they also
be included in 1.2?

Yes, everything that's in v1.1 is (or should be) always in v1.2.