From: Anand Jain <anand.jain@oracle.com>
To: dsterba@suse.cz, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v1.1 04/18] btrfs-progs: add global verbose and quiet options and helper functions
Date: Tue, 19 Nov 2019 13:07:05 +0800 [thread overview]
Message-ID: <dc018b50-def0-1a21-695b-e8ed068ee82a@oracle.com> (raw)
In-Reply-To: <20191115155816.GX3001@twin.jikos.cz>
On 11/15/19 11:58 PM, David Sterba wrote:
> On Mon, Nov 04, 2019 at 02:33:02PM +0800, Anand Jain wrote:
>> + case 'v':
>> + bconf.verbose < 0 ? bconf.verbose = 1 : bconf.verbose++;
>
> This code gets repeated and it's not IMO simple enough to be copy-pasted
> around. Eg. bconf_be_verbose() and eventually bconf_be_quiet().
I was just concerned- it will make life of other developers difficult,
IMO too much abstraction in the code is almost like learning a new
programming language for the new person looking at the code.
For example fstests. To write patch for fstests you need to
learn about a lot of helpers, defines and functions and filters
specific to fstests but you wouldn't have had this problem if the
fstests abstractions were limited and if it embraced open-code style.
Just my 1c.
For now I have added bconf_be_verbose() and bconf_be_quiet() it looks
neat as below,
+ case 'v':
+ bconf_be_verbose();
+ break;
+ case 'q':
+ bconf_be_quiet();
+ break;
>> + break;
>> + case 'q':
>> + bconf.verbose = 0;
>> + break;
>> default:
>> fprintf(stderr, "Unknown global option: %s\n",
>> argv[optind - 1]);
>> --- a/common/help.h
>> +++ b/common/help.h
>> @@ -53,6 +53,17 @@
>> "-t|--tbytes show sizes in TiB, or TB with --si"
>>
>> /*
>> + * Global verbose option for the sub-commands
>> + */
>> +#define HELPINFO_GLOBAL_OPTIONS_HEADER \
>> + "", \
>> + "Global options:"
>> +#define HELPINFO_INSERT_VERBOSE \
>> + "-v|--verbose show verbose output"
>
> increase output verbosity
fixed.
>> +#define HELPINFO_INSERT_QUIET \
>> + "-q|--quiet run the command quietly"
> print only errors
fixed.
>> +
>> +/*
>> * Special marker in the help strings that will preemptively insert the global
>> * options and then continue with the following text that possibly follows
>> * after the regular options
>> --- a/common/utils.h
>> +++ b/common/utils.h
>> @@ -122,6 +122,9 @@ void print_all_devices(struct list_head *devices);
>> */
>> struct btrfs_config {
>> unsigned int output_format;
>> +
>> + /* -1:unset 0:quiet >0:verbose */
>
> Instead of the constants, please add some defines for the unset and
> default states. Maybe also for quiet.
Fixed.
+#define BTRFS_BCONF_QUIET 0
+#define BTRFS_BCONF_UNSET -1
Thanks for the valuable comments.
Anand
>> + int verbose;
>> };
>> extern struct btrfs_config bconf;
next prev parent reply other threads:[~2019-11-19 5:07 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-04 6:32 [PATCH v1.1 00/18] btrfs-progs: global verbose and quiet option Anand Jain
2019-11-04 6:32 ` [PATCH v1 01/18] btrfs-progs: receive: fix option quiet Anand Jain
2019-11-04 6:33 ` [PATCH v1 02/18] btrfs-progs: balance status: fix usage show long verbose Anand Jain
2019-11-04 6:33 ` [PATCH v1 03/18] btrfs-progs: balance start: fix usage add " Anand Jain
2019-11-04 6:33 ` [PATCH v1.1 04/18] btrfs-progs: add global verbose and quiet options and helper functions Anand Jain
2019-11-14 16:08 ` David Sterba
2019-11-19 2:44 ` Anand Jain
2019-11-19 3:36 ` Anand Jain
2019-11-19 16:51 ` David Sterba
2019-11-25 10:36 ` Anand Jain
2019-11-15 15:58 ` David Sterba
2019-11-19 5:07 ` Anand Jain [this message]
2019-11-19 17:02 ` David Sterba
2019-11-04 6:33 ` [PATCH v1.1 05/18] btrfs-progs: send: use global verbose and quiet options Anand Jain
2019-11-04 6:33 ` [PATCH v1.1 06/18] btrfs-progs: receive: " Anand Jain
2019-11-04 6:33 ` [PATCH v1.1 07/18] btrfs-progs: subvolume delete: use global verbose option Anand Jain
2019-11-04 6:33 ` [PATCH v1 08/18] btrfs-progs: filesystem defragment: " Anand Jain
2019-11-14 16:16 ` David Sterba
2019-11-25 10:35 ` Anand Jain
2019-11-04 6:33 ` [PATCH v1 09/18] btrfs-progs: balance start: " Anand Jain
2019-11-04 6:33 ` [PATCH v1 10/18] btrfs-progs: balance status: " Anand Jain
2019-11-04 6:33 ` [PATCH v1 11/18] btrfs-progs: rescue chunk-recover: " Anand Jain
2019-11-04 6:33 ` [PATCH v1 12/18] btrfs-progs: rescue super-recover: " Anand Jain
2019-11-04 6:33 ` [PATCH v1.1 13/18] btrfs-progs: restore: " Anand Jain
2019-11-04 6:33 ` [PATCH v1 14/18] btrfs-progs: inspect-internal inode-resolve: use global verbose Anand Jain
2019-11-04 6:33 ` [PATCH v1 15/18] btrfs-progs: inspect-internal logical-resolve: use global verbose option Anand Jain
2019-11-04 6:33 ` [PATCH v1.1 16/18] btrfs-progs: refactor btrfs_scan_devices() to accept verbose argument Anand Jain
2019-11-04 6:33 ` [PATCH v1 17/18] btrfs-progs: device scan: add verbose option Anand Jain
2019-11-04 6:33 ` [PATCH v1.1 18/18] btrfs-progs: device scan: add quiet option Anand Jain
2019-11-15 16:11 ` [PATCH v1.1 00/18] btrfs-progs: global verbose and " David Sterba
2019-11-19 3:50 ` Anand Jain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dc018b50-def0-1a21-695b-e8ed068ee82a@oracle.com \
--to=anand.jain@oracle.com \
--cc=dsterba@suse.cz \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.