A handful of small cleanups and improvements

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/[email protected]>

---

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

include/Makefile.am +1 -1

index 221c481..c99da99 100644
@@ -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

include/kern_compat.h deleted +0 -47

deleted file mode 100644
index a2cffdc..0000000
@@ -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 */

lib/feature.c +1 -1

index b346cea..b5752f2 100644
@@ -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"
 

sbin/mkfs/mkfs.c +2 -1

index 43a9657..870c682 100644
@@ -72,8 +72,9 @@
 
 #include <errno.h>
 
-#include "mkfs.h"
+#include "nilfs.h"
 #include "nilfs_feature.h"
+#include "mkfs.h"
 
 
 typedef __u64  blocknr_t;

sbin/mkfs/mkfs.h +0 -4

index 643060c..ce065a0 100644
@@ -24,7 +24,6 @@
  * Revised by Ryusuke Konishi <[email protected]>.
  */
 
-#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

Signature · 6 lines

1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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/[email protected]>

---

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(-)

bin/rmcp.c +0 -0

index 780a192..3c5c184 100644

bin/rmcp.c +0 -0

 			nss++;
 			if (!force) {
 				fprintf(stderr,

@@ -115,14 +115,16 @@ static int rmcp_remove_range(struct nilfs *nilfs,
 		} else if (errno == ENOENT) {

bin/rmcp.c +1 -1

 		}
 	}
 	if (!force && (nss > 0 || (nocp > 0 && nd == 0)))
 		ret = 1;
- out:
+
 	*ndeleted = nd;
 	*nsnapshots = nss;

return ret;

Signature · 6 lines

1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 48 lines · 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/[email protected]>

---

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(-)

bin/rmcp.c +0 -0

index 780a192..3c5c184 100644

bin/rmcp.c static int rmcp_remove_range(struct nilfs *nilfs, · line 108

Quoted 5 lines

bin/rmcp.c +1 -1

@@ -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.

Quoted 13 lines

bin/rmcp.c +4 -1

                       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;

bin/rmcp.c static int rmcp_remove_range(struct nilfs *nilfs, · line 120

Quoted 1 line

bin/rmcp.c +0 -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.

bin/rmcp.c static int rmcp_remove_range(struct nilfs *nilfs,

Quoted 10 lines

bin/rmcp.c +1 -2

               }
       }
       if (!force && (nss > 0 || (nocp > 0 && nd == 0)))
               ret = 1;
- out:
+
       *ndeleted = nd;
       *nsnapshots = nss;
       return ret;
--

Quoted 80 lines · Ryusuke Konishi

Quoted 48 lines · 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/[email protected]>

---

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(-)

bin/rmcp.c +0 -0

index 780a192..3c5c184 100644

bin/rmcp.c static int rmcp_remove_range(struct nilfs *nilfs, · line 108

Quoted 5 lines

bin/rmcp.c +1 -1

@@ -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.

Quoted 5 lines

bin/rmcp.c +0 -0

                       nss++;
                       if (!force) {
                               fprintf(stderr,

@@ -115,14 +115,16 @@ static int rmcp_remove_range(struct nilfs *nilfs,
               } else if (errno == ENOENT) {

bin/rmcp.c static int rmcp_remove_range(struct nilfs *nilfs, · line 121

Quoted 10 lines

bin/rmcp.c +4 -2

                       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 ?

Quoted 3 lines

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?

Quoted 10 lines

Quoted 10 lines

bin/rmcp.c +1 -1

               }
       }
       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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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/[email protected]>

---
 bin/rmcp.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

bin/rmcp.c +8 -3

index 780a192..9f4666a 100644
@@ -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;

Signature · 6 lines

1.7.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Signature · 6 lines

1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi,

Quoted 3 lines · 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.

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

Regards,
Ryusuke Konishi

Quoted 43 lines

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

Signature · 1 line

1.7.8

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html