| Message ID | 1398556834-31913-5-git-send-email-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 4/26/14, 7:00 PM, Theodore Ts'o wrote: > If mke2fs needs to ask the user for permission, and the user doesn't > type anything for five seconds, proceed as if the user had said yes. > > This will allow us to add more stringent checks without breaking > existing scripts (much). Hm, this sounds a little dangerous - "-F" overrides a lot. What motivates this? Would it be worth treating this differently depending on whether or not we're on a tty? While we're at it, I see that you've added another tunable to a conf file. Where are these documented? Thanks, -Eric > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > --- > misc/mke2fs.c | 12 ++++++++---- > misc/tune2fs.c | 2 +- > misc/util.c | 29 ++++++++++++++++++++++++++--- > misc/util.h | 2 +- > 4 files changed, 36 insertions(+), 9 deletions(-) > > diff --git a/misc/mke2fs.c b/misc/mke2fs.c > index a2b1f65..799132a 100644 > --- a/misc/mke2fs.c > +++ b/misc/mke2fs.c > @@ -102,6 +102,7 @@ static __u32 fs_stride; > static int quotatype = -1; /* Initialize both user and group quotas by default */ > static __u64 offset; > static blk64_t journal_location = ~0LL; > +static int proceed_delay = -1; > > static struct ext2_super_block fs_param; > static char *fs_uuid = NULL; > @@ -1749,9 +1750,12 @@ profile_error: > if (optind < argc) > usage(); > > + profile_get_integer(profile, "options", "proceed_delay", 0, 5, > + &proceed_delay); > + > if (!check_plausibility(device_name, CREATE_FILE, > &is_device) && !force) > - proceed_question(); > + proceed_question(proceed_delay); > > check_mount(device_name, force, _("filesystem")); > > @@ -1797,7 +1801,7 @@ profile_error: > } else if (!force && is_device && (fs_blocks_count > dev_size)) { > com_err(program_name, 0, "%s", > _("Filesystem larger than apparent device size.")); > - proceed_question(); > + proceed_question(proceed_delay); > } > > if (!fs_type) > @@ -2071,7 +2075,7 @@ profile_error: > com_err(program_name, 0, > _("%d-byte blocks too big for system (max %d)"), > blocksize, sys_page_size); > - proceed_question(); > + proceed_question(proceed_delay); > } > fprintf(stderr, _("Warning: %d-byte blocks too big for system " > "(max %d), forced to continue\n"), > @@ -2785,7 +2789,7 @@ int main (int argc, char *argv[]) > > if (!check_plausibility(journal_device, CHECK_BLOCK_DEV, > NULL) && !force) > - proceed_question(); > + proceed_question(proceed_delay); > check_mount(journal_device, force, _("journal")); > > retval = ext2fs_open(journal_device, EXT2_FLAG_RW| > diff --git a/misc/tune2fs.c b/misc/tune2fs.c > index fbf5f52..7b3723b 100644 > --- a/misc/tune2fs.c > +++ b/misc/tune2fs.c > @@ -675,7 +675,7 @@ static int add_journal(ext2_filsys fs) > if (journal_device) { > if (!check_plausibility(journal_device, CHECK_BLOCK_DEV, > NULL)) > - proceed_question(); > + proceed_question(-1); > check_mount(journal_device, 0, _("journal")); > #ifdef CONFIG_TESTIO_DEBUG > if (getenv("TEST_IO_FLAGS") || getenv("TEST_IO_BLOCK")) { > diff --git a/misc/util.c b/misc/util.c > index f85942e..afb0058 100644 > --- a/misc/util.c > +++ b/misc/util.c > @@ -14,6 +14,8 @@ > > #include "config.h" > #include <fcntl.h> > +#include <setjmp.h> > +#include <signal.h> > #include <stdio.h> > #include <string.h> > #ifdef HAVE_ERRNO_H > @@ -68,18 +70,39 @@ char *get_progname(char *argv_zero) > return cp+1; > } > > -void proceed_question(void) > +static jmp_buf alarm_env; > + > +static void alarm_signal(int signal) > +{ > + longjmp(alarm_env, 1); > +} > + > +void proceed_question(int delay) > { > char buf[256]; > const char *short_yes = _("yY"); > > fflush(stdout); > fflush(stderr); > - fputs(_("Proceed anyway? (y,n) "), stdout); > + if (delay > 0) { > + if (setjmp(alarm_env)) { > + signal(SIGALRM, SIG_IGN); > + printf(_("<proceeding>\n")); > + return; > + } > + signal(SIGALRM, alarm_signal); > + printf(_("Proceed anyway (or wait %d seconds) ? (y,n) "), > + delay); > + alarm(delay); > + } else > + fputs(_("Proceed anyway? (y,n) "), stdout); > buf[0] = 0; > if (!fgets(buf, sizeof(buf), stdin) || > - strchr(short_yes, buf[0]) == 0) > + strchr(short_yes, buf[0]) == 0) { > + putc('\n', stdout); > exit(1); > + } > + signal(SIGALRM, SIG_IGN); > } > > /* > diff --git a/misc/util.h b/misc/util.h > index b80d489..9de3fbf 100644 > --- a/misc/util.h > +++ b/misc/util.h > @@ -25,7 +25,7 @@ extern char *journal_location_string; > extern int strcasecmp (char *s1, char *s2); > #endif > extern char *get_progname(char *argv_zero); > -extern void proceed_question(void); > +extern void proceed_question(int delay); > extern int check_plausibility(const char *device, int flags, > int *ret_is_dev); > extern void parse_journal_opts(const char *opts); > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/28/14, 10:33 AM, Eric Sandeen wrote: > On 4/26/14, 7:00 PM, Theodore Ts'o wrote: >> If mke2fs needs to ask the user for permission, and the user doesn't >> type anything for five seconds, proceed as if the user had said yes. >> >> This will allow us to add more stringent checks without breaking >> existing scripts (much). > > Hm, this sounds a little dangerous - "-F" overrides a lot. To mitigate the danger, it might be better to keep the default, but allow setting a non-infinite delay in environments which require it. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 28, 2014 at 10:33:40AM -0500, Eric Sandeen wrote: > On 4/26/14, 7:00 PM, Theodore Ts'o wrote: > > If mke2fs needs to ask the user for permission, and the user doesn't > > type anything for five seconds, proceed as if the user had said yes. > > > > This will allow us to add more stringent checks without breaking > > existing scripts (much). > > Hm, this sounds a little dangerous - "-F" overrides a lot. Actually, if you take a look at what we use proceed_question() for, it doesn't actually override anything (up until now) that might lead to data loss. It's for things like trying to create an file system with a block size greater than 4k on an x86 platform, creating a file system larger than the apparent block size, etc. The main goal was to make sure the user actually *sees* the darned message. Perhaps the only case where proceed_question() can prevent data loss is the one where the user typo's /dev/sda3 as /dev/sda. Everything else is in the category of "we want to make sure the user sees the warning". The motivation behind this is adding this safety check: % ./misc/mke2fs -t ext4 -L test-filesystem /dev/sdc3 8M mke2fs 1.42.9 (4-Feb-2014) /dev/sdc3 contains a ext4 file system labelled 'test-filesystem' Proceed anyway (or wait 5 seconds) ? (y,n) Previously, we would blithely blow away /dev/sdc3 without even giving a warning. So if stdin (fd 0) is not a tty, we skip this test entirely --- otherwise existing scripts would fail. However, if a script is attached to a tty, we would end up stalling the script waiting for the user to answer yes/no where previously no question would be asked at all. This is the case where it's important that proceed_question() will now pause five seconds, and then continue. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/28/14, 6:26 PM, Theodore Ts'o wrote: > On Mon, Apr 28, 2014 at 10:33:40AM -0500, Eric Sandeen wrote: >> On 4/26/14, 7:00 PM, Theodore Ts'o wrote: >>> If mke2fs needs to ask the user for permission, and the user doesn't >>> type anything for five seconds, proceed as if the user had said yes. >>> >>> This will allow us to add more stringent checks without breaking >>> existing scripts (much). >> >> Hm, this sounds a little dangerous - "-F" overrides a lot. > > Actually, if you take a look at what we use proceed_question() for, it > doesn't actually override anything (up until now) that might lead to > data loss. It's for things like trying to create an file system with > a block size greater than 4k on an x86 platform, creating a file > system larger than the apparent block size, etc. The main goal was to > make sure the user actually *sees* the darned message. > > Perhaps the only case where proceed_question() can prevent data loss > is the one where the user typo's /dev/sda3 as /dev/sda. Everything > else is in the category of "we want to make sure the user sees the > warning". > > The motivation behind this is adding this safety check: > > % ./misc/mke2fs -t ext4 -L test-filesystem /dev/sdc3 8M > mke2fs 1.42.9 (4-Feb-2014) > /dev/sdc3 contains a ext4 file system labelled 'test-filesystem' > Proceed anyway (or wait 5 seconds) ? (y,n) > > Previously, we would blithely blow away /dev/sdc3 without even giving > a warning. So if stdin (fd 0) is not a tty, we skip this test > entirely --- otherwise existing scripts would fail. However, if a > script is attached to a tty, we would end up stalling the script > waiting for the user to answer yes/no where previously no question > would be asked at all. This is the case where it's important that > proceed_question() will now pause five seconds, and then continue. I guess it's up to you, but it gives me the heebie-jeebies. xfs and btrfs already stop on an existing fs (or a partition table) unless the script adds the force option. Stopping to make sure about an irreversible action - but proceeding after 5s anyway - seems to me like the worst of both worlds. If it doesn't matter, don't ask. If it matters, wait for a response, however long it might take. At least that's my take on it. :) -Eric > - Ted > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 28 Apr 2014, Eric Sandeen wrote: > Date: Mon, 28 Apr 2014 19:32:23 -0500 > From: Eric Sandeen <sandeen@redhat.com> > To: Theodore Ts'o <tytso@mit.edu> > Cc: Ext4 Developers List <linux-ext4@vger.kernel.org> > Subject: Re: [PATCH 5/7] mke2fs: proceed if the user doesn't type anything > after 5 seconds > > On 4/28/14, 6:26 PM, Theodore Ts'o wrote: > > On Mon, Apr 28, 2014 at 10:33:40AM -0500, Eric Sandeen wrote: > >> On 4/26/14, 7:00 PM, Theodore Ts'o wrote: > >>> If mke2fs needs to ask the user for permission, and the user doesn't > >>> type anything for five seconds, proceed as if the user had said yes. > >>> > >>> This will allow us to add more stringent checks without breaking > >>> existing scripts (much). > >> > >> Hm, this sounds a little dangerous - "-F" overrides a lot. > > > > Actually, if you take a look at what we use proceed_question() for, it > > doesn't actually override anything (up until now) that might lead to > > data loss. It's for things like trying to create an file system with > > a block size greater than 4k on an x86 platform, creating a file > > system larger than the apparent block size, etc. The main goal was to > > make sure the user actually *sees* the darned message. > > > > Perhaps the only case where proceed_question() can prevent data loss > > is the one where the user typo's /dev/sda3 as /dev/sda. Everything > > else is in the category of "we want to make sure the user sees the > > warning". > > > > The motivation behind this is adding this safety check: > > > > % ./misc/mke2fs -t ext4 -L test-filesystem /dev/sdc3 8M > > mke2fs 1.42.9 (4-Feb-2014) > > /dev/sdc3 contains a ext4 file system labelled 'test-filesystem' > > Proceed anyway (or wait 5 seconds) ? (y,n) > > > > Previously, we would blithely blow away /dev/sdc3 without even giving > > a warning. So if stdin (fd 0) is not a tty, we skip this test > > entirely --- otherwise existing scripts would fail. However, if a > > script is attached to a tty, we would end up stalling the script > > waiting for the user to answer yes/no where previously no question > > would be asked at all. This is the case where it's important that > > proceed_question() will now pause five seconds, and then continue. > > I guess it's up to you, but it gives me the heebie-jeebies. xfs > and btrfs already stop on an existing fs (or a partition table) unless > the script adds the force option. Stopping to make sure about an > irreversible action - but proceeding after 5s anyway - seems to me > like the worst of both worlds. If it doesn't matter, don't ask. > If it matters, wait for a response, however long it might take. > > At least that's my take on it. :) I tend to agree. This solution sounds really scary and unpredictable. It's true that we do not want to break scripts, so in that case we could just test for tty and fallback to a old behaviour if there is not tty attached. Otherwise ask. Also overriding it with force is ok, but we might have another argument just specifically for this case, let's say '-w | --wipe-signatures' ? -Lukas > > -Eric > > > - Ted > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/misc/mke2fs.c b/misc/mke2fs.c index a2b1f65..799132a 100644 --- a/misc/mke2fs.c +++ b/misc/mke2fs.c @@ -102,6 +102,7 @@ static __u32 fs_stride; static int quotatype = -1; /* Initialize both user and group quotas by default */ static __u64 offset; static blk64_t journal_location = ~0LL; +static int proceed_delay = -1; static struct ext2_super_block fs_param; static char *fs_uuid = NULL; @@ -1749,9 +1750,12 @@ profile_error: if (optind < argc) usage(); + profile_get_integer(profile, "options", "proceed_delay", 0, 5, + &proceed_delay); + if (!check_plausibility(device_name, CREATE_FILE, &is_device) && !force) - proceed_question(); + proceed_question(proceed_delay); check_mount(device_name, force, _("filesystem")); @@ -1797,7 +1801,7 @@ profile_error: } else if (!force && is_device && (fs_blocks_count > dev_size)) { com_err(program_name, 0, "%s", _("Filesystem larger than apparent device size.")); - proceed_question(); + proceed_question(proceed_delay); } if (!fs_type) @@ -2071,7 +2075,7 @@ profile_error: com_err(program_name, 0, _("%d-byte blocks too big for system (max %d)"), blocksize, sys_page_size); - proceed_question(); + proceed_question(proceed_delay); } fprintf(stderr, _("Warning: %d-byte blocks too big for system " "(max %d), forced to continue\n"), @@ -2785,7 +2789,7 @@ int main (int argc, char *argv[]) if (!check_plausibility(journal_device, CHECK_BLOCK_DEV, NULL) && !force) - proceed_question(); + proceed_question(proceed_delay); check_mount(journal_device, force, _("journal")); retval = ext2fs_open(journal_device, EXT2_FLAG_RW| diff --git a/misc/tune2fs.c b/misc/tune2fs.c index fbf5f52..7b3723b 100644 --- a/misc/tune2fs.c +++ b/misc/tune2fs.c @@ -675,7 +675,7 @@ static int add_journal(ext2_filsys fs) if (journal_device) { if (!check_plausibility(journal_device, CHECK_BLOCK_DEV, NULL)) - proceed_question(); + proceed_question(-1); check_mount(journal_device, 0, _("journal")); #ifdef CONFIG_TESTIO_DEBUG if (getenv("TEST_IO_FLAGS") || getenv("TEST_IO_BLOCK")) { diff --git a/misc/util.c b/misc/util.c index f85942e..afb0058 100644 --- a/misc/util.c +++ b/misc/util.c @@ -14,6 +14,8 @@ #include "config.h" #include <fcntl.h> +#include <setjmp.h> +#include <signal.h> #include <stdio.h> #include <string.h> #ifdef HAVE_ERRNO_H @@ -68,18 +70,39 @@ char *get_progname(char *argv_zero) return cp+1; } -void proceed_question(void) +static jmp_buf alarm_env; + +static void alarm_signal(int signal) +{ + longjmp(alarm_env, 1); +} + +void proceed_question(int delay) { char buf[256]; const char *short_yes = _("yY"); fflush(stdout); fflush(stderr); - fputs(_("Proceed anyway? (y,n) "), stdout); + if (delay > 0) { + if (setjmp(alarm_env)) { + signal(SIGALRM, SIG_IGN); + printf(_("<proceeding>\n")); + return; + } + signal(SIGALRM, alarm_signal); + printf(_("Proceed anyway (or wait %d seconds) ? (y,n) "), + delay); + alarm(delay); + } else + fputs(_("Proceed anyway? (y,n) "), stdout); buf[0] = 0; if (!fgets(buf, sizeof(buf), stdin) || - strchr(short_yes, buf[0]) == 0) + strchr(short_yes, buf[0]) == 0) { + putc('\n', stdout); exit(1); + } + signal(SIGALRM, SIG_IGN); } /* diff --git a/misc/util.h b/misc/util.h index b80d489..9de3fbf 100644 --- a/misc/util.h +++ b/misc/util.h @@ -25,7 +25,7 @@ extern char *journal_location_string; extern int strcasecmp (char *s1, char *s2); #endif extern char *get_progname(char *argv_zero); -extern void proceed_question(void); +extern void proceed_question(int delay); extern int check_plausibility(const char *device, int flags, int *ret_is_dev); extern void parse_journal_opts(const char *opts);
If mke2fs needs to ask the user for permission, and the user doesn't type anything for five seconds, proceed as if the user had said yes. This will allow us to add more stringent checks without breaking existing scripts (much). Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- misc/mke2fs.c | 12 ++++++++---- misc/tune2fs.c | 2 +- misc/util.c | 29 ++++++++++++++++++++++++++--- misc/util.h | 2 +- 4 files changed, 36 insertions(+), 9 deletions(-)