[PATCH 0/7] A handful of small cleanups and improvements

[PATCH 0/7] A handful of small cleanups and improvements

[Dan McGee]

This is a short series of patches with some improvements and code cleanups.
Most should be rather independent of each other and I'm definitely open to any
feedback.

Dan McGee (7):
  Add .gitignore rules for generated binaries
  umount.nilfs2 (libmount): send proper error code to complain function
  Remove kern_compat.h
  Alpha sort AC_CHECK_FUNCS list in configure.ac
  Use strtoull instead of strtoul where applicable
  rmcp/chcp: sanity check for positive checkpoint number
  rmcp: print sensible error message on permission failure

 bin/.gitignore                 |    6 +++++
 bin/Makefile.am                |    2 +-
 bin/chcp.c                     |    7 +++--
 bin/dumpseg.c                  |    4 +-
 bin/rmcp.c                     |   10 +++++---
 configure.ac                   |    8 +++---
 include/Makefile.am            |    2 +-
 include/cno.h                  |    1 +
 include/kern_compat.h          |   47 ----------------------------------------
 lib/cno.c                      |   29 +++++++++++++++++------
 lib/feature.c                  |    2 +-
 sbin/cleanerd/.gitignore       |    1 +
 sbin/mkfs/.gitignore           |    1 +
 sbin/mkfs/mkfs.c               |    3 +-
 sbin/mkfs/mkfs.h               |    4 ---
 sbin/mount/.gitignore          |    2 +
 sbin/mount/umount_libmount.c   |    4 +--
 sbin/nilfs-clean/.gitignore    |    1 +
 sbin/nilfs-clean/nilfs-clean.c |    2 +-
 sbin/nilfs-resize/.gitignore   |    1 +
 sbin/nilfs-tune/.gitignore     |    1 +
 21 files changed, 58 insertions(+), 80 deletions(-)
 create mode 100644 bin/.gitignore
 delete mode 100644 include/kern_compat.h
 create mode 100644 sbin/cleanerd/.gitignore
 create mode 100644 sbin/mkfs/.gitignore
 create mode 100644 sbin/mount/.gitignore
 create mode 100644 sbin/nilfs-clean/.gitignore
 create mode 100644 sbin/nilfs-resize/.gitignore
 create mode 100644 sbin/nilfs-tune/.gitignore

-- 
1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] Add .gitignore rules for generated binaries

[Dan McGee]

This ignores all of the generated binaries so git status is clean after
building the project.

Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
---
 bin/.gitignore               |    6 ++++++
 sbin/cleanerd/.gitignore     |    1 +
 sbin/mkfs/.gitignore         |    1 +
 sbin/mount/.gitignore        |    2 ++
 sbin/nilfs-clean/.gitignore  |    1 +
 sbin/nilfs-resize/.gitignore |    1 +
 sbin/nilfs-tune/.gitignore   |    1 +
 7 files changed, 13 insertions(+), 0 deletions(-)
 create mode 100644 bin/.gitignore
 create mode 100644 sbin/cleanerd/.gitignore
 create mode 100644 sbin/mkfs/.gitignore
 create mode 100644 sbin/mount/.gitignore
 create mode 100644 sbin/nilfs-clean/.gitignore
 create mode 100644 sbin/nilfs-resize/.gitignore
 create mode 100644 sbin/nilfs-tune/.gitignore

diff --git a/bin/.gitignore b/bin/.gitignore
new file mode 100644
index 0000000..3718690
--- /dev/null
+++ b/bin/.gitignore
@@ -0,0 +1,6 @@
+/chcp
+/dumpseg
+/lscp
+/lssu
+/mkcp
+/rmcp
diff --git a/sbin/cleanerd/.gitignore b/sbin/cleanerd/.gitignore
new file mode 100644
index 0000000..b39e825
--- /dev/null
+++ b/sbin/cleanerd/.gitignore
@@ -0,0 +1 @@
+/nilfs_cleanerd
diff --git a/sbin/mkfs/.gitignore b/sbin/mkfs/.gitignore
new file mode 100644
index 0000000..0d8a9d6
--- /dev/null
+++ b/sbin/mkfs/.gitignore
@@ -0,0 +1 @@
+/mkfs.nilfs2
diff --git a/sbin/mount/.gitignore b/sbin/mount/.gitignore
new file mode 100644
index 0000000..766f1bb
--- /dev/null
+++ b/sbin/mount/.gitignore
@@ -0,0 +1,2 @@
+/mount.nilfs2
+/umount.nilfs2
diff --git a/sbin/nilfs-clean/.gitignore b/sbin/nilfs-clean/.gitignore
new file mode 100644
index 0000000..96d31d6
--- /dev/null
+++ b/sbin/nilfs-clean/.gitignore
@@ -0,0 +1 @@
+/nilfs-clean
diff --git a/sbin/nilfs-resize/.gitignore b/sbin/nilfs-resize/.gitignore
new file mode 100644
index 0000000..2e82991
--- /dev/null
+++ b/sbin/nilfs-resize/.gitignore
@@ -0,0 +1 @@
+/nilfs-resize
diff --git a/sbin/nilfs-tune/.gitignore b/sbin/nilfs-tune/.gitignore
new file mode 100644
index 0000000..e8a1d3e
--- /dev/null
+++ b/sbin/nilfs-tune/.gitignore
@@ -0,0 +1 @@
+/nilfs-tune
-- 
1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/7] umount.nilfs2 (libmount): send proper error code to complain function

[Dan McGee]

We were negating the error code sent to complain, which is wrong. The
documentation of mnt_context_do_umount() states that errors returned
from the umount() syscall will be returned >0, so we shouldn't be
doing a switch on the negated value. This fixes the following
non-helpful output:

Before:
    sudo sbin/mount/umount.nilfs2 /mnt/nilfs
    umount.nilfs2: /dev/loop0: Unknown error -16

After:
    $ sudo sbin/mount/umount.nilfs2 /mnt/nilfs
    umount.nilfs2: /dev/loop0: device is busy

Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
---

The complain() function was copied from the original umount code; I removed the
comment as it doesn't make much sense in the context of using libmount and not
parsing fstab directly.

 sbin/mount/umount_libmount.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/sbin/mount/umount_libmount.c b/sbin/mount/umount_libmount.c
index 939c6b0..2c983b3 100644
--- a/sbin/mount/umount_libmount.c
+++ b/sbin/mount/umount_libmount.c
@@ -183,8 +183,6 @@ static void complain(int err, const char *dev)
 		error(_("%s: %s: I/O error while unmounting"), progname, dev);
 		break;
 	case EBUSY:
-		/* Let us hope fstab has a line "proc /proc ..."
-		   and not "none /proc ..."*/
 		error(_("%s: %s: device is busy"), progname, dev);
 		break;
 	case ENOENT:
@@ -284,7 +282,7 @@ static int nilfs_umount_one(struct nilfs_umount_info *umi)
 	if (!mnt_context_is_fake(umi->cxt)) {
 		res = nilfs_do_umount_one(umi);
 		if (res) {
-			complain(-res, mnt_context_get_source(umi->cxt));
+			complain(res, mnt_context_get_source(umi->cxt));
 			goto failed;
 		}
 	}
-- 
1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/7] Remove kern_compat.h

[Dan McGee]

All necessary macros are already definied in include/nilfs.h, which we
can use instead. This also removes the duplicate definitions of
NILFS_CNO_MIN and NILFS_CNO_MAX.

Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
---

I would appreciate other people compile testing this one as some other
platforms or kernel headers might yield different results.

 include/Makefile.am   |    2 +-
 include/kern_compat.h |   47 -----------------------------------------------
 lib/feature.c         |    2 +-
 sbin/mkfs/mkfs.c      |    3 ++-
 sbin/mkfs/mkfs.h      |    4 ----
 5 files changed, 4 insertions(+), 54 deletions(-)
 delete mode 100644 include/kern_compat.h

diff --git a/include/Makefile.am b/include/Makefile.am
index 221c481..c99da99 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -1,5 +1,5 @@
 ## Makefile.am
 
 include_HEADERS = nilfs.h nilfs2_fs.h nilfs_cleaner.h
-noinst_HEADERS = realpath.h nls.h cno.h kern_compat.h nilfs_feature.h \
+noinst_HEADERS = realpath.h nls.h cno.h nilfs_feature.h \
 	vector.h nilfs_gc.h cnoconv.h cleaner_msg.h cleaner_exec.h
diff --git a/include/kern_compat.h b/include/kern_compat.h
deleted file mode 100644
index a2cffdc..0000000
--- a/include/kern_compat.h
+++ /dev/null
@@ -1,47 +0,0 @@
-/*
- * kern_compat.h - kernel compat declarations
- *
- * Copyright (C) 2005-2011 Nippon Telegraph and Telephone Corporation.
- *
- * This program can be redistributed under the terms of the GNU Lesser
- * General Public License.
- */
-
-#ifndef NILFS_KERN_COMPAT_H
-#define NILFS_KERN_COMPAT_H
-
-#include <linux/types.h>
-#include <endian.h>
-#include <byteswap.h>
-
-#ifndef __bitwise  /* Tricky workaround; should be replaced */
-typedef __u64 __le64;
-typedef __u32 __le32;
-typedef __u16 __le16;
-#endif
-
-#ifndef le32_to_cpu
-#if __BYTE_ORDER == __LITTLE_ENDIAN
-#define cpu_to_le16(x) (x)
-#define cpu_to_le32(x) (x)
-#define cpu_to_le64(x) (x)
-#define le16_to_cpu(x) (x)
-#define le32_to_cpu(x) (x)
-#define le64_to_cpu(x) (x)
-#elif __BYTE_ORDER == __BIG_ENDIAN
-#define le16_to_cpu(x) bswap_16(x)
-#define le32_to_cpu(x) bswap_32(x)
-#define le64_to_cpu(x) bswap_64(x)
-#define cpu_to_le16(x) bswap_16(x)
-#define cpu_to_le32(x) bswap_32(x)
-#define cpu_to_le64(x) bswap_64(x)
-#else
-#error "unsupported endian"
-#endif /* __BYTE_ORDER */
-#endif /* le32_to_cpu */
-
-#ifndef BUG
-#define BUG()	abort()
-#endif
-
-#endif	/* NILFS_KERN_COMPAT_H */
diff --git a/lib/feature.c b/lib/feature.c
index b346cea..b5752f2 100644
--- a/lib/feature.c
+++ b/lib/feature.c
@@ -43,7 +43,7 @@
 
 #include <errno.h>
 #include <assert.h>
-#include "kern_compat.h"
+#include "nilfs.h"
 #include "nilfs2_fs.h"
 #include "nilfs_feature.h"
 
diff --git a/sbin/mkfs/mkfs.c b/sbin/mkfs/mkfs.c
index 43a9657..870c682 100644
--- a/sbin/mkfs/mkfs.c
+++ b/sbin/mkfs/mkfs.c
@@ -72,8 +72,9 @@
 
 #include <errno.h>
 
-#include "mkfs.h"
+#include "nilfs.h"
 #include "nilfs_feature.h"
+#include "mkfs.h"
 
 
 typedef __u64  blocknr_t;
diff --git a/sbin/mkfs/mkfs.h b/sbin/mkfs/mkfs.h
index 643060c..ce065a0 100644
--- a/sbin/mkfs/mkfs.h
+++ b/sbin/mkfs/mkfs.h
@@ -24,7 +24,6 @@
  * Revised by Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org>.
  */
 
-#include "kern_compat.h"
 #include "nilfs2_fs.h"
 
 #define NILFS_DISKHDR_SIZE 		4096 /* HDD header (MBR+superblock) */
@@ -41,9 +40,6 @@
 #define NILFS_DEF_RESERVED_SEGMENTS     5    /* default percentage of reserved
 						segments: 5% */
 
-#define NILFS_CNO_MIN   1
-#define NILFS_CNO_MAX   (~(__u64)0)
-
 #define NILFS_MAX_BMAP_ROOT_PTRS	(NILFS_INODE_BMAP_SIZE - 1)
 #define NILFS_MIN_BLOCKSIZE		1024
 #define NILFS_MIN_NUSERSEGS		8    /* Minimum number of user
-- 
1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/7] Alpha sort AC_CHECK_FUNCS list in configure.ac

[Dan McGee]

And add strtoull(), which is used in several places. This also changes a
` to a ' character in the libuuid description to fix syntax highlighting
in some editors.

Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
---
 configure.ac |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index ccd8008..da671a7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -37,7 +37,7 @@ AC_ARG_ENABLE(libmount,
 # Checks for libraries.
 AC_CHECK_LIB([uuid], [uuid_generate],
 	[AC_DEFINE([HAVE_LIBUUID], 1,
-	  [Define to 1 if you have the `uuid' library (-luuid).])],
+	  [Define to 1 if you have the 'uuid' library (-luuid).])],
 	[AC_MSG_ERROR([UUID library not found])])
 
 LIB_POSIX_MQ=''
@@ -101,9 +101,9 @@ AC_FUNC_STRFTIME
 AC_FUNC_VPRINTF
 AC_CHECK_FUNC(posix_memalign,,
 	      [AC_MSG_ERROR([cannot find posix_memalign() function])])
-AC_CHECK_FUNCS([alarm atexit ftruncate getcwd gettimeofday localtime_r \
-		memmove memset munmap strchr strdup strerror strrchr strstr \
-		strtoul getgrgid getpwuid strcasecmp getmntent_r strtok_r])
+AC_CHECK_FUNCS([alarm atexit ftruncate getcwd getgrgid getmntent_r getpwuid \
+		gettimeofday localtime_r memmove memset munmap strcasecmp strchr \
+		strdup strerror strrchr strstr strtok_r strtoul strtoull])
 
 # Checks for system services
 AC_SYS_LARGEFILE
-- 
1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/7] Use strtoull instead of strtoul where applicable

[Dan McGee]

When parsing into u64 or unsigned long long storage, we should use the
correct library function.

Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
---
 bin/dumpseg.c                  |    4 ++--
 sbin/nilfs-clean/nilfs-clean.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/bin/dumpseg.c b/bin/dumpseg.c
index fbc587d..d6b4290 100644
--- a/bin/dumpseg.c
+++ b/bin/dumpseg.c
@@ -211,7 +211,7 @@ int main(int argc, char *argv[])
 		fprintf(stderr, "%s: too few arguments\n", progname);
 		exit(1);
 	} else {
-		strtoul(argv[optind], &endptr, DUMPSEG_BASE);
+		strtoull(argv[optind], &endptr, DUMPSEG_BASE);
 		if (*endptr == '\0')
 			dev = NULL;
 		else
@@ -229,7 +229,7 @@ int main(int argc, char *argv[])
 
 	status = 0;
 	for (i = optind; i < argc; i++) {
-		segnum = strtoul(argv[i], &endptr, DUMPSEG_BASE);
+		segnum = strtoull(argv[i], &endptr, DUMPSEG_BASE);
 		if (*endptr != '\0') {
 			fprintf(stderr, "%s: %s: invalid segment number\n",
 				progname, argv[i]);
diff --git a/sbin/nilfs-clean/nilfs-clean.c b/sbin/nilfs-clean/nilfs-clean.c
index 9d92b67..25aa14a 100644
--- a/sbin/nilfs-clean/nilfs-clean.c
+++ b/sbin/nilfs-clean/nilfs-clean.c
@@ -331,7 +331,7 @@ static int nilfs_clean_parse_protection_period(const char *arg)
 	char *endptr;
 	int ret = 0;
 
-	period = strtoul(arg, &endptr, 10);
+	period = strtoull(arg, &endptr, 10);
 	if (endptr == arg) {
 		myprintf(_("Error: invalid protection period: %s\n"), arg);
 		ret = -1;
-- 
1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/7] rmcp/chcp: sanity check for positive checkpoint number

[Dan McGee]

These two utilities parse a command line argument with either a
checkpoint number or range. Unfortunately, an unchecked strtoull() call
happily parses a negative number (casting it to a really large unsigned
number).

For rmcp, ensure the single number or range passed in is not negative.
Before, this caused a near-infinite loop with a command such as
`rmcp -- -1`.

For chcp, also ensure the passed in number is not negative by using the
new helper function introduced for the range validation,
nilfs_parse_cno().

Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
---

Some other commands to try for fuzz testing:

$ rmcp -- '-234'
$ rmcp -- ' -234'
$ rmcp 4135860689012436789324689324061328960812949788907

 bin/Makefile.am |    2 +-
 bin/chcp.c      |    7 ++++---
 include/cno.h   |    1 +
 lib/cno.c       |   29 +++++++++++++++++++++--------
 4 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/bin/Makefile.am b/bin/Makefile.am
index 78928d0..32b6ee8 100644
--- a/bin/Makefile.am
+++ b/bin/Makefile.am
@@ -7,7 +7,7 @@ LDADD = $(top_builddir)/lib/libnilfs.la
 bin_PROGRAMS = chcp dumpseg lscp lssu mkcp rmcp
 
 chcp_SOURCES = chcp.c
-chcp_LDADD = $(LDADD) $(LIB_POSIX_SEM)
+chcp_LDADD = $(LDADD) $(LIB_POSIX_SEM) $(top_builddir)/lib/libcno.la
 
 dumpseg_SOURCES = dumpseg.c
 
diff --git a/bin/chcp.c b/bin/chcp.c
index 13a73d7..a7072ef 100644
--- a/bin/chcp.c
+++ b/bin/chcp.c
@@ -49,6 +49,7 @@
 #include <errno.h>
 #include <signal.h>
 #include "nilfs.h"
+#include "cno.h"
 
 
 #define CHCP_MODE_CP	"cp"
@@ -121,7 +122,7 @@ int main(int argc, char *argv[])
 		dev = NULL;
 	} else {
 		modestr = argv[optind++];
-		strtoul(argv[optind], &endptr, CHCP_BASE);
+		nilfs_parse_cno(argv[optind], &endptr, CHCP_BASE);
 		if (*endptr == '\0')
 			dev = NULL;
 		else
@@ -178,8 +179,8 @@ int main(int argc, char *argv[])
 			break;
 		}
 
-		cno = strtoul(argv[optind], &endptr, CHCP_BASE);
-		if (*endptr != '\0') {
+		cno = nilfs_parse_cno(argv[optind], &endptr, CHCP_BASE);
+		if (cno >= NILFS_CNO_MAX || *endptr != '\0') {
 			fprintf(stderr, "%s: %s: invalid checkpoint number\n",
 				progname, argv[optind]);
 			status = 1;
diff --git a/include/cno.h b/include/cno.h
index 2d5c4af..79a327f 100644
--- a/include/cno.h
+++ b/include/cno.h
@@ -10,6 +10,7 @@
 #ifndef NILFS_CNO_H
 #define NILFS_CNO_H
 
+extern nilfs_cno_t nilfs_parse_cno(const char *arg, char **endptr, int base);
 extern int nilfs_parse_cno_range(const char *arg, __u64 *start, __u64 *end,
 				 int base);
 
diff --git a/lib/cno.c b/lib/cno.c
index 2391f0b..7dcdb71 100644
--- a/lib/cno.c
+++ b/lib/cno.c
@@ -35,8 +35,21 @@
 #endif	/* HAVE_STRING_H */
 
 #include <assert.h>
+#include <ctype.h>
 #include "nilfs.h"
 
+nilfs_cno_t nilfs_parse_cno(const char *arg, char **endptr, int base)
+{
+	/* ensure the number we are about to parse is not negative, which
+	 * strtoull() will happily accept and cast to an unsigned value. */
+	while (isspace(*arg))
+		arg++;
+	if (*arg == '-')
+		return NILFS_CNO_MAX;
+
+	return strtoull(arg, endptr, base);
+}
+
 int nilfs_parse_cno_range(const char *arg, __u64 *start, __u64 *end, int base)
 {
 	const char *delim;
@@ -49,8 +62,8 @@ int nilfs_parse_cno_range(const char *arg, __u64 *start, __u64 *end, int base)
 	if (delim && delim == arg) {
 		if (arg[2] != '\0') {
 			/* ..yyy */
-			cno = strtoull(arg + 2, &endptr, base);
-			if (*endptr == '\0') {
+			cno = nilfs_parse_cno(arg + 2, &endptr, base);
+			if (cno < NILFS_CNO_MAX && *endptr == '\0') {
 				/* ..CNO */
 				*start = NILFS_CNO_MIN;
 				*end = cno;
@@ -59,24 +72,24 @@ int nilfs_parse_cno_range(const char *arg, __u64 *start, __u64 *end, int base)
 		}
 	} else if (!delim) {
 		/* xxx */
-		cno = strtoull(arg, &endptr, base);
-		if (*endptr == '\0') {
+		cno = nilfs_parse_cno(arg, &endptr, base);
+		if (cno < NILFS_CNO_MAX && *endptr == '\0') {
 			/* CNO */
 			*start = *end = cno;
 			return 0;
 		}
 	} else {
 		/* xxx..yyy */
-		cno = strtoull(arg, &endptr, base);
-		if (endptr == delim) {
+		cno = nilfs_parse_cno(arg, &endptr, base);
+		if (cno < NILFS_CNO_MAX && endptr == delim) {
 			if (delim[2] == '\0') {
 				/* CNO.. */
 				*start = cno;
 				*end = NILFS_CNO_MAX;
 				return 0;
 			}
-			cno2 = strtoull(delim + 2, &endptr, base);
-			if (*endptr == '\0') {
+			cno2 = nilfs_parse_cno(delim + 2, &endptr, base);
+			if (cno2 < NILFS_CNO_MAX && *endptr == '\0') {
 				/* CNO..CNO */
 				*start = cno;
 				*end = cno2;
-- 
1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/7] rmcp: print sensible error message on permission failure

[Dan McGee]

When running as non-root, the error message seems to indicate a
checkpoint is a snapshot, rather than the actual problem, which is that
I do not have permission to delete the checkpoints.

Before:
    $ rmcp 29..35
    rmcp: 29: cannot remove snapshot
    rmcp: 30: cannot remove snapshot
    rmcp: 31: cannot remove snapshot
    rmcp: 32: cannot remove snapshot
    rmcp: 33: cannot remove snapshot
    rmcp: 34: cannot remove snapshot
    rmcp: 35: cannot remove snapshot

    To delete snapshot(s), change them into checkpoints with
    chcp command before removal.

After:
    $ ./bin/rmcp 29..35
    lt-rmcp: 29: cannot remove checkpoint: Operation not permitted
    lt-rmcp: 30: cannot remove checkpoint: Operation not permitted
    lt-rmcp: 31: cannot remove checkpoint: Operation not permitted
    lt-rmcp: 32: cannot remove checkpoint: Operation not permitted
    lt-rmcp: 33: cannot remove checkpoint: Operation not permitted
    lt-rmcp: 34: cannot remove checkpoint: Operation not permitted
    lt-rmcp: 35: cannot remove checkpoint: Operation not permitted

Since May 2009 the kernel has returned EBUSY instead of EPERM for
removal requests against snapshots, commit 30c25be71fcbd87fd. We should
be able to treat EPERM as expected now, as the commit message there
indicates.

Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
---

This does introduce a behavior change as well- we no longer exit the range loop
immediately on an error code, instead we continue the loop and attempt to
delete the rest of the checkpoints in the range. Objections to this? I'm not an
expert by any means.

 bin/rmcp.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/bin/rmcp.c b/bin/rmcp.c
index 780a192..3c5c184 100644
--- a/bin/rmcp.c
+++ b/bin/rmcp.c
@@ -105,7 +105,7 @@ static int rmcp_remove_range(struct nilfs *nilfs,
 			nd++;
 			continue;
 		}
-		if (errno == EBUSY || errno == EPERM) {
+		if (errno == EBUSY) {
 			nss++;
 			if (!force) {
 				fprintf(stderr,
@@ -115,14 +115,16 @@ static int rmcp_remove_range(struct nilfs *nilfs,
 		} else if (errno == ENOENT) {
 			nocp++;
 		} else {
-			fprintf(stderr, "%s: %s\n", progname, strerror(errno));
+			fprintf(stderr,
+				"%s: %llu: cannot remove checkpoint: %s\n",
+				progname, (unsigned long long)cno,
+				strerror(errno));
 			ret = -1;
-			goto out;
 		}
 	}
 	if (!force && (nss > 0 || (nocp > 0 && nd == 0)))
 		ret = 1;
- out:
+
 	*ndeleted = nd;
 	*nsnapshots = nss;
 	return ret;
-- 
1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] A handful of small cleanups and improvements

[Ryusuke Konishi]

Hi,
On Wed, 21 Dec 2011 15:34:02 -0600, Dan McGee wrote:
> This is a short series of patches with some improvements and code cleanups.
> Most should be rather independent of each other and I'm definitely open to any
> feedback.

Thank you for posting this series.
I will review them each later.

Regards,
Ryusuke Konishi

> Dan McGee (7):
>   Add .gitignore rules for generated binaries
>   umount.nilfs2 (libmount): send proper error code to complain function
>   Remove kern_compat.h
>   Alpha sort AC_CHECK_FUNCS list in configure.ac
>   Use strtoull instead of strtoul where applicable
>   rmcp/chcp: sanity check for positive checkpoint number
>   rmcp: print sensible error message on permission failure
> 
>  bin/.gitignore                 |    6 +++++
>  bin/Makefile.am                |    2 +-
>  bin/chcp.c                     |    7 +++--
>  bin/dumpseg.c                  |    4 +-
>  bin/rmcp.c                     |   10 +++++---
>  configure.ac                   |    8 +++---
>  include/Makefile.am            |    2 +-
>  include/cno.h                  |    1 +
>  include/kern_compat.h          |   47 ----------------------------------------
>  lib/cno.c                      |   29 +++++++++++++++++------
>  lib/feature.c                  |    2 +-
>  sbin/cleanerd/.gitignore       |    1 +
>  sbin/mkfs/.gitignore           |    1 +
>  sbin/mkfs/mkfs.c               |    3 +-
>  sbin/mkfs/mkfs.h               |    4 ---
>  sbin/mount/.gitignore          |    2 +
>  sbin/mount/umount_libmount.c   |    4 +--
>  sbin/nilfs-clean/.gitignore    |    1 +
>  sbin/nilfs-clean/nilfs-clean.c |    2 +-
>  sbin/nilfs-resize/.gitignore   |    1 +
>  sbin/nilfs-tune/.gitignore     |    1 +
>  21 files changed, 58 insertions(+), 80 deletions(-)
>  create mode 100644 bin/.gitignore
>  delete mode 100644 include/kern_compat.h
>  create mode 100644 sbin/cleanerd/.gitignore
>  create mode 100644 sbin/mkfs/.gitignore
>  create mode 100644 sbin/mount/.gitignore
>  create mode 100644 sbin/nilfs-clean/.gitignore
>  create mode 100644 sbin/nilfs-resize/.gitignore
>  create mode 100644 sbin/nilfs-tune/.gitignore
> 
> -- 
> 1.7.8
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] Remove kern_compat.h

[Ryusuke Konishi]

On Wed, 21 Dec 2011 15:34:05 -0600, Dan McGee wrote:
> All necessary macros are already definied in include/nilfs.h, which we
> can use instead. This also removes the duplicate definitions of
> NILFS_CNO_MIN and NILFS_CNO_MAX.
> 
> Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
> ---
> 
> I would appreciate other people compile testing this one as some other
> platforms or kernel headers might yield different results.

This is working in several different environments so far.

The separation of kern_compat.h was intentional.  But, I have decided
to accept this change because nilfs.h needs the same definitions
anyway.

These difinitions should be cleaned out from nilfs.h, but it's the
only realistic way that I know at present to support wide variety of
platforms.

Thanks,
Ryusuke Konishi


>  include/Makefile.am   |    2 +-
>  include/kern_compat.h |   47 -----------------------------------------------
>  lib/feature.c         |    2 +-
>  sbin/mkfs/mkfs.c      |    3 ++-
>  sbin/mkfs/mkfs.h      |    4 ----
>  5 files changed, 4 insertions(+), 54 deletions(-)
>  delete mode 100644 include/kern_compat.h
> 
> diff --git a/include/Makefile.am b/include/Makefile.am
> index 221c481..c99da99 100644
> --- a/include/Makefile.am
> +++ b/include/Makefile.am
> @@ -1,5 +1,5 @@
>  ## Makefile.am
>  
>  include_HEADERS = nilfs.h nilfs2_fs.h nilfs_cleaner.h
> -noinst_HEADERS = realpath.h nls.h cno.h kern_compat.h nilfs_feature.h \
> +noinst_HEADERS = realpath.h nls.h cno.h nilfs_feature.h \
>  	vector.h nilfs_gc.h cnoconv.h cleaner_msg.h cleaner_exec.h
> diff --git a/include/kern_compat.h b/include/kern_compat.h
> deleted file mode 100644
> index a2cffdc..0000000
> --- a/include/kern_compat.h
> +++ /dev/null
> @@ -1,47 +0,0 @@
> -/*
> - * kern_compat.h - kernel compat declarations
> - *
> - * Copyright (C) 2005-2011 Nippon Telegraph and Telephone Corporation.
> - *
> - * This program can be redistributed under the terms of the GNU Lesser
> - * General Public License.
> - */
> -
> -#ifndef NILFS_KERN_COMPAT_H
> -#define NILFS_KERN_COMPAT_H
> -
> -#include <linux/types.h>
> -#include <endian.h>
> -#include <byteswap.h>
> -
> -#ifndef __bitwise  /* Tricky workaround; should be replaced */
> -typedef __u64 __le64;
> -typedef __u32 __le32;
> -typedef __u16 __le16;
> -#endif
> -
> -#ifndef le32_to_cpu
> -#if __BYTE_ORDER == __LITTLE_ENDIAN
> -#define cpu_to_le16(x) (x)
> -#define cpu_to_le32(x) (x)
> -#define cpu_to_le64(x) (x)
> -#define le16_to_cpu(x) (x)
> -#define le32_to_cpu(x) (x)
> -#define le64_to_cpu(x) (x)
> -#elif __BYTE_ORDER == __BIG_ENDIAN
> -#define le16_to_cpu(x) bswap_16(x)
> -#define le32_to_cpu(x) bswap_32(x)
> -#define le64_to_cpu(x) bswap_64(x)
> -#define cpu_to_le16(x) bswap_16(x)
> -#define cpu_to_le32(x) bswap_32(x)
> -#define cpu_to_le64(x) bswap_64(x)
> -#else
> -#error "unsupported endian"
> -#endif /* __BYTE_ORDER */
> -#endif /* le32_to_cpu */
> -
> -#ifndef BUG
> -#define BUG()	abort()
> -#endif
> -
> -#endif	/* NILFS_KERN_COMPAT_H */
> diff --git a/lib/feature.c b/lib/feature.c
> index b346cea..b5752f2 100644
> --- a/lib/feature.c
> +++ b/lib/feature.c
> @@ -43,7 +43,7 @@
>  
>  #include <errno.h>
>  #include <assert.h>
> -#include "kern_compat.h"
> +#include "nilfs.h"
>  #include "nilfs2_fs.h"
>  #include "nilfs_feature.h"
>  
> diff --git a/sbin/mkfs/mkfs.c b/sbin/mkfs/mkfs.c
> index 43a9657..870c682 100644
> --- a/sbin/mkfs/mkfs.c
> +++ b/sbin/mkfs/mkfs.c
> @@ -72,8 +72,9 @@
>  
>  #include <errno.h>
>  
> -#include "mkfs.h"
> +#include "nilfs.h"
>  #include "nilfs_feature.h"
> +#include "mkfs.h"
>  
>  
>  typedef __u64  blocknr_t;
> diff --git a/sbin/mkfs/mkfs.h b/sbin/mkfs/mkfs.h
> index 643060c..ce065a0 100644
> --- a/sbin/mkfs/mkfs.h
> +++ b/sbin/mkfs/mkfs.h
> @@ -24,7 +24,6 @@
>   * Revised by Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org>.
>   */
>  
> -#include "kern_compat.h"
>  #include "nilfs2_fs.h"
>  
>  #define NILFS_DISKHDR_SIZE 		4096 /* HDD header (MBR+superblock) */
> @@ -41,9 +40,6 @@
>  #define NILFS_DEF_RESERVED_SEGMENTS     5    /* default percentage of reserved
>  						segments: 5% */
>  
> -#define NILFS_CNO_MIN   1
> -#define NILFS_CNO_MAX   (~(__u64)0)
> -
>  #define NILFS_MAX_BMAP_ROOT_PTRS	(NILFS_INODE_BMAP_SIZE - 1)
>  #define NILFS_MIN_BLOCKSIZE		1024
>  #define NILFS_MIN_NUSERSEGS		8    /* Minimum number of user
> -- 
> 1.7.8
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] rmcp: print sensible error message on permission failure

[Ryusuke Konishi]

On Wed, 21 Dec 2011 15:34:09 -0600, Dan McGee wrote:
> When running as non-root, the error message seems to indicate a
> checkpoint is a snapshot, rather than the actual problem, which is that
> I do not have permission to delete the checkpoints.
> 
> Before:
>     $ rmcp 29..35
>     rmcp: 29: cannot remove snapshot
>     rmcp: 30: cannot remove snapshot
>     rmcp: 31: cannot remove snapshot
>     rmcp: 32: cannot remove snapshot
>     rmcp: 33: cannot remove snapshot
>     rmcp: 34: cannot remove snapshot
>     rmcp: 35: cannot remove snapshot
> 
>     To delete snapshot(s), change them into checkpoints with
>     chcp command before removal.
> 
> After:
>     $ ./bin/rmcp 29..35
>     lt-rmcp: 29: cannot remove checkpoint: Operation not permitted
>     lt-rmcp: 30: cannot remove checkpoint: Operation not permitted
>     lt-rmcp: 31: cannot remove checkpoint: Operation not permitted
>     lt-rmcp: 32: cannot remove checkpoint: Operation not permitted
>     lt-rmcp: 33: cannot remove checkpoint: Operation not permitted
>     lt-rmcp: 34: cannot remove checkpoint: Operation not permitted
>     lt-rmcp: 35: cannot remove checkpoint: Operation not permitted
> 
> Since May 2009 the kernel has returned EBUSY instead of EPERM for
> removal requests against snapshots, commit 30c25be71fcbd87fd. We should
> be able to treat EPERM as expected now, as the commit message there
> indicates.
> 
> Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
> ---
> 
> This does introduce a behavior change as well- we no longer exit the range loop
> immediately on an error code, instead we continue the loop and attempt to
> delete the rest of the checkpoints in the range. Objections to this? I'm not an
> expert by any means.
> 
>  bin/rmcp.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/bin/rmcp.c b/bin/rmcp.c
> index 780a192..3c5c184 100644
> --- a/bin/rmcp.c
> +++ b/bin/rmcp.c
> @@ -105,7 +105,7 @@ static int rmcp_remove_range(struct nilfs *nilfs,
>  			nd++;
>  			continue;
>  		}
> -		if (errno == EBUSY || errno == EPERM) {
> +		if (errno == EBUSY) {

OK, let's stop regarding the EPERM error as EBUSY.  For most kernels,
it's more important to distinguish real permission errors from the
snapshot removal error.

We have only recently bumped the version of nilfs-utils to 2.1.x from
2.0.x.  It's a good opportunity for this change.

>  			nss++;
>  			if (!force) {
>  				fprintf(stderr,
> @@ -115,14 +115,16 @@ static int rmcp_remove_range(struct nilfs *nilfs,
>  		} else if (errno == ENOENT) {
>  			nocp++;
>  		} else {
> -			fprintf(stderr, "%s: %s\n", progname, strerror(errno));
> +			fprintf(stderr,
> +				"%s: %llu: cannot remove checkpoint: %s\n",
> +				progname, (unsigned long long)cno,
> +				strerror(errno));
>  			ret = -1;

> -			goto out;

Could you revise this ?

I think the loop should abort immidiately when the rmcp meets an
important error even though it will change behavior for the real
permission error.

Thanks,
Ryusuke Konishi

>  		}
>  	}
>  	if (!force && (nss > 0 || (nocp > 0 && nd == 0)))
>  		ret = 1;
> - out:
> +
>  	*ndeleted = nd;
>  	*nsnapshots = nss;
>  	return ret;
> -- 
> 1.7.8
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] A handful of small cleanups and improvements

[Ryusuke Konishi]

Hi,
On Fri, 23 Dec 2011 01:35:41 +0900 (JST), Ryusuke Konishi wrote:
> Hi,
> On Wed, 21 Dec 2011 15:34:02 -0600, Dan McGee wrote:
> > This is a short series of patches with some improvements and code cleanups.
> > Most should be rather independent of each other and I'm definitely open to any
> > feedback.
> 
> Thank you for posting this series.
> I will review them each later.
> 
> Regards,
> Ryusuke Konishi

Most of the patches looks fine to me.

A nice cleanup series, thank you.

My only objection is for a change in error handling of the patch 7/7.
I already added the comment on the patch, please consider revising it.

Then, I will apply them all toward nilfs-utils-2.1.1 release.


Thank you,
Ryusuke Konishi


> > Dan McGee (7):
> >   Add .gitignore rules for generated binaries
> >   umount.nilfs2 (libmount): send proper error code to complain function
> >   Remove kern_compat.h
> >   Alpha sort AC_CHECK_FUNCS list in configure.ac
> >   Use strtoull instead of strtoul where applicable
> >   rmcp/chcp: sanity check for positive checkpoint number
> >   rmcp: print sensible error message on permission failure
> > 
> >  bin/.gitignore                 |    6 +++++
> >  bin/Makefile.am                |    2 +-
> >  bin/chcp.c                     |    7 +++--
> >  bin/dumpseg.c                  |    4 +-
> >  bin/rmcp.c                     |   10 +++++---
> >  configure.ac                   |    8 +++---
> >  include/Makefile.am            |    2 +-
> >  include/cno.h                  |    1 +
> >  include/kern_compat.h          |   47 ----------------------------------------
> >  lib/cno.c                      |   29 +++++++++++++++++------
> >  lib/feature.c                  |    2 +-
> >  sbin/cleanerd/.gitignore       |    1 +
> >  sbin/mkfs/.gitignore           |    1 +
> >  sbin/mkfs/mkfs.c               |    3 +-
> >  sbin/mkfs/mkfs.h               |    4 ---
> >  sbin/mount/.gitignore          |    2 +
> >  sbin/mount/umount_libmount.c   |    4 +--
> >  sbin/nilfs-clean/.gitignore    |    1 +
> >  sbin/nilfs-clean/nilfs-clean.c |    2 +-
> >  sbin/nilfs-resize/.gitignore   |    1 +
> >  sbin/nilfs-tune/.gitignore     |    1 +
> >  21 files changed, 58 insertions(+), 80 deletions(-)
> >  create mode 100644 bin/.gitignore
> >  delete mode 100644 include/kern_compat.h
> >  create mode 100644 sbin/cleanerd/.gitignore
> >  create mode 100644 sbin/mkfs/.gitignore
> >  create mode 100644 sbin/mount/.gitignore
> >  create mode 100644 sbin/nilfs-clean/.gitignore
> >  create mode 100644 sbin/nilfs-resize/.gitignore
> >  create mode 100644 sbin/nilfs-tune/.gitignore
> > 
> > -- 
> > 1.7.8
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] rmcp: print sensible error message on permission failure

[Dan McGee]

On Fri, Dec 23, 2011 at 12:56 AM, Ryusuke Konishi
<konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org> wrote:
> On Wed, 21 Dec 2011 15:34:09 -0600, Dan McGee wrote:
>> When running as non-root, the error message seems to indicate a
>> checkpoint is a snapshot, rather than the actual problem, which is that
>> I do not have permission to delete the checkpoints.
>>
>> Before:
>>     $ rmcp 29..35
>>     rmcp: 29: cannot remove snapshot
>>     rmcp: 30: cannot remove snapshot
>>     rmcp: 31: cannot remove snapshot
>>     rmcp: 32: cannot remove snapshot
>>     rmcp: 33: cannot remove snapshot
>>     rmcp: 34: cannot remove snapshot
>>     rmcp: 35: cannot remove snapshot
>>
>>     To delete snapshot(s), change them into checkpoints with
>>     chcp command before removal.
>>
>> After:
>>     $ ./bin/rmcp 29..35
>>     lt-rmcp: 29: cannot remove checkpoint: Operation not permitted
>>     lt-rmcp: 30: cannot remove checkpoint: Operation not permitted
>>     lt-rmcp: 31: cannot remove checkpoint: Operation not permitted
>>     lt-rmcp: 32: cannot remove checkpoint: Operation not permitted
>>     lt-rmcp: 33: cannot remove checkpoint: Operation not permitted
>>     lt-rmcp: 34: cannot remove checkpoint: Operation not permitted
>>     lt-rmcp: 35: cannot remove checkpoint: Operation not permitted
>>
>> Since May 2009 the kernel has returned EBUSY instead of EPERM for
>> removal requests against snapshots, commit 30c25be71fcbd87fd. We should
>> be able to treat EPERM as expected now, as the commit message there
>> indicates.
>>
>> Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
>> ---
>>
>> This does introduce a behavior change as well- we no longer exit the range loop
>> immediately on an error code, instead we continue the loop and attempt to
>> delete the rest of the checkpoints in the range. Objections to this? I'm not an
>> expert by any means.
>>
>>  bin/rmcp.c |   10 ++++++----
>>  1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/bin/rmcp.c b/bin/rmcp.c
>> index 780a192..3c5c184 100644
>> --- a/bin/rmcp.c
>> +++ b/bin/rmcp.c
>> @@ -105,7 +105,7 @@ static int rmcp_remove_range(struct nilfs *nilfs,
>>                       nd++;
>>                       continue;
>>               }
>> -             if (errno == EBUSY || errno == EPERM) {
>> +             if (errno == EBUSY) {
>
> OK, let's stop regarding the EPERM error as EBUSY.  For most kernels,
> it's more important to distinguish real permission errors from the
> snapshot removal error.
>
> We have only recently bumped the version of nilfs-utils to 2.1.x from
> 2.0.x.  It's a good opportunity for this change.
>
>>                       nss++;
>>                       if (!force) {
>>                               fprintf(stderr,
>> @@ -115,14 +115,16 @@ static int rmcp_remove_range(struct nilfs *nilfs,
>>               } else if (errno == ENOENT) {
>>                       nocp++;
>>               } else {
>> -                     fprintf(stderr, "%s: %s\n", progname, strerror(errno));
>> +                     fprintf(stderr,
>> +                             "%s: %llu: cannot remove checkpoint: %s\n",
>> +                             progname, (unsigned long long)cno,
>> +                             strerror(errno));
>>                       ret = -1;
>
>> -                     goto out;
>
> Could you revise this ?
>
> I think the loop should abort immidiately when the rmcp meets an
> important error even though it will change behavior for the real
> permission error.

1. Please define "important"- is EPERM important? I would tend to say
the error should be printed once per checkpoint as it is now in this
case for EPERM, and then restore the prior behavior for other actions.
2. I'm not very fond of the error messages with the immediate exit:

    $ ./bin/rmcp 100..105 115..125
    lt-rmcp: 100: cannot remove checkpoint: Operation not permitted
    $ echo $?
    1

Did 101-105 succeed? Did the second range succeed? Judging from this
(lack of) output, I would think it did, which is quite obviously not
the case.

So I would vote to not exit early in at least the semi-normal EPERM
case; perhaps we do for the others. What is the reasoning behind
exiting early in even the abnormal error cases?

>>               }
>>       }
>>       if (!force && (nss > 0 || (nocp > 0 && nd == 0)))
>>               ret = 1;
>> - out:
>> +
>>       *ndeleted = nd;
>>       *nsnapshots = nss;
>>       return ret;
>> --
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] rmcp: print sensible error message on permission failure

[Ryusuke Konishi]

Hi Dan,
On Fri, 23 Dec 2011 10:00:54 -0500, Dan McGee wrote:
> On Fri, Dec 23, 2011 at 12:56 AM, Ryusuke Konishi
> <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org> wrote:
> > On Wed, 21 Dec 2011 15:34:09 -0600, Dan McGee wrote:
> >> When running as non-root, the error message seems to indicate a
> >> checkpoint is a snapshot, rather than the actual problem, which is that
> >> I do not have permission to delete the checkpoints.
> >>
> >> Before:
> >>     $ rmcp 29..35
> >>     rmcp: 29: cannot remove snapshot
> >>     rmcp: 30: cannot remove snapshot
> >>     rmcp: 31: cannot remove snapshot
> >>     rmcp: 32: cannot remove snapshot
> >>     rmcp: 33: cannot remove snapshot
> >>     rmcp: 34: cannot remove snapshot
> >>     rmcp: 35: cannot remove snapshot
> >>
> >>     To delete snapshot(s), change them into checkpoints with
> >>     chcp command before removal.
> >>
> >> After:
> >>     $ ./bin/rmcp 29..35
> >>     lt-rmcp: 29: cannot remove checkpoint: Operation not permitted
> >>     lt-rmcp: 30: cannot remove checkpoint: Operation not permitted
> >>     lt-rmcp: 31: cannot remove checkpoint: Operation not permitted
> >>     lt-rmcp: 32: cannot remove checkpoint: Operation not permitted
> >>     lt-rmcp: 33: cannot remove checkpoint: Operation not permitted
> >>     lt-rmcp: 34: cannot remove checkpoint: Operation not permitted
> >>     lt-rmcp: 35: cannot remove checkpoint: Operation not permitted
> >>
> >> Since May 2009 the kernel has returned EBUSY instead of EPERM for
> >> removal requests against snapshots, commit 30c25be71fcbd87fd. We should
> >> be able to treat EPERM as expected now, as the commit message there
> >> indicates.
> >>
> >> Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
> >> ---
> >>
> >> This does introduce a behavior change as well- we no longer exit the range loop
> >> immediately on an error code, instead we continue the loop and attempt to
> >> delete the rest of the checkpoints in the range. Objections to this? I'm not an
> >> expert by any means.
> >>
> >>  bin/rmcp.c |   10 ++++++----
> >>  1 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/bin/rmcp.c b/bin/rmcp.c
> >> index 780a192..3c5c184 100644
> >> --- a/bin/rmcp.c
> >> +++ b/bin/rmcp.c
> >> @@ -105,7 +105,7 @@ static int rmcp_remove_range(struct nilfs *nilfs,
> >>                       nd++;
> >>                       continue;
> >>               }
> >> -             if (errno == EBUSY || errno == EPERM) {
> >> +             if (errno == EBUSY) {
> >
> > OK, let's stop regarding the EPERM error as EBUSY.  For most kernels,
> > it's more important to distinguish real permission errors from the
> > snapshot removal error.
> >
> > We have only recently bumped the version of nilfs-utils to 2.1.x from
> > 2.0.x.  It's a good opportunity for this change.
> >
> >>                       nss++;
> >>                       if (!force) {
> >>                               fprintf(stderr,
> >> @@ -115,14 +115,16 @@ static int rmcp_remove_range(struct nilfs *nilfs,
> >>               } else if (errno == ENOENT) {
> >>                       nocp++;
> >>               } else {
> >> -                     fprintf(stderr, "%s: %s\n", progname, strerror(errno));
> >> +                     fprintf(stderr,
> >> +                             "%s: %llu: cannot remove checkpoint: %s\n",
> >> +                             progname, (unsigned long long)cno,
> >> +                             strerror(errno));
> >>                       ret = -1;
> >
> >> -                     goto out;
> >
> > Could you revise this ?
> >
> > I think the loop should abort immidiately when the rmcp meets an
> > important error even though it will change behavior for the real
> > permission error.
> 
> 1. Please define "important"- is EPERM important? I would tend to say
> the error should be printed once per checkpoint as it is now in this
> case for EPERM, and then restore the prior behavior for other actions.

I meant something critical (e.g. ENOMEM, EIO) or violative (EPERM).

The original EPERM check was just a workaround for early nilfs
implementation, which returns EPERM when trying to remove checkpoints
in snapshot mode.

Now EPERM is returned only if the caller is not allowed to
remove/change checkpoints.

If the caller does not have proper right, every remove operation fails
and it never succeeds partially.  Although the result is predictable,
it may repeat thousands of the same error messages.

As for the critical errors, I think continuing operations is not
undesirable not to worsen the situation.


> 2. I'm not very fond of the error messages with the immediate exit:
> 
>     $ ./bin/rmcp 100..105 115..125
>     lt-rmcp: 100: cannot remove checkpoint: Operation not permitted
>     $ echo $?
>     1
> 
> Did 101-105 succeed? Did the second range succeed? Judging from this
> (lack of) output, I would think it did, which is quite obviously not
> the case.

Right.  It's ambiguous from the current messages.

How about printing an additional message to indicate the range of
unprocessed checkpoints if aborting the loop ?

  104: cannot remove checkpoint: xxxxx
  Checkpoints 104..105 115..125 were not removed.


With regards,
Ryusuke Konishi
 
> So I would vote to not exit early in at least the semi-normal EPERM
> case; perhaps we do for the others. What is the reasoning behind
> exiting early in even the abnormal error cases?
> 
> >>               }
> >>       }
> >>       if (!force && (nss > 0 || (nocp > 0 && nd == 0)))
> >>               ret = 1;
> >> - out:
> >> +
> >>       *ndeleted = nd;
> >>       *nsnapshots = nss;
> >>       return ret;
> >> --
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] rmcp: print sensible error message on permission failure

[Dan McGee]

When running as non-root, the error message seems to indicate a
checkpoint is a snapshot, rather than the actual problem, which is that
I do not have permission to delete the checkpoints.

Before:
    $ rmcp 29..35
    rmcp: 29: cannot remove snapshot
    rmcp: 30: cannot remove snapshot
    rmcp: 31: cannot remove snapshot
    rmcp: 32: cannot remove snapshot
    rmcp: 33: cannot remove snapshot
    rmcp: 34: cannot remove snapshot
    rmcp: 35: cannot remove snapshot

    To delete snapshot(s), change them into checkpoints with
    chcp command before removal.

After:
    $ ./bin/rmcp 29..35
    lt-rmcp: 29: cannot remove checkpoint: Operation not permitted
    Remaining checkpoints were not removed.

Since May 2009 the kernel has returned EBUSY instead of EPERM for
removal requests against snapshots, commit 30c25be71fcbd87fd. We should
be able to treat EPERM as expected now, as the commit message there
indicates.

Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>
---
 bin/rmcp.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/bin/rmcp.c b/bin/rmcp.c
index 780a192..9f4666a 100644
--- a/bin/rmcp.c
+++ b/bin/rmcp.c
@@ -105,7 +105,7 @@ static int rmcp_remove_range(struct nilfs *nilfs,
 			nd++;
 			continue;
 		}
-		if (errno == EBUSY || errno == EPERM) {
+		if (errno == EBUSY) {
 			nss++;
 			if (!force) {
 				fprintf(stderr,
@@ -115,7 +115,10 @@ static int rmcp_remove_range(struct nilfs *nilfs,
 		} else if (errno == ENOENT) {
 			nocp++;
 		} else {
-			fprintf(stderr, "%s: %s\n", progname, strerror(errno));
+			fprintf(stderr,
+				"%s: %llu: cannot remove checkpoint: %s\n",
+				progname, (unsigned long long)cno,
+				strerror(errno));
 			ret = -1;
 			goto out;
 		}
@@ -237,8 +240,10 @@ int main(int argc, char *argv[])
 			continue;
 
 		status = 1;
-		if (ret < 0)
+		if (ret < 0) {
+			fprintf(stderr, "Remaining checkpoints were not removed.\n");
 			break;
+		}
 
 		if (force || ndel != 0 || end - start + 1 - nss == 0)
 			continue;
-- 
1.7.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rmcp: print sensible error message on permission failure

[Ryusuke Konishi]

On Tue,  3 Jan 2012 13:46:00 -0600, Dan McGee wrote:
> When running as non-root, the error message seems to indicate a
> checkpoint is a snapshot, rather than the actual problem, which is that
> I do not have permission to delete the checkpoints.
> 
> Before:
>     $ rmcp 29..35
>     rmcp: 29: cannot remove snapshot
>     rmcp: 30: cannot remove snapshot
>     rmcp: 31: cannot remove snapshot
>     rmcp: 32: cannot remove snapshot
>     rmcp: 33: cannot remove snapshot
>     rmcp: 34: cannot remove snapshot
>     rmcp: 35: cannot remove snapshot
> 
>     To delete snapshot(s), change them into checkpoints with
>     chcp command before removal.
> 
> After:
>     $ ./bin/rmcp 29..35
>     lt-rmcp: 29: cannot remove checkpoint: Operation not permitted
>     Remaining checkpoints were not removed.
> 
> Since May 2009 the kernel has returned EBUSY instead of EPERM for
> removal requests against snapshots, commit 30c25be71fcbd87fd. We should
> be able to treat EPERM as expected now, as the commit message there
> indicates.
> 
> Signed-off-by: Dan McGee <dan-fd97jBR+K/6hPH1hqNUYSQ@public.gmane.org>

All your patches were applied to nilfs-utils git repository.

Thank you very much.

Ryusuke Konishi

> ---
>  bin/rmcp.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/bin/rmcp.c b/bin/rmcp.c
> index 780a192..9f4666a 100644
> --- a/bin/rmcp.c
> +++ b/bin/rmcp.c
> @@ -105,7 +105,7 @@ static int rmcp_remove_range(struct nilfs *nilfs,
>  			nd++;
>  			continue;
>  		}
> -		if (errno == EBUSY || errno == EPERM) {
> +		if (errno == EBUSY) {
>  			nss++;
>  			if (!force) {
>  				fprintf(stderr,
> @@ -115,7 +115,10 @@ static int rmcp_remove_range(struct nilfs *nilfs,
>  		} else if (errno == ENOENT) {
>  			nocp++;
>  		} else {
> -			fprintf(stderr, "%s: %s\n", progname, strerror(errno));
> +			fprintf(stderr,
> +				"%s: %llu: cannot remove checkpoint: %s\n",
> +				progname, (unsigned long long)cno,
> +				strerror(errno));
>  			ret = -1;
>  			goto out;
>  		}
> @@ -237,8 +240,10 @@ int main(int argc, char *argv[])
>  			continue;
>  
>  		status = 1;
> -		if (ret < 0)
> +		if (ret < 0) {
> +			fprintf(stderr, "Remaining checkpoints were not removed.\n");
>  			break;
> +		}
>  
>  		if (force || ndel != 0 || end - start + 1 - nss == 0)
>  			continue;
> -- 
> 1.7.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html