btrfs-progs: Introduce warning and error for common use
v2 cover btrfs-progs: Introduce warning and error for common use
Current code use fprintf(stderr, "...") to output warnning and error information.
The error message have different style, as:
# grep fprintf *.c fprintf(stderr, "Open ctree failed\n"); fprintf(stderr, "%s: open ctree failed\n", __func__); fprintf(stderr, "ERROR: cannot open ctree\n"); ...
And sometimes, we forgot add tailed '\n', or use printf instead, as in current code:
printf("warning, device %llu is missing\n",
This patch introduce warning() and error() as common function, to make:
1: Each warning and error information have same format
2: Easy to search/change all error message
3: Easy to modify function's internal for debug or other requirement,
for example:
print function/linenumber in error()
dumpstack in error()
add some trace for some style of message
add support for -v, -vv, ...
support for locales
custom output functions
support some special device/tty
Converting all source is a big work, this patch convert cmds-scrub.c We'll convert others these days, and new code can use these function directly.
Changelog v1->v2:
1: Rename following functions:
warningon() -> warning_on()
erroron() -> error_on()
Suggested-by: David Sterba <[email protected]>2: Use static inline instead of macro
3: Add return value of warning/error_on()
Zhao Lei (2):
btrfs-progs: Introduce warning and error for common use
btrfs-progs: use common warning/error for cmds-scrub.c
cmds-scrub.c | 171 +++++++++++++++++++++++++++++------------------------------ utils.h | 55 +++++++++++++++++++ 2 files changed, 140 insertions(+), 86 deletions(-)
Signature · 1 line
1.8.5.1
PATCH v2 1/2 btrfs-progs: Introduce warning and error for common use
Current code use fprintf(stderr, "...") to output warnning and error information.
The error message have different style, as:
# grep fprintf *.c fprintf(stderr, "Open ctree failed\n"); fprintf(stderr, "%s: open ctree failed\n", __func__); fprintf(stderr, "ERROR: cannot open ctree\n"); ...
And sometimes, we forgot add tailed '\n', or use printf instead, as in current code:
printf("warning, device %llu is missing\n",
This patch introduce warning() and error() as common function, to make:
1: Each warning and error information have same format
2: Easy to search/change all error message
3: Easy to modify function's internal for debug or other requirement,
for example:
print function/linenumber in error()
dumpstack in error()
add some trace for some style of message
add support for -v, -vv, ...
support for locales
custom output functions
support some special device/tty
--- utils.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
utils.h +55 -0
@@ -22,6 +22,7 @@#include <sys/stat.h> #include "ctree.h" #include <dirent.h> +#include <stdarg.h> #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024)#define BTRFS_MKFS_SMALL_VOLUME_SIZE (1024 * 1024 * 1024)
@@ -269,4 +270,58 @@ const char *get_argv0_buf(void);unsigned int get_unit_mode_from_arg (int *argc,char *argv[],int df_mode); +static inline void __veprintf (const char *prefix,const char *format, +va_list ap) +{ +if (prefix) +fprintf (stderr,"%s" , prefix); +vfprintf (stderr, format, ap); +} + +static inline void warning (const char *fmt, ...) +{ +va_list args; + +va_start (args, fmt); +__veprintf ("WARNING: " , fmt, args); +va_end (args); +} + +static inline void error (const char *fmt, ...) +{ +va_list args; + +va_start (args, fmt); +__veprintf ("ERROR: " , fmt, args); +va_end (args); +} + +static inline int warning_on (int condition,const char *fmt, ...) +{ +if (!condition) +return 0 ; + +va_list args; + +va_start (args, fmt); +__veprintf ("WARNING: " , fmt, args); +va_end (args); + +return 1 ; +} + +static inline int error_on (int condition,const char *fmt, ...) +{ +if (!condition) +return 0 ; + +va_list args; + +va_start (args, fmt); +__veprintf ("ERROR: " , fmt, args); +va_end (args); + +return 1 ; +} +#endif
Signature · 1 line
1.8.5.1
utils.h const char *get_argv0_buf(void); · line 278
Quoted 6 lines · Zhao Lei
utils.h +6
-0+static inline void __veprintf (const char *prefix,const char *format, +va_list ap) +{ +if (prefix) +fprintf (stderr,"%s" , prefix); +vfprintf (stderr, format, ap);
I'm not sure we need this helper. All it does it prints a fixed string, we can simply add fputs("prefix", stderr) into warning/error functions.
utils.h const char *get_argv0_buf(void); · line 304
Quoted 6 lines
utils.h +6
-0+static inline int warning_on (int condition,const char *fmt, ...) +{ +if (!condition) +return 0 ; + +va_list args;
Please do not put declaration(s) after statements.
utils.h const char *get_argv0_buf(void); · line 318
Quoted 6 lines
utils.h +6
-0+static inline int error_on (int condition,const char *fmt, ...) +{ +if (!condition) +return 0 ; + +va_list args;
dtto
Quoted 7 lines
utils.h +7
-0+ +va_start (args, fmt); +__veprintf ("ERROR: " , fmt, args); +va_end (args); + +return 1 ; +}
Hi, David Sterba
Thanks for review.
Quoted 9 lines
-----Original Message-----
From: David Sterba [mailto:[email protected]]
Sent: Friday, September 25, 2015 6:50 PM
To: Zhao Lei <[email protected]>
Cc: [email protected]; Qu Wenruo <[email protected]>
Subject: Re: [PATCH v2 1/2] btrfs-progs: Introduce warning and error for common useOn Wed, Sep 16, 2015 at 05:40:46PM +0800, Zhao Lei wrote:
Quoted 10 lines
utils.h const char *get_argv0_buf(void); · line 278
Quoted 6 lines
utils.h +6
-0+static inline void __veprintf (const char *prefix,const char *format, +va_list ap) +{ +if (prefix) +fprintf (stderr,"%s" , prefix); +vfprintf (stderr, format, ap);I'm not sure we need this helper. All it does it prints a fixed string, we can simply add
fputs("prefix", stderr)into warning/error functions.
This wrapper is to support locale in future:
Make all message output by one function, then we can put locale code into it simply.
But now it is hard to introduce locale, because user-land tools changes output frequently, we need to wait the output about fixed.
So I removed __veprintf() in v3, we can modify it when we need.
Quoted 8 lines
utils.h const char *get_argv0_buf(void); · line 304
Quoted 5 lines
utils.h +5
-0+static inline int warning_on (int condition,const char *fmt, ...) { +if (!condition) +return 0 ; + +va_list args;Please do not put declaration(s) after statements.
Fixed in v3.
Quoted 8 lines
utils.h const char *get_argv0_buf(void); · line 304
Quoted 5 lines
utils.h +5
-0+static inline int error_on (int condition,const char *fmt, ...) { +if (!condition) +return 0 ; + +va_list args;dtto
Fixed in v3.
Thanks
Zhaolei
Quoted 7 lines
Quoted 7 lines
utils.h +7
-0+ +va_start (args, fmt); +__veprintf ("ERROR: " , fmt, args); +va_end (args); + +return 1 ; +}
PATCH v2 2/2 btrfs-progs: use common warning/error for cmds-scrub.c
In reply to Zhao LeiUse common warning/error functions in cmds-scrub.c, it can make message format unified and make code simple.
--- cmds-scrub.c | 171 +++++++++++++++++++++++++++++------------------------------ 1 file changed, 85 insertions(+), 86 deletions(-)
cmds-scrub.c +85 -86
@@ -128,11 +128,6 @@ static void print_scrub_full(struct btrfs_scrub_progress *sp)printf ("\tlast_physical: %lld\n" , sp->last_physical); } -#define ERR (test, ...) do { \ - if (test) \ - fprintf(stderr, __VA_ARGS__); \ -} while (0) -#define PRINT_SCRUB_ERROR (test, desc) do { \ if (test) \ printf(" %s=%llu", desc, test); \
@@ -463,7 +458,7 @@ static int scrub_kvread(int *i, int len, int avail, const char *buf,#define _SCRUB_INVALID do { \ if (report_errors) \ - fprintf(stderr, "WARNING: invalid data in line %d pos " \ + warning("invalid data in line %d pos " \ "%d state %d (near \"%.*s\") at %s:%d\n", \ lineno, i, state, 20 > avail ? avail : 20, \ l + i, __FILE__, __LINE__); \
@@ -850,8 +845,7 @@ static void *scrub_one_dev(void *ctx)IOPRIO_PRIO_VALUE (sp->ioprio_class, sp->ioprio_classdata));if (ret) -fprintf (stderr, -"WARNING: setting ioprio failed: %s (ignored).\n" , +warning ("setting ioprio failed: %s (ignored).\n" ,strerror (errno)); ret =ioctl (sp->fd,BTRFS_IOC_SCRUB , &sp->scrub_args);
@@ -1197,9 +1191,9 @@ static int scrub_start(int argc, char **argv, int resume) do_print =0 ;if (mkdir_p (datafile)) { -ERR (!do_quiet,"WARNING: cannot create scrub data " -"file, mkdir %s failed: %s. Status recording " -"disabled\n" , datafile,strerror (errno)); +warning_on (!do_quiet, +"cannot create scrub data file, mkdir %s failed: %s. Status recording disabled\n" , + datafile,strerror (errno)); do_record =0 ; }free (datafile);
@@ -1210,24 +1204,27 @@ static int scrub_start(int argc, char **argv, int resume)if (fdmnt <0 ) {if (errno ==EINVAL ) -ERR (!do_quiet, -"ERROR: '%s' is not a mounted btrfs device\n" , - path); +error_on (!do_quiet, +"'%s' is not a mounted btrfs device\n" , + path);else -ERR (!do_quiet,"ERROR: can't access '%s': %s\n" , - path,strerror (errno)); +error_on (!do_quiet, +"can't access '%s': %s\n" , + path,strerror (errno));return 1 ; } ret =get_fs_info (path, &fi_args, &di_args);if (ret) { -ERR (!do_quiet,"ERROR: getting dev info for scrub failed: " -"%s\n" ,strerror (-ret)); +error_on (!do_quiet, +"getting dev info for scrub failed: %s\n" , +strerror (-ret)); err =1 ; goto out; }if (!fi_args.num_devices) { -ERR (!do_quiet,"ERROR: no devices found\n" ); +error_on (!do_quiet, +"no devices found\n" ); err =1 ; goto out; }
@@ -1235,13 +1232,15 @@ static int scrub_start(int argc, char **argv, int resume)uuid_unparse (fi_args.fsid, fsid); fdres =scrub_open_file_r (SCRUB_DATA_FILE , fsid);if (fdres <0 && fdres != -ENOENT ) { -ERR (!do_quiet,"WARNING: failed to open status file: " -"%s\n" ,strerror (-fdres)); +warning_on (!do_quiet, +"failed to open status file: %s\n" , +strerror (-fdres)); }else if (fdres >=0 ) { past_scrubs =scrub_read_file (fdres, !do_quiet);if (IS_ERR (past_scrubs)) -ERR (!do_quiet,"WARNING: failed to read status file: " -"%s\n" ,strerror (-PTR_ERR (past_scrubs))); +warning_on (!do_quiet, +"failed to read status file: %s\n" , +strerror (-PTR_ERR (past_scrubs)));close (fdres); }
@@ -1264,11 +1263,11 @@ static int scrub_start(int argc, char **argv, int resume)* single devices, there is no reason to prevent this. */ if (!force &&is_scrub_running_on_fs (&fi_args, di_args, past_scrubs)) { -ERR (!do_quiet, -"ERROR: scrub is already running.\n" -"To cancel use 'btrfs scrub cancel %s'.\n" -"To see the status use 'btrfs scrub status [-d] %s'.\n" , - path, path); +error_on (!do_quiet, +"scrub is already running.\n" +"To cancel use 'btrfs scrub cancel %s'.\n" +"To see the status use 'btrfs scrub status [-d] %s'.\n" , + path, path); err =1 ; goto out; }
@@ -1278,7 +1277,8 @@ static int scrub_start(int argc, char **argv, int resume) spc.progress =calloc (fi_args.num_devices *2 ,sizeof (*spc.progress));if (!t_devs || !sp || !spc.progress) { -ERR (!do_quiet,"ERROR: scrub failed: %s" ,strerror (errno)); +error_on (!do_quiet, +"scrub failed: %s" ,strerror (errno)); err =1 ; goto out; }
@@ -1287,8 +1287,8 @@ static int scrub_start(int argc, char **argv, int resume) devid = di_args[i].devid; ret =pthread_mutex_init (&sp[i].progress_mutex,NULL );if (ret) { -ERR (!do_quiet,"ERROR: pthread_mutex_init failed: " -"%s\n" ,strerror (ret)); +error_on (!do_quiet,"pthread_mutex_init failed: %s\n" , +strerror (ret)); err =1 ; goto out; }
@@ -1342,7 +1342,7 @@ static int scrub_start(int argc, char **argv, int resume) ret =connect (prg_fd, (struct sockaddr *)&addr,sizeof (addr));if (!ret || errno !=ECONNREFUSED ) {/* ... yes, so scrub must be running. error out */ -fprintf (stderr,"ERROR: scrub already running\n" ); +error ("scrub already running\n" );close (prg_fd); prg_fd =-1 ; goto out;
@@ -1356,10 +1356,10 @@ static int scrub_start(int argc, char **argv, int resume)if (ret !=-1 ) ret =listen (prg_fd,100 );if (ret ==-1 ) { -ERR (!do_quiet,"WARNING: failed to open the progress status " -"socket at %s: %s. Progress cannot be queried\n" , - sock_path[0 ] ? sock_path :SCRUB_PROGRESS_SOCKET_PATH , -strerror (errno)); +warning_on (!do_quiet, +"failed to open the progress status socket at %s: %s. Progress cannot be queried\n" , + sock_path[0 ] ? sock_path : +SCRUB_PROGRESS_SOCKET_PATH ,strerror (errno));if (prg_fd !=-1 ) {close (prg_fd); prg_fd =-1 ;
@@ -1373,9 +1373,9 @@ static int scrub_start(int argc, char **argv, int resume) ret =scrub_write_progress (&spc_write_mutex, fsid, sp, fi_args.num_devices);if (ret) { -ERR (!do_quiet,"WARNING: failed to write the progress " -"status file: %s. Status recording disabled\n" , -strerror (-ret)); +warning_on (!do_quiet, +"failed to write the progress status file: %s. Status recording disabled\n" , +strerror (-ret)); do_record =0 ; } }
@@ -1383,8 +1383,9 @@ static int scrub_start(int argc, char **argv, int resume)if (do_background) { pid =fork ();if (pid ==-1 ) { -ERR (!do_quiet,"ERROR: cannot scrub, fork failed: " -"%s\n" ,strerror (errno)); +error_on (!do_quiet, +"cannot scrub, fork failed: %s\n" , +strerror (errno)); err =1 ; goto out; }
@@ -1402,13 +1403,15 @@ static int scrub_start(int argc, char **argv, int resume) } ret =wait (&stat);if (ret != pid) { -ERR (!do_quiet,"ERROR: wait failed: (ret=%d) " -"%s\n" , ret,strerror (errno)); +error_on (!do_quiet, +"wait failed: (ret=%d) %s\n" , + ret,strerror (errno)); err =1 ; goto out; }if (!WIFEXITED (stat) ||WEXITSTATUS (stat)) { -ERR (!do_quiet,"ERROR: scrub process failed\n" ); +error_on (!do_quiet, +"scrub process failed\n" ); err =WIFEXITED (stat) ?WEXITSTATUS (stat) :-1 ; goto out; }
@@ -1434,9 +1437,8 @@ static int scrub_start(int argc, char **argv, int resume) scrub_one_dev, &sp[i]);if (ret) {if (do_print) -fprintf (stderr,"ERROR: creating " -"scrub_one_dev[%llu] thread failed: " -"%s\n" , devid,strerror (ret)); +error ("creating scrub_one_dev[%llu] thread failed: %s\n" , + devid,strerror (ret)); err =1 ; goto out; }
@@ -1451,8 +1453,8 @@ static int scrub_start(int argc, char **argv, int resume) ret =pthread_create (&t_prog,NULL , scrub_progress_cycle, &spc);if (ret) {if (do_print) -fprintf (stderr,"ERROR: creating progress thread " -"failed: %s\n" ,strerror (ret)); +error ("creating progress thread failed: %s\n" , +strerror (ret)); err =1 ; goto out; }
@@ -1465,25 +1467,23 @@ static int scrub_start(int argc, char **argv, int resume) ret =pthread_join (t_devs[i],NULL );if (ret) {if (do_print) -fprintf (stderr,"ERROR: pthread_join failed " -"for scrub_one_dev[%llu]: %s\n" , devid, -strerror (ret)); +error ("pthread_join failed for scrub_one_dev[%llu]: %s\n" , + devid,strerror (ret)); ++err;continue ; }if (sp[i].ret && sp[i].ioctl_errno ==ENODEV ) {if (do_print) -fprintf (stderr,"WARNING: device %lld not " -"present\n" , devid); +warning ("device %lld not present\n" , + devid);continue ; }if (sp[i].ret && sp[i].ioctl_errno ==ECANCELED ) { ++err; }else if (sp[i].ret) {if (do_print) -fprintf (stderr,"ERROR: scrubbing %s failed " -"for device id %lld (%s)\n" , path, - devid,strerror (sp[i].ioctl_errno)); +error ("scrubbing %s failed for device id %lld (%s)\n" , + path, devid,strerror (sp[i].ioctl_errno)); ++err;continue ; }
@@ -1524,22 +1524,22 @@ static int scrub_start(int argc, char **argv, int resume)/* check for errors from the handling of the progress thread */ if (do_print && ret) { -fprintf (stderr,"ERROR: progress thread handling failed: %s\n" , -strerror (ret)); +error ("progress thread handling failed: %s\n" , +strerror (ret)); }/* check for errors returned from the progress thread itself */ if (do_print && terr && terr !=PTHREAD_CANCELED ) { -fprintf (stderr,"ERROR: recording progress " -"failed: %s\n" ,strerror (-PTR_ERR (terr))); +error ("recording progress failed: %s\n" , +strerror (-PTR_ERR (terr))); }if (do_record) { ret =scrub_write_progress (&spc_write_mutex, fsid, sp, fi_args.num_devices);if (ret && do_print) { -fprintf (stderr,"ERROR: failed to record the result: " -"%s\n" ,strerror (-ret)); +error ("failed to record the result: %s\n" , +strerror (-ret)); } }
@@ -1563,11 +1563,12 @@ out:if (nothing_to_resume)return 2 ;if (e_uncorrectable) { -ERR (!do_quiet,"ERROR: There are uncorrectable errors.\n" ); +error_on (!do_quiet,"There are uncorrectable errors.\n" );return 3 ; }if (e_correctable) -ERR (!do_quiet,"WARNING: errors detected during scrubbing, corrected.\n" ); +warning_on (!do_quiet, +"errors detected during scrubbing, corrected.\n" );return 0 ; }
@@ -1614,12 +1615,11 @@ static int cmd_scrub_cancel(int argc, char **argv) fdmnt =open_path_or_dev_mnt (path, &dirstream);if (fdmnt <0 ) {if (errno ==EINVAL ) -fprintf (stderr, -"ERROR: '%s' is not a mounted btrfs device\n" , - path); +error ("'%s' is not a mounted btrfs device\n" , + path);else -fprintf (stderr,"ERROR: can't access '%s': %s\n" , - path,strerror (errno)); +error ("can't access '%s': %s\n" , + path,strerror (errno)); ret =1 ; goto out; }
@@ -1627,8 +1627,9 @@ static int cmd_scrub_cancel(int argc, char **argv) ret =ioctl (fdmnt,BTRFS_IOC_SCRUB_CANCEL ,NULL );if (ret <0 ) { -fprintf (stderr,"ERROR: scrub cancel failed on %s: %s\n" , path, - errno ==ENOTCONN ?"not running" :strerror (errno)); +error ("scrub cancel failed on %s: %s\n" , + path, + errno ==ENOTCONN ?"not running" :strerror (errno));if (errno ==ENOTCONN ) ret =2 ;else
@@ -1719,24 +1720,23 @@ static int cmd_scrub_status(int argc, char **argv)if (fdmnt <0 ) {if (errno ==EINVAL ) -fprintf (stderr, -"ERROR: '%s' is not a mounted btrfs device\n" , - path); +error ("'%s' is not a mounted btrfs device\n" , + path);else -fprintf (stderr,"ERROR: can't access '%s': %s\n" , - path,strerror (errno)); +error ("can't access '%s': %s\n" , + path,strerror (errno));return 1 ; } ret =get_fs_info (path, &fi_args, &di_args);if (ret) { -fprintf (stderr,"ERROR: getting dev info for scrub failed: " -"%s\n" ,strerror (-ret)); +error ("getting dev info for scrub failed: %s\n" , +strerror (-ret)); err =1 ; goto out; }if (!fi_args.num_devices) { -fprintf (stderr,"ERROR: no devices found\n" ); +error ("no devices found\n" ); err =1 ; goto out; }
@@ -1745,9 +1745,8 @@ static int cmd_scrub_status(int argc, char **argv) fdres =socket (AF_UNIX ,SOCK_STREAM ,0 );if (fdres ==-1 ) { -fprintf (stderr,"ERROR: failed to create socket to " -"receive progress information: %s\n" , -strerror (errno)); +error ("failed to create socket to receive progress information: %s\n" , +strerror (errno)); err =1 ; goto out; }
@@ -1760,8 +1759,8 @@ static int cmd_scrub_status(int argc, char **argv)close (fdres); fdres =scrub_open_file_r (SCRUB_DATA_FILE , fsid);if (fdres <0 && fdres != -ENOENT ) { -fprintf (stderr,"WARNING: failed to open status file: " -"%s\n" ,strerror (-fdres)); +warning ("failed to open status file: %s\n" , +strerror (-fdres)); err =1 ; goto out; }
@@ -1770,7 +1769,7 @@ static int cmd_scrub_status(int argc, char **argv)if (fdres >=0 ) { past_scrubs =scrub_read_file (fdres,1 );if (IS_ERR (past_scrubs)) -fprintf (stderr,"WARNING: failed to read status: %s\n" , +warning ("failed to read status: %s\n" ,strerror (-PTR_ERR (past_scrubs))); } in_progress =is_scrub_running_in_kernel (fdmnt, di_args, fi_args.num_devices);
Signature · 1 line
1.8.5.1