bug-gnulib
[Top][All Lists]
Advanced

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

RE: bug#10305: coreutils-8.14, "rm -r" fails with EBADF


From: V S, Nagendra (Nonstop Filesystems Team)
Subject: RE: bug#10305: coreutils-8.14, "rm -r" fails with EBADF
Date: Fri, 20 Jul 2012 06:55:57 +0000

Was doing more code study on the getcwd problem,

The coreutils config.hin no more defines any of RAW_DECL_* functions, which 
where present in 8.15. Can we except more problems because of this?

Thanks & Regards
Nagendra.V.S

-----Original Message-----
From: Joachim Schmitz [mailto:address@hidden 
Sent: Thursday, July 19, 2012 11:24 PM
To: 'Paul Eggert'
Cc: address@hidden; address@hidden; 'Eric Blake'; 'Jim Meyering'; Schmitz, 
Joachim; V S, Nagendra (Nonstop Filesystems Team)
Subject: RE: bug#10305: coreutils-8.14, "rm -r" fails with EBADF

> From: Joachim Schmitz [mailto:address@hidden
> Sent: Wednesday, June 27, 2012 9:25 AM
> To: 'Paul Eggert'
> Cc: 'address@hidden'; 'address@hidden'; 'Eric Blake'; 'Jim 
> Meyering'
> Subject: RE: bug#10305: coreutils-8.14, "rm -r" fails with EBADF
> 
> > From: Paul Eggert [mailto:address@hidden
> > Sent: Wednesday, June 27, 2012 2:49 AM
> > To: Joachim Schmitz
> > Cc: address@hidden; address@hidden; 'Eric Blake'; 'Jim
> Meyering'
> > Subject: Re: bug#10305: coreutils-8.14, "rm -r" fails with EBADF
> >
> > On 06/26/2012 09:38 AM, Joachim Schmitz wrote:
> >
> > > Let me know what you think and where/how you'd do it differently.
> >
> > The changes mostly look good.  The trivial ones we've incorporated 
> > already.  I have some comments on the nontrivial ones (please see below).

> > Here are some comments about that patch:

> > > --- ./gnu/dirfd.c.orig    2011-03-12 03:14:28.000000000 -0600
> > > +++ ./gnu/dirfd.c 2012-06-25 02:55:09.000000000 -0500
> > > ...
> > > +#ifdef __TANDEM
> > > +# include <unistd.h> /* for _gl_fnum2dt(), needed in C99 mode */ 
> > > +#endif
> >
> > Shouldn't that be "_gl_fnum2fd"?
> 
> Yes, of course, stupid typo.
> 
> > More important, doesn't the declaration of _gl_fnum2fd belong better 
> > in dirent.h, not unistd.h?  Among other things, that would mean the 
> > above change can be omitted.
>
> My attempts to integrate this into coreutils-8.17 seem to indicate 
> that this function and a couple more are used elsewhere too Looks like 
> there closedir.o needs _gl_ungerister_fnum(), dirfd.c needs 
> _gl_fnum2ds() and opendir.c needs _gl_register_fnum().

Furthermore that function needs access to fnum2fdmap, which is local to fchdir.c
 
> > >  int
> > >  dirfd (DIR *dir_p)
> > >  {
> > >    int fd = DIR_TO_FD (dir_p);
> > >    if (fd == -1)
> > >      errno = ENOTSUP;
> > > +#ifdef __TANDEM
> > > +  fd = _gl_fnum2fd(fd);
> > > +#endif
> >
> > This might be cleaner if DIR_TO_FD invoked _gl_fnum2fd directly.
> > That way, dirfd.c could be left alone.  (Or perhaps not; I don't 
> > understand the code that well.)
> 
> Won't that make that macro too complicated?

I believe it would. Prove me wrong ;-)

> > > +  char fnum;        /* 'y' or 'n', actually a bool */
> >
> > Why not use 1 and 0?  That's far more typical for boolean values, 
> > and generates better code.

Done, revealed a bad bug in our code, fixed now too.

> Maybe we could also make it a short and place fnum right there? That 
> would change the implementation quite a bit though, but might make it leaner 
> too.
> And the comment
> /* FIXME - add a DIR* member to make dirfd possible on mingw?  */ 
> Indicates that there is a change pending for a similar purpose on a different 
> platform...

Ok now...

After quite a few iterations and with integrating the things you already fixed 
for the next releases of gnulib, coreuitls and tar, we finally got it running 
fine for coreutils-8.15. Doing the equivalent changes (not 100% identical, the 
coreutils resp. gnulib code had changed and reshuffled quite a bit) lead us 
into an endless recursion (well as endless as the stack could grow without 
aborting). After quite some effort in debugging, Nagendra finally found the 
culprit: in 8.15 configure checks for several functions to be declared without 
a macro, one of them is getcwd(), and then puts a #define HAVE_RAW_DECL_GETCWD 
into config.h. Coreutils-8.17 doesn't have that check anymore, hence doesn't 
set that define and then in getcwd.c enters that endless recursion loop: 
getcwd(), openat(), rpl_open(), get_name(), getcwd() ....

Our (temp.?) fix is to add an "|| __TANDEM" to lib/getcwd.c, line 138.

Find the entire patch we use attached. Some more comments on these:

We had to hop thru several loops to get getmntent() implemented for NonStop, 
you may or may not be interested in the details. It is very system specific. 
Part of the stuff is outside this patch in our own GNUlib-like library (called 
floss, Freeware Look for Open System Services). Maybe eventually this gets 
added to our system libs, who knows...

The patch to lib/i-ring.h is needed for the fts-functions to recur deeper than 
4 levels, at least on NonStop. It is not really fully understood why that is 
(not by me at least), but that change cured the symptom ;-). Probably doesn't 
need to be quit as big...

The patch to lib/readutmp.h is ugly but needed, at least for now, ignore it, 
unless you have a better idea. We don't have utmp and friends at all. This is a 
bit work in progress, probably falls into the same category as getmntent().

The patches reg. NO_UID and NO_GID might be better done someplace else, or not 
at all, I'm sure whether they are worth the effort.

I've disable a bit of apparently dead code in src/remove.c

I'm not sure what's wrong with  src/stat.c, but our compiler throws up on the 
original code. Might well be a bug in our compiler?
Same for src/system.h, I can't get it thru the compiler without that hack, but 
haven't been able to find out why.

The patch in src/su.c is not really needed, we can't use this su anyway, as our 
authentication works differently. But the DEFAULT_USER being "root" might be 
worth getting added to root-uid.h, possibly?

In src/uptime.c it gets pretty system specific (but not as deeply as 
getmntent), don't know whether you're interested in this, but I included it 
here anyway.

I've tried to fix some tests, split/filter and touch/not-owner. Some more tests 
are failing for no good reason, but a) I haven't yet found why they fail nor 
how to fix it and b) never check for error conditions you can't handle, so I 
just don't care about these test too much ;-)

As mentioned further above some patches are taken what you already did to 
coreutils, gnulib and tar.

I've removed a few hunks out of the patch, this may result in warnings by patch 
about an offset, but I guess you wouldn't apply them unaltered anyway.

Wherever you see room for improvement or have more questions, we're open, just 
let Nagendra and me know (and yes, address@hidden and address@hidden is the 
same person, me, just in different roles, business and private, but feel free 
to pick your choice)

Bye, Jojo

reply via email to

[Prev in Thread] Current Thread [Next in Thread]