linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] btrfs-progs: misc trivial fixes
@ 2013-07-15  5:35 Anand Jain
  2013-07-15  5:35 ` [PATCH 1/2] btrfs-progs: fix the long lines Anand Jain
  2013-07-15  5:35 ` [PATCH 2/2] btrfs-progs: close_all_devices() declared in multiple files Anand Jain
  0 siblings, 2 replies; 8+ messages in thread
From: Anand Jain @ 2013-07-15  5:35 UTC (permalink / raw)
  To: linux-btrfs


Anand Jain (2):
  btrfs-progs: fix the long lines
  btrfs-progs: close_all_devices() declared in multiple files

 btrfs-find-root.c |   17 +----------------
 cmds-replace.c    |   17 ++++++++++-------
 disk-io.c         |    3 +--
 disk-io.h         |    1 +
 4 files changed, 13 insertions(+), 25 deletions(-)

-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] btrfs-progs: fix the long lines
  2013-07-15  5:35 [PATCH 0/2] btrfs-progs: misc trivial fixes Anand Jain
@ 2013-07-15  5:35 ` Anand Jain
  2013-07-15  7:25   ` Stefan Behrens
  2013-07-15 14:06   ` Eric Sandeen
  2013-07-15  5:35 ` [PATCH 2/2] btrfs-progs: close_all_devices() declared in multiple files Anand Jain
  1 sibling, 2 replies; 8+ messages in thread
From: Anand Jain @ 2013-07-15  5:35 UTC (permalink / raw)
  To: linux-btrfs

trivial: cmds-replace.c contains long lines fix it

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-replace.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/cmds-replace.c b/cmds-replace.c
index 6397bb5..c68986a 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -211,8 +211,8 @@ static int cmd_start_replace(int argc, char **argv)
 		struct btrfs_ioctl_dev_info_args *di_args = NULL;
 
 		if (atoi(srcdev) == 0) {
-			fprintf(stderr, "Error: Failed to parse the numerical devid value '%s'\n",
-				srcdev);
+			fprintf(stderr, "Error: Failed to parse the numerical "\
+				"devid value '%s'\n", srcdev);
 			goto leave_with_error;
 		}
 		start_args.start.srcdevid = (__u64)atoi(srcdev);
@@ -235,8 +235,8 @@ static int cmd_start_replace(int argc, char **argv)
 				break;
 		free(di_args);
 		if (i == fi_args.num_devices) {
-			fprintf(stderr, "Error: '%s' is not a valid devid for filesystem '%s'\n",
-				srcdev, path);
+			fprintf(stderr, "Error: '%s' is not a valid devid for "\
+				"filesystem '%s'\n", srcdev, path);
 			goto leave_with_error;
 		}
 	} else {
@@ -283,7 +283,8 @@ static int cmd_start_replace(int argc, char **argv)
 				    &total_devs, BTRFS_SUPER_INFO_OFFSET);
 	if (ret >= 0 && !force_using_targetdev) {
 		fprintf(stderr,
-			"Error, target device %s contains filesystem, use '-f' to force overwriting.\n",
+			"Error, target device %s contains filesystem, "\
+			"use '-f' to force overwriting.\n",
 			dstdev);
 		goto leave_with_error;
 	}
@@ -321,7 +322,8 @@ static int cmd_start_replace(int argc, char **argv)
 	if (do_not_background) {
 		if (ret) {
 			fprintf(stderr,
-				"ERROR: ioctl(DEV_REPLACE_START) failed on \"%s\": %s, %s\n",
+				"ERROR: ioctl(DEV_REPLACE_START) failed "\
+				"on \"%s\": %s, %s\n",
 				path, strerror(errno),
 				replace_dev_result2string(start_args.result));
 			goto leave_with_error;
@@ -330,7 +332,8 @@ static int cmd_start_replace(int argc, char **argv)
 		if (start_args.result !=
 		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
 			fprintf(stderr,
-				"ERROR: ioctl(DEV_REPLACE_START) on \"%s\" returns error: %s\n",
+				"ERROR: ioctl(DEV_REPLACE_START) on \"%s\" "\
+				"returns error: %s\n",
 				path,
 				replace_dev_result2string(start_args.result));
 			goto leave_with_error;
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] btrfs-progs: close_all_devices() declared in multiple files
  2013-07-15  5:35 [PATCH 0/2] btrfs-progs: misc trivial fixes Anand Jain
  2013-07-15  5:35 ` [PATCH 1/2] btrfs-progs: fix the long lines Anand Jain
@ 2013-07-15  5:35 ` Anand Jain
  2013-07-15  8:06   ` Stefan Behrens
  2013-07-15  8:40   ` [PATCH] btrfs-progs: close_all_devices() in btrfs-find-root.c does nothing Anand Jain
  1 sibling, 2 replies; 8+ messages in thread
From: Anand Jain @ 2013-07-15  5:35 UTC (permalink / raw)
  To: linux-btrfs

close_all_devices() is declared once in disk-io.c and again
in btrfs-find-root.c I see no reasons or is there any ?

This patch will fix it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 btrfs-find-root.c |   17 +----------------
 disk-io.c         |    3 +--
 disk-io.h         |    1 +
 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/btrfs-find-root.c b/btrfs-find-root.c
index 253c200..82440f6 100644
--- a/btrfs-find-root.c
+++ b/btrfs-find-root.c
@@ -34,6 +34,7 @@
 #include "volumes.h"
 #include "utils.h"
 #include "crc32c.h"
+#include "disk-io.h"
 
 static u16 csum_size = 0;
 static u64 search_objectid = BTRFS_ROOT_TREE_OBJECTID;
@@ -65,22 +66,6 @@ int csum_block(void *buf, u32 len)
 	return ret;
 }
 
-static int close_all_devices(struct btrfs_fs_info *fs_info)
-{
-	struct list_head *list;
-	struct list_head *next;
-	struct btrfs_device *device;
-
-	return 0;
-
-	list = &fs_info->fs_devices->devices;
-	list_for_each(next, list) {
-		device = list_entry(next, struct btrfs_device, dev_list);
-		close(device->fd);
-	}
-	return 0;
-}
-
 static struct btrfs_root *open_ctree_broken(int fd, const char *device)
 {
 	u32 sectorsize;
diff --git a/disk-io.c b/disk-io.c
index 5b84b02..d23c4d9 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -35,7 +35,6 @@
 #include "utils.h"
 #include "print-tree.h"
 
-static int close_all_devices(struct btrfs_fs_info *fs_info);
 
 static int check_tree_block(struct btrfs_root *root, struct extent_buffer *buf)
 {
@@ -1165,7 +1164,7 @@ int write_ctree_super(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static int close_all_devices(struct btrfs_fs_info *fs_info)
+int close_all_devices(struct btrfs_fs_info *fs_info)
 {
 	struct list_head *list;
 	struct btrfs_device *device;
diff --git a/disk-io.h b/disk-io.h
index 0158d17..c25bc07 100644
--- a/disk-io.h
+++ b/disk-io.h
@@ -89,6 +89,7 @@ int csum_tree_block_size(struct extent_buffer *buf, u16 csum_sectorsize,
 int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 		    int verify);
 int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid);
+int close_all_devices(struct btrfs_fs_info *fs_info);
 #endif
 
 /* raid6.c */
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] btrfs-progs: fix the long lines
  2013-07-15  5:35 ` [PATCH 1/2] btrfs-progs: fix the long lines Anand Jain
@ 2013-07-15  7:25   ` Stefan Behrens
  2013-07-15 14:06   ` Eric Sandeen
  1 sibling, 0 replies; 8+ messages in thread
From: Stefan Behrens @ 2013-07-15  7:25 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On Mon, 15 Jul 2013 13:35:26 +0800, Anand Jain wrote:
> trivial: cmds-replace.c contains long lines fix it
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  cmds-replace.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/cmds-replace.c b/cmds-replace.c
> index 6397bb5..c68986a 100644
> --- a/cmds-replace.c
> +++ b/cmds-replace.c
> @@ -211,8 +211,8 @@ static int cmd_start_replace(int argc, char **argv)
>  		struct btrfs_ioctl_dev_info_args *di_args = NULL;
>  
>  		if (atoi(srcdev) == 0) {
> -			fprintf(stderr, "Error: Failed to parse the numerical devid value '%s'\n",
> -				srcdev);
> +			fprintf(stderr, "Error: Failed to parse the numerical "\
> +				"devid value '%s'\n", srcdev);
>  			goto leave_with_error;
>  		}
>  		start_args.start.srcdevid = (__u64)atoi(srcdev);
> @@ -235,8 +235,8 @@ static int cmd_start_replace(int argc, char **argv)
>  				break;
>  		free(di_args);
>  		if (i == fi_args.num_devices) {
> -			fprintf(stderr, "Error: '%s' is not a valid devid for filesystem '%s'\n",
> -				srcdev, path);
> +			fprintf(stderr, "Error: '%s' is not a valid devid for "\
> +				"filesystem '%s'\n", srcdev, path);
>  			goto leave_with_error;
>  		}
>  	} else {
> @@ -283,7 +283,8 @@ static int cmd_start_replace(int argc, char **argv)
>  				    &total_devs, BTRFS_SUPER_INFO_OFFSET);
>  	if (ret >= 0 && !force_using_targetdev) {
>  		fprintf(stderr,
> -			"Error, target device %s contains filesystem, use '-f' to force overwriting.\n",
> +			"Error, target device %s contains filesystem, "\
> +			"use '-f' to force overwriting.\n",
>  			dstdev);
>  		goto leave_with_error;
>  	}
> @@ -321,7 +322,8 @@ static int cmd_start_replace(int argc, char **argv)
>  	if (do_not_background) {
>  		if (ret) {
>  			fprintf(stderr,
> -				"ERROR: ioctl(DEV_REPLACE_START) failed on \"%s\": %s, %s\n",
> +				"ERROR: ioctl(DEV_REPLACE_START) failed "\
> +				"on \"%s\": %s, %s\n",
>  				path, strerror(errno),
>  				replace_dev_result2string(start_args.result));
>  			goto leave_with_error;
> @@ -330,7 +332,8 @@ static int cmd_start_replace(int argc, char **argv)
>  		if (start_args.result !=
>  		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
>  			fprintf(stderr,
> -				"ERROR: ioctl(DEV_REPLACE_START) on \"%s\" returns error: %s\n",
> +				"ERROR: ioctl(DEV_REPLACE_START) on \"%s\" "\
> +				"returns error: %s\n",
>  				path,
>  				replace_dev_result2string(start_args.result));
>  			goto leave_with_error;
> 

Btrfs-progs follows the Linux kernel coding style. Refer to
Documentation/CodingStyle: "never break user-visible strings such as
printk messages, because that breaks the ability to grep for them".


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] btrfs-progs: close_all_devices() declared in multiple files
  2013-07-15  5:35 ` [PATCH 2/2] btrfs-progs: close_all_devices() declared in multiple files Anand Jain
@ 2013-07-15  8:06   ` Stefan Behrens
  2013-07-15  8:40   ` [PATCH] btrfs-progs: close_all_devices() in btrfs-find-root.c does nothing Anand Jain
  1 sibling, 0 replies; 8+ messages in thread
From: Stefan Behrens @ 2013-07-15  8:06 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On Mon, 15 Jul 2013 13:35:27 +0800, Anand Jain wrote:
> close_all_devices() is declared once in disk-io.c and again
> in btrfs-find-root.c I see no reasons or is there any ?
> 
> This patch will fix it.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  btrfs-find-root.c |   17 +----------------
>  disk-io.c         |    3 +--
>  disk-io.h         |    1 +
>  3 files changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/btrfs-find-root.c b/btrfs-find-root.c
> index 253c200..82440f6 100644
> --- a/btrfs-find-root.c
> +++ b/btrfs-find-root.c
> @@ -34,6 +34,7 @@
>  #include "volumes.h"
>  #include "utils.h"
>  #include "crc32c.h"
> +#include "disk-io.h"
>  
>  static u16 csum_size = 0;
>  static u64 search_objectid = BTRFS_ROOT_TREE_OBJECTID;
> @@ -65,22 +66,6 @@ int csum_block(void *buf, u32 len)
>  	return ret;
>  }
>  
> -static int close_all_devices(struct btrfs_fs_info *fs_info)
> -{
> -	struct list_head *list;
> -	struct list_head *next;
> -	struct btrfs_device *device;
> -
> -	return 0;

Nice and useful function :)

This function is not equal to the one from disk-io.c, only the function
name is equal. But looking at the code, this patch should work. Maybe
the commit message could be improved since it reads as if it would
handle two equal functions and this is not the case.


> -
> -	list = &fs_info->fs_devices->devices;
> -	list_for_each(next, list) {
> -		device = list_entry(next, struct btrfs_device, dev_list);
> -		close(device->fd);
> -	}
> -	return 0;
> -}
> -
>  static struct btrfs_root *open_ctree_broken(int fd, const char *device)
>  {
>  	u32 sectorsize;
> diff --git a/disk-io.c b/disk-io.c
> index 5b84b02..d23c4d9 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -35,7 +35,6 @@
>  #include "utils.h"
>  #include "print-tree.h"
>  
> -static int close_all_devices(struct btrfs_fs_info *fs_info);
>  
>  static int check_tree_block(struct btrfs_root *root, struct extent_buffer *buf)
>  {
> @@ -1165,7 +1164,7 @@ int write_ctree_super(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -static int close_all_devices(struct btrfs_fs_info *fs_info)
> +int close_all_devices(struct btrfs_fs_info *fs_info)
>  {
>  	struct list_head *list;
>  	struct btrfs_device *device;
> diff --git a/disk-io.h b/disk-io.h
> index 0158d17..c25bc07 100644
> --- a/disk-io.h
> +++ b/disk-io.h
> @@ -89,6 +89,7 @@ int csum_tree_block_size(struct extent_buffer *buf, u16 csum_sectorsize,
>  int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
>  		    int verify);
>  int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid);
> +int close_all_devices(struct btrfs_fs_info *fs_info);
>  #endif
>  
>  /* raid6.c */
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] btrfs-progs: close_all_devices() in btrfs-find-root.c does nothing
  2013-07-15  5:35 ` [PATCH 2/2] btrfs-progs: close_all_devices() declared in multiple files Anand Jain
  2013-07-15  8:06   ` Stefan Behrens
@ 2013-07-15  8:40   ` Anand Jain
  1 sibling, 0 replies; 8+ messages in thread
From: Anand Jain @ 2013-07-15  8:40 UTC (permalink / raw)
  To: linux-btrfs

close_all_devices() is declared once in disk-io.c and again
in btrfs-find-root.c. The one in latter is completely useless
so delete it.

This patch will fix it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 btrfs-find-root.c |   17 +----------------
 disk-io.c         |    3 +--
 disk-io.h         |    1 +
 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/btrfs-find-root.c b/btrfs-find-root.c
index 253c200..82440f6 100644
--- a/btrfs-find-root.c
+++ b/btrfs-find-root.c
@@ -34,6 +34,7 @@
 #include "volumes.h"
 #include "utils.h"
 #include "crc32c.h"
+#include "disk-io.h"
 
 static u16 csum_size = 0;
 static u64 search_objectid = BTRFS_ROOT_TREE_OBJECTID;
@@ -65,22 +66,6 @@ int csum_block(void *buf, u32 len)
 	return ret;
 }
 
-static int close_all_devices(struct btrfs_fs_info *fs_info)
-{
-	struct list_head *list;
-	struct list_head *next;
-	struct btrfs_device *device;
-
-	return 0;
-
-	list = &fs_info->fs_devices->devices;
-	list_for_each(next, list) {
-		device = list_entry(next, struct btrfs_device, dev_list);
-		close(device->fd);
-	}
-	return 0;
-}
-
 static struct btrfs_root *open_ctree_broken(int fd, const char *device)
 {
 	u32 sectorsize;
diff --git a/disk-io.c b/disk-io.c
index 5b84b02..d23c4d9 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -35,7 +35,6 @@
 #include "utils.h"
 #include "print-tree.h"
 
-static int close_all_devices(struct btrfs_fs_info *fs_info);
 
 static int check_tree_block(struct btrfs_root *root, struct extent_buffer *buf)
 {
@@ -1165,7 +1164,7 @@ int write_ctree_super(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static int close_all_devices(struct btrfs_fs_info *fs_info)
+int close_all_devices(struct btrfs_fs_info *fs_info)
 {
 	struct list_head *list;
 	struct btrfs_device *device;
diff --git a/disk-io.h b/disk-io.h
index 0158d17..c25bc07 100644
--- a/disk-io.h
+++ b/disk-io.h
@@ -89,6 +89,7 @@ int csum_tree_block_size(struct extent_buffer *buf, u16 csum_sectorsize,
 int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 		    int verify);
 int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid);
+int close_all_devices(struct btrfs_fs_info *fs_info);
 #endif
 
 /* raid6.c */
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] btrfs-progs: fix the long lines
  2013-07-15  5:35 ` [PATCH 1/2] btrfs-progs: fix the long lines Anand Jain
  2013-07-15  7:25   ` Stefan Behrens
@ 2013-07-15 14:06   ` Eric Sandeen
  2013-07-17  2:46     ` Anand Jain
  1 sibling, 1 reply; 8+ messages in thread
From: Eric Sandeen @ 2013-07-15 14:06 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On 7/15/13 12:35 AM, Anand Jain wrote:
> trivial: cmds-replace.c contains long lines fix it

I realize that this is total bikeshedding, so you can take or leave it, but:

One downside to this is that it makes it a little harder to grep for strings
when they get arbitrarily split across lines.

One thing XFS userspace did was to out-dent in cases like this, i.e.:

> --- a/cmds-replace.c
> +++ b/cmds-replace.c
> @@ -211,8 +211,8 @@ static int cmd_start_replace(int argc, char **argv)
>  		struct btrfs_ioctl_dev_info_args *di_args = NULL;
>  
>  		if (atoi(srcdev) == 0) {
> -			fprintf(stderr, "Error: Failed to parse the numerical devid value '%s'\n",
> -				srcdev);
> +			fprintf(stderr,
> +	"Error: Failed to parse the numerical devid value '%s'\n",
> +				srcdev);
>  			goto leave_with_error;
>  		}
>  		start_args.start.srcdevid = (__u64)atoi(srcdev);

so that the strings remain more easily searchable.  Just a thought; maybe folks hate that idea but I thought I'd throw it out there.

But as a more topical review:

> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  cmds-replace.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/cmds-replace.c b/cmds-replace.c
> index 6397bb5..c68986a 100644
> --- a/cmds-replace.c
> +++ b/cmds-replace.c
> @@ -211,8 +211,8 @@ static int cmd_start_replace(int argc, char **argv)
>  		struct btrfs_ioctl_dev_info_args *di_args = NULL;
>  
>  		if (atoi(srcdev) == 0) {
> -			fprintf(stderr, "Error: Failed to parse the numerical devid value '%s'\n",
> -				srcdev);
> +			fprintf(stderr, "Error: Failed to parse the numerical "\
                                                                               ^
There's no need for the line continuation, this isn't a macro.  ;)

To split the line this way just do:

> +			fprintf(stderr, "Error: Failed to parse the numerical "
> +				"devid value '%s'\n", srcdev);

-Eric

>  			goto leave_with_error;
>  		}
>  		start_args.start.srcdevid = (__u64)atoi(srcdev);
> @@ -235,8 +235,8 @@ static int cmd_start_replace(int argc, char **argv)
>  				break;
>  		free(di_args);
>  		if (i == fi_args.num_devices) {
> -			fprintf(stderr, "Error: '%s' is not a valid devid for filesystem '%s'\n",
> -				srcdev, path);
> +			fprintf(stderr, "Error: '%s' is not a valid devid for "\
> +				"filesystem '%s'\n", srcdev, path);
>  			goto leave_with_error;
>  		}
>  	} else {
> @@ -283,7 +283,8 @@ static int cmd_start_replace(int argc, char **argv)
>  				    &total_devs, BTRFS_SUPER_INFO_OFFSET);
>  	if (ret >= 0 && !force_using_targetdev) {
>  		fprintf(stderr,
> -			"Error, target device %s contains filesystem, use '-f' to force overwriting.\n",
> +			"Error, target device %s contains filesystem, "\
> +			"use '-f' to force overwriting.\n",
>  			dstdev);
>  		goto leave_with_error;
>  	}
> @@ -321,7 +322,8 @@ static int cmd_start_replace(int argc, char **argv)
>  	if (do_not_background) {
>  		if (ret) {
>  			fprintf(stderr,
> -				"ERROR: ioctl(DEV_REPLACE_START) failed on \"%s\": %s, %s\n",
> +				"ERROR: ioctl(DEV_REPLACE_START) failed "\
> +				"on \"%s\": %s, %s\n",
>  				path, strerror(errno),
>  				replace_dev_result2string(start_args.result));
>  			goto leave_with_error;
> @@ -330,7 +332,8 @@ static int cmd_start_replace(int argc, char **argv)
>  		if (start_args.result !=
>  		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
>  			fprintf(stderr,
> -				"ERROR: ioctl(DEV_REPLACE_START) on \"%s\" returns error: %s\n",
> +				"ERROR: ioctl(DEV_REPLACE_START) on \"%s\" "\
> +				"returns error: %s\n",
>  				path,
>  				replace_dev_result2string(start_args.result));
>  			goto leave_with_error;
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] btrfs-progs: fix the long lines
  2013-07-15 14:06   ` Eric Sandeen
@ 2013-07-17  2:46     ` Anand Jain
  0 siblings, 0 replies; 8+ messages in thread
From: Anand Jain @ 2013-07-17  2:46 UTC (permalink / raw)
  To: linux-btrfs



  Strings being grep-able is important. Thanks Stefan and Eric
  for the comments.

  Hopefully we shall have some better ways to handle long strings.

  OR

  shorter error message and still communicate the
  intended message is another choice but challenging. :-)

  I have dropped this patch for now.

Thanks Anand


On 07/15/2013 10:06 PM, Eric Sandeen wrote:
> On 7/15/13 12:35 AM, Anand Jain wrote:
>> trivial: cmds-replace.c contains long lines fix it
>
> I realize that this is total bikeshedding, so you can take or leave it, but:
>
> One downside to this is that it makes it a little harder to grep for strings
> when they get arbitrarily split across lines.
>
> One thing XFS userspace did was to out-dent in cases like this, i.e.:
>
>> --- a/cmds-replace.c
>> +++ b/cmds-replace.c
>> @@ -211,8 +211,8 @@ static int cmd_start_replace(int argc, char **argv)
>>   		struct btrfs_ioctl_dev_info_args *di_args = NULL;
>>
>>   		if (atoi(srcdev) == 0) {
>> -			fprintf(stderr, "Error: Failed to parse the numerical devid value '%s'\n",
>> -				srcdev);
>> +			fprintf(stderr,
>> +	"Error: Failed to parse the numerical devid value '%s'\n",
>> +				srcdev);
>>   			goto leave_with_error;
>>   		}
>>   		start_args.start.srcdevid = (__u64)atoi(srcdev);
>
> so that the strings remain more easily searchable.  Just a thought; maybe folks hate that idea but I thought I'd throw it out there.
>
> But as a more topical review:
>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   cmds-replace.c |   17 ++++++++++-------
>>   1 files changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/cmds-replace.c b/cmds-replace.c
>> index 6397bb5..c68986a 100644
>> --- a/cmds-replace.c
>> +++ b/cmds-replace.c
>> @@ -211,8 +211,8 @@ static int cmd_start_replace(int argc, char **argv)
>>   		struct btrfs_ioctl_dev_info_args *di_args = NULL;
>>
>>   		if (atoi(srcdev) == 0) {
>> -			fprintf(stderr, "Error: Failed to parse the numerical devid value '%s'\n",
>> -				srcdev);
>> +			fprintf(stderr, "Error: Failed to parse the numerical "\
>                                                                                 ^
> There's no need for the line continuation, this isn't a macro.  ;)
>
> To split the line this way just do:
>
>> +			fprintf(stderr, "Error: Failed to parse the numerical "
>> +				"devid value '%s'\n", srcdev);
>
> -Eric
>
>>   			goto leave_with_error;
>>   		}
>>   		start_args.start.srcdevid = (__u64)atoi(srcdev);
>> @@ -235,8 +235,8 @@ static int cmd_start_replace(int argc, char **argv)
>>   				break;
>>   		free(di_args);
>>   		if (i == fi_args.num_devices) {
>> -			fprintf(stderr, "Error: '%s' is not a valid devid for filesystem '%s'\n",
>> -				srcdev, path);
>> +			fprintf(stderr, "Error: '%s' is not a valid devid for "\
>> +				"filesystem '%s'\n", srcdev, path);
>>   			goto leave_with_error;
>>   		}
>>   	} else {
>> @@ -283,7 +283,8 @@ static int cmd_start_replace(int argc, char **argv)
>>   				    &total_devs, BTRFS_SUPER_INFO_OFFSET);
>>   	if (ret >= 0 && !force_using_targetdev) {
>>   		fprintf(stderr,
>> -			"Error, target device %s contains filesystem, use '-f' to force overwriting.\n",
>> +			"Error, target device %s contains filesystem, "\
>> +			"use '-f' to force overwriting.\n",
>>   			dstdev);
>>   		goto leave_with_error;
>>   	}
>> @@ -321,7 +322,8 @@ static int cmd_start_replace(int argc, char **argv)
>>   	if (do_not_background) {
>>   		if (ret) {
>>   			fprintf(stderr,
>> -				"ERROR: ioctl(DEV_REPLACE_START) failed on \"%s\": %s, %s\n",
>> +				"ERROR: ioctl(DEV_REPLACE_START) failed "\
>> +				"on \"%s\": %s, %s\n",
>>   				path, strerror(errno),
>>   				replace_dev_result2string(start_args.result));
>>   			goto leave_with_error;
>> @@ -330,7 +332,8 @@ static int cmd_start_replace(int argc, char **argv)
>>   		if (start_args.result !=
>>   		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
>>   			fprintf(stderr,
>> -				"ERROR: ioctl(DEV_REPLACE_START) on \"%s\" returns error: %s\n",
>> +				"ERROR: ioctl(DEV_REPLACE_START) on \"%s\" "\
>> +				"returns error: %s\n",
>>   				path,
>>   				replace_dev_result2string(start_args.result));
>>   			goto leave_with_error;
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-07-17  2:41 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-15  5:35 [PATCH 0/2] btrfs-progs: misc trivial fixes Anand Jain
2013-07-15  5:35 ` [PATCH 1/2] btrfs-progs: fix the long lines Anand Jain
2013-07-15  7:25   ` Stefan Behrens
2013-07-15 14:06   ` Eric Sandeen
2013-07-17  2:46     ` Anand Jain
2013-07-15  5:35 ` [PATCH 2/2] btrfs-progs: close_all_devices() declared in multiple files Anand Jain
2013-07-15  8:06   ` Stefan Behrens
2013-07-15  8:40   ` [PATCH] btrfs-progs: close_all_devices() in btrfs-find-root.c does nothing Anand Jain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).