Re: Wishing rmdir had a prompt
On 9/11/19 10:29 AM, Lady Aleena wrote: I thank everyone who got involved with my little wish. I did not mean for it to explode like this. I am entirely comfortable letting the whole idea drop since there appears to be little to no support for it. I was just an idle wish that crossed my mind that I wanted to share. Again, thank you for reading it and taking the time to respond. I don't think this discussion "exploded". Instead, I think asking for such little wishes like you did is how free software works: one has an idea, other share their thoughts about it. And some of such ideas are considered valuable or some even cool - and easy and overall worthwhile to add - and therefore make it as a new feature into new versions of the software. In other cases like this, the discussion leads to a consent that existing functionality or alternatives are deemed good enough. So in this sense: thanks for asking. ;-) Have a nice day, Berny
[PATCH 3/3] ls: add statx-enabled variants of stat and lstat calls
* add wrapper functions for stat, lstat and stat_for_mode so that we
can conditionally plug in statx-enabled variants
* add statx-enabled functions and set the request mask based on the
output format and what values are needed
---
src/ls.c | 73
1 file changed, 69 insertions(+), 4 deletions(-)
diff --git a/src/ls.c b/src/ls.c
index 8e5015e5ac12..3c12f63e595b 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3166,6 +3166,71 @@ needs_quoting (char const* name)
return *name != *test || strlen (name) != len;
}
+#if USE_STATX
+static unsigned int
+calc_request_mask(void)
+{
+ unsigned int mask = STATX_MODE;
+
+ if (print_inode)
+mask |= STATX_INO;
+ if (format == long_format) {
+mask |= STATX_NLINK | STATX_SIZE;
+if (print_owner || print_author)
+ mask |= STATX_UID;
+if (print_group)
+ mask |= STATX_GID;
+ }
+ return mask;
+}
+
+static int
+do_statx (const char *name, struct stat *st, int flags, unsigned int mask)
+{
+ struct statx stx;
+ int ret = statx(AT_FDCWD, name, flags, mask, );
+ if (ret >= 0)
+statx_to_stat(, st);
+ return ret;
+}
+
+static inline int
+do_stat(const char *name, struct stat *st)
+{
+ return do_statx(name, st, 0, calc_request_mask());
+}
+
+static int
+do_lstat (const char *name, struct stat *st)
+{
+ return do_statx(name, st, AT_SYMLINK_NOFOLLOW, calc_request_mask());
+}
+
+static int
+stat_for_mode(const char *name, struct stat *st)
+{
+ return do_statx(name, st, 0, STATX_MODE);
+}
+#else
+static inline int
+do_stat (const char *name, struct stat *st)
+{
+ return stat(name, st);
+}
+
+static inline int
+do_lstat (const char *name, struct stat *st)
+{
+ return lstat(name, st);
+}
+
+static int
+stat_for_mode(const char *name, struct stat *st)
+{
+ return do_stat(name, st);
+}
+#endif
+
/* Add a file to the current table of files.
Verify that the file exists, and print an error message if it does not.
Return the number of blocks that the file occupies. */
@@ -3264,7 +3329,7 @@ gobble_file (char const *name, enum filetype type, ino_t
inode,
switch (dereference)
{
case DEREF_ALWAYS:
- err = stat (full_name, >stat);
+ err = do_stat (full_name, >stat);
do_deref = true;
break;
@@ -3273,7 +3338,7 @@ gobble_file (char const *name, enum filetype type, ino_t
inode,
if (command_line_arg)
{
bool need_lstat;
- err = stat (full_name, >stat);
+ err = do_stat (full_name, >stat);
do_deref = true;
if (dereference == DEREF_COMMAND_LINE_ARGUMENTS)
@@ -3293,7 +3358,7 @@ gobble_file (char const *name, enum filetype type, ino_t
inode,
FALLTHROUGH;
default: /* DEREF_NEVER */
- err = lstat (full_name, >stat);
+ err = do_lstat (full_name, >stat);
do_deref = false;
break;
}
@@ -3382,7 +3447,7 @@ gobble_file (char const *name, enum filetype type, ino_t
inode,
they won't be traced and when no indicator is needed. */
if (linkname
&& (file_type <= indicator_style || check_symlink_mode)
- && stat (linkname, ) == 0)
+ && stat_for_mode(linkname, ) == 0)
{
f->linkok = true;
f->linkmode = linkstats.st_mode;
--
2.21.0
[PATCH 1/3] stat: move struct statx to struct stat conversion routines to new header
* move statx_timestamp_to_timespec and statx_to_stat to a new header
---
src/stat.c | 32 +--
src/statx.h | 54 +
2 files changed, 55 insertions(+), 31 deletions(-)
create mode 100644 src/statx.h
diff --git a/src/stat.c b/src/stat.c
index ee68f1682bc8..f2bf0dcb7901 100644
--- a/src/stat.c
+++ b/src/stat.c
@@ -73,6 +73,7 @@
#include "strftime.h"
#include "find-mount-point.h"
#include "xvasprintf.h"
+#include "statx.h"
#if HAVE_STATX && defined STATX_INO
# define USE_STATX 1
@@ -1245,37 +1246,6 @@ static bool dont_sync;
static bool force_sync;
#if USE_STATX
-/* Much of the format printing requires a struct stat or timespec */
-static struct timespec
-statx_timestamp_to_timespec (struct statx_timestamp tsx)
-{
- struct timespec ts;
-
- ts.tv_sec = tsx.tv_sec;
- ts.tv_nsec = tsx.tv_nsec;
- return ts;
-}
-
-static void
-statx_to_stat (struct statx *stx, struct stat *stat)
-{
- stat->st_dev = makedev (stx->stx_dev_major, stx->stx_dev_minor);
- stat->st_ino = stx->stx_ino;
- stat->st_mode = stx->stx_mode;
- stat->st_nlink = stx->stx_nlink;
- stat->st_uid = stx->stx_uid;
- stat->st_gid = stx->stx_gid;
- stat->st_rdev = makedev (stx->stx_rdev_major, stx->stx_rdev_minor);
- stat->st_size = stx->stx_size;
- stat->st_blksize = stx->stx_blksize;
-/* define to avoid sc_prohibit_stat_st_blocks. */
-# define SC_ST_BLOCKS st_blocks
- stat->SC_ST_BLOCKS = stx->stx_blocks;
- stat->st_atim = statx_timestamp_to_timespec (stx->stx_atime);
- stat->st_mtim = statx_timestamp_to_timespec (stx->stx_mtime);
- stat->st_ctim = statx_timestamp_to_timespec (stx->stx_ctime);
-}
-
static unsigned int
fmt_to_mask (char fmt)
{
diff --git a/src/statx.h b/src/statx.h
new file mode 100644
index ..a1db527b3308
--- /dev/null
+++ b/src/statx.h
@@ -0,0 +1,54 @@
+/* statx -> stat conversion utilities for coreutils
+ Copyright (C) 2019 Free Software Foundation, Inc.
+
+ This program is free software: you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation, either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see
[PATCH 0/3] ls: convert it to use statx() when available
This patchset converts the ls command to use statx instead of stat when available. This allows ls to indicate interest in only certain inode metadata. This is potentially a win on networked/clustered/distributed filesystems. In cases where we'd have to do a full, heavyweight stat() call we can now do a much lighter statx() call. As a real-world example, consider a filesystem like CephFS where one client is actively writing to a file and another client does an ls --color in the same directory. --color means that we need to fetch the mode of the file. Doing that with a stat() call means that we have to fetch the size and mtime in addition to the mode. The MDS in that situation will have to revoke caps in order to ensure that it has up-to-date values to report, which disrupts the writer. This has a measurable affect on performance. I ran a fio sequential write test on one client and had a second client do "ls --color" in a tight loop on the directory that held the file: Baseline -- no activity on the second client: WRITE: bw=76.7MiB/s (80.4MB/s), 76.7MiB/s-76.7MiB/s (80.4MB/s-80.4MB/s), io=4600MiB (4824MB), run=60016-60016msec Without this patch series, we see a noticable performance hit: WRITE: bw=70.4MiB/s (73.9MB/s), 70.4MiB/s-70.4MiB/s (73.9MB/s-73.9MB/s), io=4228MiB (4433MB), run=60012-60012msec With this patch series, we gain most of that ground back: WRITE: bw=75.9MiB/s (79.6MB/s), 75.9MiB/s-75.9MiB/s (79.6MB/s-79.6MB/s), io=4555MiB (4776MB), run=60019-60019msec Jeff Layton (3): stat: move struct statx to struct stat conversion routines to new header ls: use statx for loop detection if it's available ls: add statx-enabled variants of stat and lstat calls src/ls.c| 179 src/stat.c | 32 +- src/statx.h | 54 3 files changed, 208 insertions(+), 57 deletions(-) create mode 100644 src/statx.h -- 2.21.0
[PATCH 2/3] ls: use statx for loop detection if it's available
* move loop detection routine into separate function
* add a statx-enabled variant that is used when it's available. No need
for a full stat since all we care about is the dev/ino.
* Since dev/ino should never change, set AT_STATX_DONT_SYNC
unconditionally.
---
src/ls.c | 106 +++
1 file changed, 84 insertions(+), 22 deletions(-)
diff --git a/src/ls.c b/src/ls.c
index 120ce153e340..8e5015e5ac12 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -114,6 +114,13 @@
#include "xgethostname.h"
#include "c-ctype.h"
#include "canonicalize.h"
+#include "statx.h"
+
+#if HAVE_STATX && defined STATX_INO
+# define USE_STATX 1
+#else
+# define USE_STATX 0
+#endif
/* Include last to avoid a clash of
include guards with some premature versions of libcap.
@@ -2711,27 +2718,54 @@ queue_directory (char const *name, char const
*realname, bool command_line_arg)
pending_dirs = new;
}
-/* Read directory NAME, and list the files in it.
- If REALNAME is nonzero, print its name instead of NAME;
- this is used for symbolic links to directories.
- COMMAND_LINE_ARG means this directory was mentioned on the command line. */
-
-static void
-print_dir (char const *name, char const *realname, bool command_line_arg)
+#if USE_STATX
+static bool
+loop_detected (const char *name, DIR *dirp, bool command_line_arg)
{
- DIR *dirp;
- struct dirent *next;
- uintmax_t total_blocks = 0;
- static bool first = true;
-
- errno = 0;
- dirp = opendir (name);
- if (!dirp)
+ if (LOOP_DETECT)
{
- file_failure (command_line_arg, _("cannot open directory %s"), name);
- return;
-}
+ struct statx stx;
+ int fd = dirfd (dirp);
+ int flags = AT_STATX_DONT_SYNC;
+ const char *pathname = name;
+
+ if (0 <= fd)
+ {
+ pathname = "";
+ flags |= AT_EMPTY_PATH;
+ }
+ else
+{
+ /* If dirfd failed, endure the overhead of statx by path. */
+ fd = AT_FDCWD;
+ }
+
+ if (statx (fd, pathname, flags, STATX_INO, ) < 0)
+{
+ file_failure (command_line_arg,
+_("cannot determine device and inode of %s"), name);
+ return true;
+}
+ /* If we've already visited this dev/inode pair, warn that
+ we've found a loop, and do not process this directory. */
+ dev_t dev = makedev (stx.stx_dev_major, stx.stx_dev_minor);
+ if (visit_dir (dev, stx.stx_ino))
+{
+ error (0, 0, _("%s: not listing already-listed directory"),
+ quotef (name));
+ set_exit_status (true);
+ return true;
+}
+
+ dev_ino_push (dev, stx.stx_ino);
+}
+ return false;
+}
+#else
+static bool
+loop_detected (const char *name, DIR *dirp, bool command_line_arg)
+{
if (LOOP_DETECT)
{
struct stat dir_stat;
@@ -2744,8 +2778,7 @@ print_dir (char const *name, char const *realname, bool
command_line_arg)
{
file_failure (command_line_arg,
_("cannot determine device and inode of %s"), name);
- closedir (dirp);
- return;
+ return true;
}
/* If we've already visited this dev/inode pair, warn that
@@ -2754,13 +2787,42 @@ print_dir (char const *name, char const *realname, bool
command_line_arg)
{
error (0, 0, _("%s: not listing already-listed directory"),
quotef (name));
- closedir (dirp);
set_exit_status (true);
- return;
+ return true;
}
dev_ino_push (dir_stat.st_dev, dir_stat.st_ino);
}
+ return false;
+}
+#endif
+
+/* Read directory NAME, and list the files in it.
+ If REALNAME is nonzero, print its name instead of NAME;
+ this is used for symbolic links to directories.
+ COMMAND_LINE_ARG means this directory was mentioned on the command line. */
+
+static void
+print_dir (char const *name, char const *realname, bool command_line_arg)
+{
+ DIR *dirp;
+ struct dirent *next;
+ uintmax_t total_blocks = 0;
+ static bool first = true;
+
+ errno = 0;
+ dirp = opendir (name);
+ if (!dirp)
+{
+ file_failure (command_line_arg, _("cannot open directory %s"), name);
+ return;
+}
+
+ if (loop_detected(name, dirp, command_line_arg))
+{
+ closedir (dirp);
+ return;
+}
clear_files ();
--
2.21.0
Re: Wishing rmdir had a prompt
I thank everyone who got involved with my little wish. I did not mean for it to explode like this. I am entirely comfortable letting the whole idea drop since there appears to be little to no support for it. I was just an idle wish that crossed my mind that I wanted to share. Again, thank you for reading it and taking the time to respond. Have a very nice day! Lady Aleena On 2019-09-02 11:22, Bernhard Voelker wrote: On 9/2/19 10:03 AM, Sami Kerola wrote: [...] I don't see any problem adding --interactive long only option. a) Bloat: Apart from the --parents option, rmdir(1) is just a simple wrapper around rmdir(2). Just recently I've been asked (off-list) why src/true.c needs to have 80+ lines while 3 lines would be enough. b) Alternatives: The case is trivial enough to have some little prompt wrapper around it. E.g. a bit contrived but works: $ xargs -p rmdir DIR c) As Kaz already wrote: it's easy enough to re-create an empty directory which has been deleted accidentally. d) My personal opinion: I'm not in favor of those "do you really want to XYZ?" questions - especially in the case one just explicitly told the tool exactly to "do XYZ". Similarly, one could argument why not to add it to sleep, kill, cat, and so on. Have a nice day, Berny
