Re: [PATCH] ls -i: print consistent inode numbers also for mount points
Jim Meyering wrote: > Following up on a long thread from a year ago, here's a patch > to fix the 3.5-year-old readdir-vs-mountpoint-inode bug in ls -i. ... > http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/14020 ... > It's a shame to have to pessimize ls -i performance, even by this > small amount on linux-based kernels, but correctness definitely > trumps performance, here. Pushed.
Re: cp(1) extended attributes bug
Hi, thanks for report... Ernest N. Mamikonyan wrote: > Cp(1) doesn't correctly copy extended attributes for read-only files: > > touch foo > setfattr -n user.key -v value foo > chmod a-w foo > cp --preserve=xattr foo bar > cp: setting attribute `user.key' for `user.key': Permission denied This error message is not produced by coreutils sources, but by libattr - all messages about extended attributes are generated there. I'm not sure why they are trying to set attributes for source file - maybe they are not and access rights for destination file are more relevant. Strace/ltrace of the failure could be helpful as well. > If one uses "cp -a" instead, it simply strips the metadata and doesn't > complain. cp -a shouldn't complain, but the xattrs are likely not preserved in this case - just error message from xattr preservation is suppressed and not preserving extended attributes is not considered as error in that case. Greetings, Ondřej Vašík signature.asc Description: Toto je digitálně podepsaná část zprávy
Re: timeout.c warning about returning undefined value
Pádraig Brady wrote: > I'll check in the attached later on today > unless there are objections. ... > Subject: [PATCH] timeout: defensive handling of all wait() errors > > * src/timeout.c (main): Handle all possible cases of unexpected > failures from wait(). This was prompted by the clang tool reporting > the possible non-initialization of the status variable. Looks perfect. Thanks!
Re: timeout.c warning about returning undefined value
I'll check in the attached later on today
unless there are objections.
cheers,
Pádraig.
>From 9fd105fed6c317d12c949a1f8b66b0c0aacc77b3 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?=
Date: Mon, 31 Aug 2009 19:18:27 +0100
Subject: [PATCH] timeout: defensive handling of all wait() errors
* src/timeout.c (main): Handle all possible cases of unexpected
failures from wait(). This was prompted by the clang tool reporting
the possible non-initialization of the status variable.
---
src/timeout.c | 25 +++--
1 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/src/timeout.c b/src/timeout.c
index 20efddd..62f3d4b 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -317,12 +317,25 @@ main (int argc, char **argv)
child exits, not on this process receiving a signal. Also we're not
passing WUNTRACED | WCONTINUED to a waitpid() call and so will not get
indication that the child has stopped or continued. */
- wait (&status);
-
- if (WIFEXITED (status))
-status = WEXITSTATUS (status);
- else if (WIFSIGNALED (status))
-status = WTERMSIG (status) + 128; /* what sh does at least. */
+ if (wait (&status) == -1)
+{
+ /* shouldn't happen. */
+ error (0, errno, _("error waiting for command"));
+ status = EXIT_CANCELED;
+}
+ else
+{
+ if (WIFEXITED (status))
+status = WEXITSTATUS (status);
+ else if (WIFSIGNALED (status))
+status = WTERMSIG (status) + 128; /* what sh does at least. */
+ else
+{
+ /* shouldn't happen. */
+ error (0, 0, _("unknown status from command (0x%X)"), status);
+ status = EXIT_FAILURE;
+}
+}
if (timed_out)
return EXIT_TIMEDOUT;
--
1.6.2.5
cp(1) extended attributes bug
Cp(1) doesn't correctly copy extended attributes for read-only files: touch foo setfattr -n user.key -v value foo chmod a-w foo cp --preserve=xattr foo bar cp: setting attribute `user.key' for `user.key': Permission denied If one uses "cp -a" instead, it simply strips the metadata and doesn't complain.
Re: timeout.c warning about returning undefined value
Jim Meyering wrote:
> Hi Pádraig,
>
> clang reports this:
>
> timeout.c:330:9: warning: Uninitialized or undefined value returned to
> caller.
> return status;
> ^
>
> Even if it really can't happen (which the comment suggests), please
> initialize "status" or ensure that we get a diagnostic and exit nonzero
> if/when wait ever fails.
I'll check something like this in later when I've a chance to test.
diff --git a/src/timeout.c b/src/timeout.c
index 20efddd..c47069c 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -317,12 +317,18 @@ main (int argc, char **argv)
child exits, not on this process receiving a signal. Also we're not
passing WUNTRACED | WCONTINUED to a waitpid() call and so will not get
indication that the child has stopped or continued. */
- wait (&status);
-
- if (WIFEXITED (status))
-status = WEXITSTATUS (status);
- else if (WIFSIGNALED (status))
-status = WTERMSIG (status) + 128; /* what sh does at least. */
+ if (wait (&status) == -1)
+{
+ error (0, errno, _("error waiting for command"));
+ status = EXIT_CANCELED;
+}
+ else
+{
+ if (WIFEXITED (status))
+status = WEXITSTATUS (status);
+ else if (WIFSIGNALED (status))
+status = WTERMSIG (status) + 128; /* what sh does at least. */
+}
if (timed_out)
return EXIT_TIMEDOUT;
timeout.c warning about returning undefined value
Hi Pádraig, clang reports this: timeout.c:330:9: warning: Uninitialized or undefined value returned to caller. return status; ^ Even if it really can't happen (which the comment suggests), please initialize "status" or ensure that we get a diagnostic and exit nonzero if/when wait ever fails.
[PATCH] maint: mbsalign.c: remove unnecessary assignment
FYI, clang found an unnecessary assignment. This removes it, >From c6cc0321f3a07d4828e1043c21a468d45941850a Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Mon, 31 Aug 2009 16:37:36 +0200 Subject: [PATCH] maint: mbsalign.c: remove unnecessary assignment * gl/lib/mbsalign.c (mbsalign): Remove assignment, the result of which is never used. --- gl/lib/mbsalign.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/gl/lib/mbsalign.c b/gl/lib/mbsalign.c index 0dfda75..a075747 100644 --- a/gl/lib/mbsalign.c +++ b/gl/lib/mbsalign.c @@ -219,7 +219,7 @@ mbsalign (const char *src, char *dest, size_t dest_size, dest = mbs_align_pad (dest, dest_end, start_spaces); dest = mempcpy(dest, str_to_print, MIN (n_used_bytes, dest_end - dest)); - dest = mbs_align_pad (dest, dest_end, end_spaces); + mbs_align_pad (dest, dest_end, end_spaces); } mbsalign_cleanup: -- 1.6.4.2.363.g2d6e
Re: [PATCH] ls -i: print consistent inode numbers also for mount points
Pádraig Brady wrote: ... >> Yep, it has some performance impact... >> checked `time ./ls -i1R /home /dev /usr /var /lib >myinodes with >> approximate number of 31k dirs, 450k files on ext3 >> >> and results are >> old binary without the patch: >> real2m5.631s >> user0m3.012s >> sys 0m4.815s >> >> new binary: >> real6m8.560s >> user0m3.947s >> sys 0m30.572s > > So there was a lot more disk access with the new binary. As expected. The new version has to stat every single file. The optimized one could deal with a 1-million-entry directory with a single stat call (solely for the directory). > but the NEWS suggests that should only be the case for > "systems with dysfunctional readdir". Currently, as far as I know, all systems except Cygwin have a dysfunctional readdir. On a modern system with lots of RAM and a hot cache, listing 238,000 inodes takes about twice as long: $ env time ./ls -i1R /home /dev /usr /var /lib > /t/inodes 0.34user 0.95system 0:01.29elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+761minor)pagefaults 0swaps 0.34user 0.94system 0:01.29elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+761minor)pagefaults 0swaps $ env time /bin/ls -i1R /home /dev /usr /var /lib > /t/inodes 0.31user 0.29system 0:00.65elapsed 93%CPU (0avgtext+0avgdata 0maxresident)k 80inputs+0outputs (2major+750minor)pagefaults 0swaps 0.31user 0.30system 0:00.61elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+752minor)pagefaults 0swaps 0.29user 0.30system 0:00.60elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+751minor)pagefaults 0swaps But as you can read in that year old thread, we found no widely-used application that relies on the performance of ls -i.
Re: [PATCH] ls -i: print consistent inode numbers also for mount points
Ondřej Vašík wrote: > Pádraig Brady wrote: >> So there was a lot more disk access with the new binary. >> but the NEWS suggests that should only be the case for >> "systems with dysfunctional readdir". >> >> What was your system? > > Quite ancient system... I checked this on my old Fedora Core 6... > Anyway could check with something newer if required... :) No need. I'll test it on F8 and F11 this evening. thanks, Pádraig.
Re: [PATCH] ls -i: print consistent inode numbers also for mount points
Pádraig Brady wrote: > So there was a lot more disk access with the new binary. > but the NEWS suggests that should only be the case for > "systems with dysfunctional readdir". > > What was your system? Quite ancient system... I checked this on my old Fedora Core 6... Anyway could check with something newer if required... :) Greetings, Ondřej signature.asc Description: Toto je digitálně podepsaná část zprávy
Re: Bug#505927: just use the date(1) -d library instead of your own poorer date parser
> "BSB" == Bernd Siggy Brentrup writes: BSB> Hi jidanni, BSB> I'm extremely angry about you forwarding my findings to the BSB> Debian BTS. Sorry. I just try to increase people's networking, and assumed the bug number, which I saw in the Subject, had fallen off the CC list and needed to be put back. So maybe I'm not the safest person to send things too. BSB> ps: Do you grant me permission to cite you to at...@free-it.org, BSB> which obviously is an address for at-ng related stuff? Yes, everybody can cite anything I say, it's all OK.
Re: [PATCH] ls -i: print consistent inode numbers also for mount points
Ondřej Vašík wrote: > Jim Meyering wrote: >> Following up on a long thread from a year ago, here's a patch >> to fix the 3.5-year-old readdir-vs-mountpoint-inode bug in ls -i. > > Checked on my system and works fine... > >> It's a shame to have to pessimize ls -i performance, even by this >> small amount on linux-based kernels, but correctness definitely >> trumps performance, here. > > Yep, it has some performance impact... > checked `time ./ls -i1R /home /dev /usr /var /lib >myinodes with > approximate number of 31k dirs, 450k files on ext3 > > and results are > old binary without the patch: > real2m5.631s > user0m3.012s > sys 0m4.815s > > new binary: > real6m8.560s > user0m3.947s > sys 0m30.572s So there was a lot more disk access with the new binary. but the NEWS suggests that should only be the case for "systems with dysfunctional readdir". What was your system? thanks, Pádraig.
Re: [PATCH] ls -i: print consistent inode numbers also for mount points
Ondřej Vašík wrote: > Jim Meyering wrote: >> Following up on a long thread from a year ago, here's a patch >> to fix the 3.5-year-old readdir-vs-mountpoint-inode bug in ls -i. > > Checked on my system and works fine... > >> It's a shame to have to pessimize ls -i performance, even by this >> small amount on linux-based kernels, but correctness definitely >> trumps performance, here. > > Yep, it has some performance impact... > checked `time ./ls -i1R /home /dev /usr /var /lib >myinodes with > approximate number of 31k dirs, 450k files on ext3 > > and results are > old binary without the patch: > real2m5.631s > user0m3.012s > sys 0m4.815s > > new binary: > real6m8.560s > user0m3.947s > sys 0m30.572s > > But correctness should be preferred as this is not so common usecase... > >> Review appreciated. > > Looks ok from my point of view. Thanks for the review and the timing info.
Re: Bug#505927: just use the date(1) -d library instead of your own poorer date parser
Hi jidanni, I'm extremely angry about you forwarding my findings to the Debian BTS. Debian decided not to give me a vote again without passing a full NM procedure which I don't accept, hence: *I don't fix their bugs without having a vote*. @ Debian's at maintainer, I hereby declare: *You are not allowed to use my findings for Debian.* No objection against the Cc to gnu though. On Mon, Aug 31, 2009 at 17:23 +0800, jida...@jidanni.org wrote: > >> >> However to get at to accept such a date, one needs: > >> >> $ at -v $(date --rfc-3339=date -d 'now + 5 years + 11 months') > >> >> Fri Oct 17 03:56:00 2014 > >> >> warning: commands will be executed using /bin/sh > > B'B> How do you like these: > B'B> at-ng/build% ./at now + 5 years + 11 months > B'B> Job will run at or after Wed, 29 Jul 2015 20:26:00 +0200. > > B'B> It was a trivial fix adding just 2 lines to the grammar. > > I'll cc them to let them know... @jidanni Please don't cite me to anybody without my permission. I addressed jida...@jidanni.org, not 505927-submit...@bugs.d.o or even 505...@bugs.d.o, which you may be assured I am capable of. My goal is not to make Debian's at package better, but to offer an enhanced replacement via https://launchpad.net/~at-ng. Thanks Siggy ps: Do you grant me permission to cite you to at...@free-it.org, which obviously is an address for at-ng related stuff? -- O< ascii ribbon campaign - stop html mail - www.asciiribbon.org+ | 仅46天时间 |Open Source in Northern Germany: www.free-it.org| |www.Ubucon.de|tech contact: bsb-at-free-dash-it-dot-de| +---> ceterum censeo javascriptum esse restrictam <+ signature.asc Description: Digital signature
Re: [PATCH] ls -i: print consistent inode numbers also for mount points
Jim Meyering wrote: > Following up on a long thread from a year ago, here's a patch > to fix the 3.5-year-old readdir-vs-mountpoint-inode bug in ls -i. Checked on my system and works fine... > It's a shame to have to pessimize ls -i performance, even by this > small amount on linux-based kernels, but correctness definitely > trumps performance, here. Yep, it has some performance impact... checked `time ./ls -i1R /home /dev /usr /var /lib >myinodes with approximate number of 31k dirs, 450k files on ext3 and results are old binary without the patch: real2m5.631s user0m3.012s sys 0m4.815s new binary: real6m8.560s user0m3.947s sys 0m30.572s But correctness should be preferred as this is not so common usecase... > Review appreciated. Looks ok from my point of view. Greetings, Ondřej Vašík signature.asc Description: Toto je digitálně podepsaná část zprávy
Re: broken link
On Mon, 31 Aug 2009, Jim Meyering wrote: Can anyone suggest a replacement? This is the same content, and references the expita.com URL as the source: http://stagecraft.theprices.net/nomime.html Cheers, Phil
Re: broken link
Jim Meyering wrote: > Anarkia Komunismo wrote: >> You have a broken link on the page http://www.gnu.org/software/coreutils/ >> >> http://www.expita.com/nomime.html is expired and leads nowhere. Please >> check it out. > > Thank you. > Can anyone suggest a replacement? Here's the latest archived copy: http://web.archive.org/web/20080213142537/http://www.expita.com/nomime.html but I'd rather point to a live page, so alternative suggestions are welcome.
Re: broken link
Anarkia Komunismo wrote: > You have a broken link on the page http://www.gnu.org/software/coreutils/ > > http://www.expita.com/nomime.html is expired and leads nowhere. Please > check it out. Thank you. Can anyone suggest a replacement?
broken link
Hi there, You have a broken link on the page http://www.gnu.org/software/coreutils/ http://www.expita.com/nomime.html is expired and leads nowhere. Please check it out. Thanks.
new "rc" used-uninitialized warning in exclude.c
Hi Sergey,
In exclude.c, we have this:
bool
excluded_file_name (struct exclude const *ex, char const *f)
{
...
switch (seg->type)
{
case exclude_pattern:
rc = excluded_file_pattern_p (seg, f);
break;
case exclude_hash:
if (!filename)
filename = xmalloc (strlen (f) + 1);
rc = excluded_file_name_p (seg, f, filename);
break;
}
if (rc != excluded)
{
excluded = rc;
break;
}
which provokes a warning about "rc" possibly being used uninitialized.
While it appears this cannot happen in practice, it would be nice to
change the code not to evoke the warning.
If you're interested, there are several approaches:
1) add a default: abort (); in this single switch.
However, there are several others that handle only those two cases.
2) replace the switch with an if/else block: if (seg->type == exclude_pattern)
...
and handle the other case (and "impossible" other values) via the "else" clause.
3) more invasively, but perhaps more cleanly in the long run,
we could change seg's "type" member to be boolean.
Re: "-T" option help text
On Sun, 30 Aug 2009, James R. Van Zandt wrote:
For the help text, here are some alternatives:
if DEST is a directory, then delete it first
This isn't what -T does. If DEST is an empty directory then it's
overwritten with the rename(2) system call. Otherwise mv will fail e.g.
if SOURCE isn't a directory or DEST is non-empty.
This is short, but doesn't handle the case of DEST being a
symbolic link
if DEST is a directory, or a symbolic link to a directory, then delete it
first
This isn't what -T does either. If DEST is a symlink it isn't
deleted, merely "clobbered" by mv. It's the difference between
rename("file", "DEST/file") = 0
and
rename("file", "DEST") = 0
For the short --help description I'd try something like
"disable special directory handling for DEST"
Cheers,
Phil
Re: Bug#505927: just use the date(1) -d library instead of your own poorer date parser
>> >> However to get at to accept such a date, one needs: >> >> $ at -v $(date --rfc-3339=date -d 'now + 5 years + 11 months') >> >> Fri Oct 17 03:56:00 2014 >> >> warning: commands will be executed using /bin/sh B'B> How do you like these: B'B> at-ng/build% ./at now + 5 years + 11 months B'B> Job will run at or after Wed, 29 Jul 2015 20:26:00 +0200. B'B> It was a trivial fix adding just 2 lines to the grammar. I'll cc them to let them know...
