Re: cygwin-latest: compile errors related to sockaddr_storage, dirent->d_type and dirent->d_ino

Christopher Faylor [off-list ref] writes:

"They" probably would like to hear about any irregularities that are found.
"They" probably don't like it when people treat an open source project as
if it was some unresponsive proprietary enterprise which does not listen
to or accept patches.

First of all, thanks for joining our discussion.  Being able to
hear from somebody from other project firsthand (not just listen
to somebody talking in his own changelogs and code comments, but
in actual e-mail exchange discussion) lets us put faces and
names to the entity "so far just one of the external projects to
us".

quoted

For reasons unknown, cygwin decided to use our sockaddr_storage.

I haven't looked at the proposed patch by Alex, so would not
comment on this part, but I'd appreciate your input.

quoted

For the other, probably unrelated, reasons, they decided to leave
declarations of DT_* macros in dirent.h without providing dirent->d_type.

I was wondering what the justification for keeping DT_* without
d_type myself.  What is the preferred resolution on this one
from your point of view?  I suspect removing d_type while
leaving DT_* was just a transient error and you would want to
remove DT_* as well, in which case the patch on this issue by
Alex would become unnecessary.

quoted

And on top of that, they removed dirent->d_ino (or probably replaced it
by __ino32, if at all).  BTW, can we somehow avoid using d_ino?  It is
referenced only in fsck-objects.c Anyway, to workaround this I put

COMPAT_CFLAGS += -Dd_ino=__ino32

It helps, but surely is not the solution.

I don't see how it could help since __ino32 is not actually filled in
with anything.  In fact, I'll rename the field to __invalid_ino32 to
make that clear.

I think renaming __invalid_* makes sense.  I'll see how we would
work this around on the git side to make things more portable.

On Thu, Jan 19, 2006 at 12:59:57AM -0800, Junio C Hamano wrote:

Christopher Faylor [off-list ref] writes:

quoted

"They" probably would like to hear about any irregularities that are
found.  "They" probably don't like it when people treat an open source
project as if it was some unresponsive proprietary enterprise which
does not listen to or accept patches.

First of all, thanks for joining our discussion.

You're welcome.  I use git on linux and cygwin so I'm happy to try to help.

Being able to hear from somebody from other project firsthand (not just
listen to somebody talking in his own changelogs and code comments, but
in actual e-mail exchange discussion) lets us put faces and names to
the entity "so far just one of the external projects to us".

I only read this list sporadically but I have chimed in from time to time
when people were talking about cygwin.  For the most part, it seems like
my input hasn't been needed all that much.

quoted

quoted

For reasons unknown, cygwin decided to use our sockaddr_storage.

I haven't looked at the proposed patch by Alex, so would not comment on
this part, but I'd appreciate your input.

quoted

quoted

For the other, probably unrelated, reasons, they decided to leave
declarations of DT_* macros in dirent.h without providing
dirent->d_type.

I was wondering what the justification for keeping DT_* without d_type
myself.  What is the preferred resolution on this one from your point
of view?  I suspect removing d_type while leaving DT_* was just a
transient error and you would want to remove DT_* as well, in which
case the patch on this issue by Alex would become unnecessary.

Actually, I started adding DT_* macros at one point, in preparation for
adding d_type, and then got sidetracked.  Their existence is a real bug
so, I have ifdef'ed out the DT macros in current CVS.

quoted

quoted

And on top of that, they removed dirent->d_ino (or probably replaced it
by __ino32, if at all).  BTW, can we somehow avoid using d_ino?  It is
referenced only in fsck-objects.c Anyway, to workaround this I put

COMPAT_CFLAGS += -Dd_ino=__ino32

It helps, but surely is not the solution.

I don't see how it could help since __ino32 is not actually filled in
with anything.  In fact, I'll rename the field to __invalid_ino32 to
make that clear.

I think renaming __invalid_* makes sense.  I'll see how we would
work this around on the git side to make things more portable.

There were two fields in the dirent struct.  One was the "real" d_ino
which was a 64-bit ino_t, __ino32 was a legacy field from a time when
inodes were 32 bits.  I had already renamed the d_ino to __invalid_d_ino
but I didn't think I had to rename __ino32, too, since it wasn't a
standard field and didn't think that anyone would be using it.  However,
it is now __invalid_ino32 (and will probably disappear entirely) in CVS.

I knew that there would be fallout from getting rid of d_ino but this
change has been a long time coming.  Previously, the inodes reported in
d_ino were different from the (correct) ones in st_ino and some
applications were understandbly confused by that fact.  Making d_ino
accurate would have meant that we'd have to open every file in readdir
to get the windows equivalent of inode information and, since we get
enough "cygwin is slow" complaints, that wasn't a cost we were willing
to pay.

cgf

On Thu, Jan 19, 2006 at 11:10:00AM -0500, Christopher Faylor wrote:

On Thu, Jan 19, 2006 at 12:59:57AM -0800, Junio C Hamano wrote:

quoted

Christopher Faylor [off-list ref] writes:

quoted

"They" probably would like to hear about any irregularities that are
found.  "They" probably don't like it when people treat an open source
project as if it was some unresponsive proprietary enterprise which
does not listen to or accept patches.

First of all, thanks for joining our discussion.

You're welcome.  I use git on linux and cygwin so I'm happy to try to help.

Btw, we're looking to roll out a new release of cygwin which fixes the
embarrassing typo in sockaddr_storage.  It is fixed in cygwin snapshots:

http://cygwin.com/snapshots/

cgf

On Thu, 19 Jan 2006, Christopher Faylor wrote:

Btw, we're looking to roll out a new release of cygwin which fixes the
embarrassing typo in sockaddr_storage.  It is fixed in cygwin snapshots:

http://cygwin.com/snapshots/

Quick question for cygwin people (I asked this at an earlier point, but I 
don't think there was any reply): would cygwin prefer using "vfork()" over 
"fork()", or is there no advantage? With vfork(), I could imagine that you 
might avoid a lot of strange VM games.. 

I think almost all of the git fork usage is of the type where "vfork()" 
would work fine (git-daemon in non-inetd form is an exception, perhaps the 
only one).

So if using vfork() is preferable, we probably should do that. It tends to 
be a small performance improvement on Linux too, although on Linux it's 
really pretty much in the noise.

Alternatively, is there anything else we can do that makes things easier?

			Linus

On Thu, Jan 19, 2006 at 01:16:17PM -0800, Linus Torvalds wrote:

On Thu, 19 Jan 2006, Christopher Faylor wrote:

quoted

Btw, we're looking to roll out a new release of cygwin which fixes the
embarrassing typo in sockaddr_storage.  It is fixed in cygwin
snapshots:

http://cygwin.com/snapshots/

Quick question for cygwin people (I asked this at an earlier point, but
I don't think there was any reply): would cygwin prefer using "vfork()"
over "fork()", or is there no advantage?  With vfork(), I could imagine
that you might avoid a lot of strange VM games..

Sorry, I missed the earlier question.  I have to get in the habit of
scanning this list more regularly for this type of thing.

Cygwin's vfork implementation currently defaults to fork so it doesn't
matter which is used.

We used to have a vfork which tried to cut down on some of the
substantial overhead that comes with cygwin's fork() but the vfork
implementation eventually grew so complicated that there was eventually
no performance gain and I decided to just yank it and revisit it at
a later point.

So, for now, there is no difference, but, eventually, there might be if
someone masters courage to revisit vfork-on-cygwin.

So, I guess that means that it would be a good idea to switch to vfork
if you were planning for the nebulous future when this made a difference
to cygwin.  Otherwise, I wouldn't bother.

Alternatively, is there anything else we can do that makes things easier?

I'm really committed to making cygwin as much like linux as possible so
that you won't have to make things easier.  The last release added some
stuff which should make building linux easier, in fact.  Also, the
mmap() implementation should be a littler closer to linux.  It is, of
course, a work in progress, though.

The only thing that would speed up process creation in cygwin now is
the use of the windows spawn* family of function calls.  Those could be
used instead of fork/exec but I have a personal aversion to using them
since they are so non-UNIX.  If performance is an issue, however, that
could be something to investigate.

cgf

On Thu, 19 Jan 2006, Christopher Faylor wrote:

The only thing that would speed up process creation in cygwin now is
the use of the windows spawn* family of function calls.  Those could be
used instead of fork/exec but I have a personal aversion to using them
since they are so non-UNIX.  If performance is an issue, however, that
could be something to investigate.

What about posix_spawn()? I don't know the windows spawn things, but I 
assume posix_spawn() is basically trying to be them.. Does it work with 
cygwin if we try to convert some of the more obvious users?

Some of the git users should be easy to convert to posix_spawn()..

		Linus

On Thu, Jan 19, 2006 at 01:44:58PM -0800, Linus Torvalds wrote:

On Thu, 19 Jan 2006, Christopher Faylor wrote:

quoted

The only thing that would speed up process creation in cygwin now is
the use of the windows spawn* family of function calls.  Those could be
used instead of fork/exec but I have a personal aversion to using them
since they are so non-UNIX.  If performance is an issue, however, that
could be something to investigate.

What about posix_spawn()? I don't know the windows spawn things, but I 
assume posix_spawn() is basically trying to be them.. Does it work with 
cygwin if we try to convert some of the more obvious users?

You know, I couldn't remember what the name of the spawn that was
introduced for posix.  I was going to mention it in my email and I
forgot.  Obviously it was because the name so obscure that no one could
possibly remember it.  :-)

We don't emulate posix_spawn currently but we could and should.  I'll
put it on the todo list.

cgf

The d_ino field is only used for performance reasons in
fsck-objects.  On a typical filesystem, i-number tends to have a
strong correlation with where the actual bits sit on the disk
platter, and we sort the entries to allow us scan things that
ought to be close together together.

If the platform lacks support for it, it is not a big deal.
Just do not use d_ino for sorting, and scan them unsorted.

---

    Junio C Hamano [off-list ref] writes:

    > ...  I'll see how we would
    > work this around on the git side to make things more portable.

 Makefile       |    6 ++++++
 fsck-objects.c |   13 +++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

4bec2123116a92fbeae12afc70317dfc85f6087b

diff --git a/Makefile b/Makefile
index e8f4b24..31898f7 100644
--- a/Makefile
+++ b/Makefile

@@ -19,6 +19,8 @@ all:
 # Define NO_EXPAT if you do not have expat installed.  git-http-push is
 # not built, and you cannot push using http:// and https:// transports.
 #
+# Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.
+#
 # Define NO_STRCASESTR if you don't have strcasestr.
 #
 # Define NO_SETENV if you don't have setenv in the C library.

@@ -231,6 +233,7 @@ ifeq ($(uname_S),SunOS)
 	ALL_CFLAGS += -D__EXTENSIONS__
 endif
 ifeq ($(uname_O),Cygwin)
+	NO_D_INO_IN_DIRENT = YesPlease
 	NO_STRCASESTR = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	# There are conflicting reports about this.

@@ -332,6 +335,9 @@ ifdef NEEDS_NSL
 	LIBS += -lnsl
 	SIMPLE_LIB += -lnsl
 endif
+ifdef NO_D_INO_IN_DIRENT
+	ALL_CFLAGS += -DNO_D_INO_IN_DIRENT
+endif
 ifdef NO_STRCASESTR
 	COMPAT_CFLAGS += -DNO_STRCASESTR
 	COMPAT_OBJS += compat/strcasestr.o

diff --git a/fsck-objects.c b/fsck-objects.c
index 90e638e..9950be2 100644
--- a/fsck-objects.c
+++ b/fsck-objects.c

@@ -20,6 +20,13 @@ static int check_strict = 0;
 static int keep_cache_objects = 0; 
 static unsigned char head_sha1[20];
 
+#if NO_D_INO_IN_DIRENT
+#define SORT_DIRENT 0
+#define DIRENT_SORT_HINT(de) 0
+#else
+#define SORT_DIRENT 1
+#define DIRENT_SORT_HINT(de) ((de)->d_ino)
+#endif
 
 static void objreport(struct object *obj, const char *severity,
                       const char *err, va_list params)

@@ -307,7 +314,9 @@ static void fsck_sha1_list(void)
 {
 	int i, nr = sha1_list.nr;
 
-	qsort(sha1_list.entry, nr, sizeof(struct sha1_entry *), ino_compare);
+	if (SORT_DIRENT)
+		qsort(sha1_list.entry, nr,
+		      sizeof(struct sha1_entry *), ino_compare);
 	for (i = 0; i < nr; i++) {
 		struct sha1_entry *entry = sha1_list.entry[i];
 		unsigned char *sha1 = entry->sha1;

@@ -361,7 +370,7 @@ static int fsck_dir(int i, char *path)
 			memcpy(name+2, de->d_name, len+1);
 			if (get_sha1_hex(name, sha1) < 0)
 				break;
-			add_sha1_list(sha1, de->d_ino);
+			add_sha1_list(sha1, DIRENT_SORT_HINT(de));
 			continue;
 		}
 		fprintf(stderr, "bad sha1 file: %s/%s\n", path, de->d_name);

-- 
1.1.3-gacdd

On Thu, Jan 19, 2006 at 05:13:51PM -0800, Junio C Hamano wrote:

The d_ino field is only used for performance reasons in
fsck-objects.  On a typical filesystem, i-number tends to have a
strong correlation with where the actual bits sit on the disk
platter, and we sort the entries to allow us scan things that
ought to be close together together.

If the platform lacks support for it, it is not a big deal.
Just do not use d_ino for sorting, and scan them unsorted.

Ah, interesting.  I don't know if the file id objects would have
that sort of correlation on NT but obviously the file hashes don't.

cgf