Very often people are creating file systems using regular files, so we
shouldn't ask the user to confirm using the proceed question.
Otherwise it encourages users to use the -F flag, which is a bad
thing.
We do need to continue to check if the external journal device is a
block device.
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
misc/mke2fs.c | 5 +++--
misc/tune2fs.c | 2 +-
misc/util.c | 16 ++++++++++------
misc/util.h | 8 +++++++-
4 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 176dc40..637ace2 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1750,7 +1750,7 @@ profile_error:
usage();
if (!force)
- check_plausibility(device_name);
+ check_plausibility(device_name, 0, NULL);
check_mount(device_name, force, _("filesystem"));
/* Determine the size of the device (if possible) */
@@ -2782,7 +2782,8 @@ int main (int argc, char *argv[])
ext2_filsys jfs;
if (!force)
- check_plausibility(journal_device);
+ check_plausibility(journal_device, CHECK_BLOCK_DEV,
+ NULL);
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 3359c4a..d61dbfb 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -673,7 +673,7 @@ static int add_journal(ext2_filsys fs)
goto err;
}
if (journal_device) {
- check_plausibility(journal_device);
+ check_plausibility(journal_device, CHECK_BLOCK_DEV, NULL);
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 92ab79f..0c3787c 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -80,9 +80,9 @@ void proceed_question(void)
exit(1);
}
-void check_plausibility(const char *device)
+void check_plausibility(const char *device, int flags, int *ret_is_dev)
{
- int val;
+ int val, is_dev = 0;
ext2fs_struct_stat s;
val = ext2fs_stat(device, &s);
@@ -95,13 +95,17 @@ void check_plausibility(const char *device)
"did you specify it correctly?\n"), stderr);
exit(1);
}
+ if (S_ISBLK(s.st_mode))
+ is_dev = 1;
#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
/* On FreeBSD, all disk devices are character specials */
- if (!S_ISBLK(s.st_mode) && !S_ISCHR(s.st_mode))
-#else
- if (!S_ISBLK(s.st_mode))
+ if (S_ISCHR(s.st_mode))
+ is_dev = 1;
#endif
- {
+ if (ret_is_dev)
+ *ret_is_dev = is_dev;
+
+ if ((flags & CHECK_BLOCK_DEV) && !is_dev) {
printf(_("%s is not a block special device.\n"), device);
proceed_question();
return;
diff --git a/misc/util.h b/misc/util.h
index 11604d0..470556a 100644
--- a/misc/util.h
+++ b/misc/util.h
@@ -15,12 +15,18 @@ extern int journal_flags;
extern char *journal_device;
extern char *journal_location_string;
+/*
+ * Flags for check_plausibility()
+ */
+#define CHECK_BLOCK_DEV 0x0001
+
#ifndef HAVE_STRCASECMP
extern int strcasecmp (char *s1, char *s2);
#endif
extern char *get_progname(char *argv_zero);
extern void proceed_question(void);
-extern void check_plausibility(const char *device);
+extern void check_plausibility(const char *device, int flags,
+ int *ret_is_dev);
extern void parse_journal_opts(const char *opts);
extern void check_mount(const char *device, int force, const char *type);
extern unsigned int figure_journal_size(int size, ext2_filsys fs);
--
1.9.0
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
misc/mke2fs.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 3694ce5..b274165 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -618,6 +618,9 @@ static void show_stats(ext2_filsys fs)
dgrp_t i;
int need, col_left;
+ if (!verbose)
+ goto skip_details;
+
if (ext2fs_blocks_count(&fs_param) != ext2fs_blocks_count(s))
fprintf(stderr, _("warning: %llu blocks unused.\n\n"),
ext2fs_blocks_count(&fs_param) - ext2fs_blocks_count(s));
@@ -666,6 +669,7 @@ static void show_stats(ext2_filsys fs)
s->s_blocks_per_group, s->s_clusters_per_group);
printf(_("%u inodes per group\n"), s->s_inodes_per_group);
+skip_details:
if (fs->group_desc_count == 1) {
printf("\n");
return;
--
1.9.0
Warn the system administrator if there is an existing file system on
the block device, and give the administrator an opportunity to abort
the mkfs operation.
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
misc/mke2fs.c | 4 +++-
misc/util.c | 29 +++++++++++++++++++++++++++++
misc/util.h | 1 +
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 799132a..3694ce5 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1753,7 +1753,9 @@ profile_error:
profile_get_integer(profile, "options", "proceed_delay", 0, 5,
&proceed_delay);
- if (!check_plausibility(device_name, CREATE_FILE,
+ /* The isatty() test is so we don't break existing scripts */
+ if (!check_plausibility(device_name, CREATE_FILE |
+ (isatty(0) ? CHECK_FS_EXIST : 0),
&is_device) && !force)
proceed_question(proceed_delay);
diff --git a/misc/util.c b/misc/util.c
index afb0058..be16ebe 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -113,6 +113,9 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev)
int fd, is_dev = 0;
ext2fs_struct_stat s;
int fl = O_RDONLY;
+ blkid_cache cache = NULL;
+ char *fs_type = NULL;
+ char *fs_label = NULL;
if (flags & CREATE_FILE)
fl |= O_CREAT;
@@ -148,6 +151,32 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev)
return 0;
}
+ if ((flags & CHECK_FS_EXIST) && blkid_get_cache(&cache, NULL) >= 0) {
+ fs_type = blkid_get_tag_value(cache, "TYPE", device);
+ if (fs_type)
+ fs_label = blkid_get_tag_value(cache, "LABEL", device);
+ blkid_put_cache(cache);
+ }
+
+ if (fs_type) {
+ if (fs_label)
+ printf(_("%s contains a %s file system "
+ "labelled '%s'\n"), device, fs_type, fs_label);
+ else
+ printf(_("%s contains a %s file system\n"), device,
+ fs_type);
+ free(fs_type);
+ free(fs_label);
+ return 0;
+ }
+
+ /*
+ * We should eventually replace this with a test for the
+ * presence of a partition table. Unfortunately the blkid
+ * library doesn't test for partition tabels, and checking for
+ * valid GPT and MBR and possibly others isn't quite trivial.
+ */
+
#ifdef HAVE_LINUX_MAJOR_H
#ifndef MAJOR
#define MAJOR(dev) ((dev)>>8)
diff --git a/misc/util.h b/misc/util.h
index 9de3fbf..745568e 100644
--- a/misc/util.h
+++ b/misc/util.h
@@ -20,6 +20,7 @@ extern char *journal_location_string;
*/
#define CHECK_BLOCK_DEV 0x0001
#define CREATE_FILE 0x0002
+#define CHECK_FS_EXIST 0x0004
#ifndef HAVE_STRCASECMP
extern int strcasecmp (char *s1, char *s2);
--
1.9.0
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" <[email protected]>
---
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);
--
1.9.0
Don't ask the user if it's OK that a regular file is smaller than the
requested size. This test only makes sense if we are creating the
file system on a block device. This allow users to not need to
manually answer the "proceed?" question when creating a file system
backed by a simple file.
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
misc/mke2fs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 3c62ede..fa61e7b 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1383,7 +1383,7 @@ static void PRS(int argc, char *argv[])
unsigned long flex_bg_size = 0;
double reserved_ratio = -1.0;
int lsector_size = 0, psector_size = 0;
- int show_version_only = 0;
+ int show_version_only = 0, is_device = 0;
unsigned long long num_inodes = 0; /* unsigned long long to catch too-large input */
errcode_t retval;
char * oldpath = getenv("PATH");
@@ -1749,7 +1749,7 @@ profile_error:
if (optind < argc)
usage();
- if (!check_plausibility(device_name, 0, NULL) && !force)
+ if (!check_plausibility(device_name, 0, &is_device) && !force)
proceed_question();
check_mount(device_name, force, _("filesystem"));
@@ -1793,7 +1793,7 @@ profile_error:
fs_blocks_count &= ~((blk64_t) ((sys_page_size /
EXT2_BLOCK_SIZE(&fs_param))-1));
}
- } else if (!force && (fs_blocks_count > dev_size)) {
+ } else if (!force && is_device && (fs_blocks_count > dev_size)) {
com_err(program_name, 0, "%s",
_("Filesystem larger than apparent device size."));
proceed_question();
--
1.9.0
Move the call to proceed_question() from check_plausibility() to its
caller. This allows more fine grained control by mke2fs about when it
might want to call check_plausibility().
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
misc/mke2fs.c | 11 ++++++-----
misc/tune2fs.c | 4 +++-
misc/util.c | 11 +++++++----
misc/util.h | 4 ++--
4 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 637ace2..3c62ede 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1749,8 +1749,9 @@ profile_error:
if (optind < argc)
usage();
- if (!force)
- check_plausibility(device_name, 0, NULL);
+ if (!check_plausibility(device_name, 0, NULL) && !force)
+ proceed_question();
+
check_mount(device_name, force, _("filesystem"));
/* Determine the size of the device (if possible) */
@@ -2781,9 +2782,9 @@ int main (int argc, char *argv[])
if (journal_device) {
ext2_filsys jfs;
- if (!force)
- check_plausibility(journal_device, CHECK_BLOCK_DEV,
- NULL);
+ if (!check_plausibility(journal_device, CHECK_BLOCK_DEV,
+ NULL) && !force)
+ proceed_question();
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 d61dbfb..fbf5f52 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -673,7 +673,9 @@ static int add_journal(ext2_filsys fs)
goto err;
}
if (journal_device) {
- check_plausibility(journal_device, CHECK_BLOCK_DEV, NULL);
+ if (!check_plausibility(journal_device, CHECK_BLOCK_DEV,
+ NULL))
+ proceed_question();
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 0c3787c..08ec761 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -80,7 +80,10 @@ void proceed_question(void)
exit(1);
}
-void check_plausibility(const char *device, int flags, int *ret_is_dev)
+/*
+ * return 1 if the device looks plausible
+ */
+int check_plausibility(const char *device, int flags, int *ret_is_dev)
{
int val, is_dev = 0;
ext2fs_struct_stat s;
@@ -107,8 +110,7 @@ void check_plausibility(const char *device, int flags, int *ret_is_dev)
if ((flags & CHECK_BLOCK_DEV) && !is_dev) {
printf(_("%s is not a block special device.\n"), device);
- proceed_question();
- return;
+ return 0;
}
#ifdef HAVE_LINUX_MAJOR_H
@@ -137,9 +139,10 @@ void check_plausibility(const char *device, int flags, int *ret_is_dev)
MINOR(s.st_rdev)%16 == 0))) {
printf(_("%s is entire device, not just one partition!\n"),
device);
- proceed_question();
+ return 0;
}
#endif
+ return 1;
}
void check_mount(const char *device, int force, const char *type)
diff --git a/misc/util.h b/misc/util.h
index 470556a..867f4b0 100644
--- a/misc/util.h
+++ b/misc/util.h
@@ -25,8 +25,8 @@ extern int strcasecmp (char *s1, char *s2);
#endif
extern char *get_progname(char *argv_zero);
extern void proceed_question(void);
-extern void check_plausibility(const char *device, int flags,
- int *ret_is_dev);
+extern int check_plausibility(const char *device, int flags,
+ int *ret_is_dev);
extern void parse_journal_opts(const char *opts);
extern void check_mount(const char *device, int force, const char *type);
extern unsigned int figure_journal_size(int size, ext2_filsys fs);
--
1.9.0
This is useful when creating a filesystem for use with a VM, for
example.
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
misc/mke2fs.c | 3 ++-
misc/util.c | 20 ++++++++++++++++----
misc/util.h | 1 +
3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index fa61e7b..a2b1f65 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1749,7 +1749,8 @@ profile_error:
if (optind < argc)
usage();
- if (!check_plausibility(device_name, 0, &is_device) && !force)
+ if (!check_plausibility(device_name, CREATE_FILE,
+ &is_device) && !force)
proceed_question();
check_mount(device_name, force, _("filesystem"));
diff --git a/misc/util.c b/misc/util.c
index 08ec761..f85942e 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -13,6 +13,7 @@
#define _LARGEFILE64_SOURCE
#include "config.h"
+#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#ifdef HAVE_ERRNO_H
@@ -21,6 +22,7 @@
#ifdef HAVE_LINUX_MAJOR_H
#include <linux/major.h>
#endif
+#include <sys/types.h>
#ifdef HAVE_SYS_STAT_H
#include <sys/stat.h>
#endif
@@ -85,19 +87,29 @@ void proceed_question(void)
*/
int check_plausibility(const char *device, int flags, int *ret_is_dev)
{
- int val, is_dev = 0;
+ int fd, is_dev = 0;
ext2fs_struct_stat s;
+ int fl = O_RDONLY;
- val = ext2fs_stat(device, &s);
+ if (flags & CREATE_FILE)
+ fl |= O_CREAT;
- if(val == -1) {
- fprintf(stderr, _("Could not stat %s --- %s\n"),
+ fd = open(device, fl, 0666);
+ if (fd < 0) {
+ fprintf(stderr, _("Could not open %s: %s\n"),
device, error_message(errno));
if (errno == ENOENT)
fputs(_("\nThe device apparently does not exist; "
"did you specify it correctly?\n"), stderr);
exit(1);
}
+
+ if (ext2fs_fstat(fd, &s) < 0) {
+ perror("stat");
+ exit(1);
+ }
+ close(fd);
+
if (S_ISBLK(s.st_mode))
is_dev = 1;
#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
diff --git a/misc/util.h b/misc/util.h
index 867f4b0..b80d489 100644
--- a/misc/util.h
+++ b/misc/util.h
@@ -19,6 +19,7 @@ extern char *journal_location_string;
* Flags for check_plausibility()
*/
#define CHECK_BLOCK_DEV 0x0001
+#define CREATE_FILE 0x0002
#ifndef HAVE_STRCASECMP
extern int strcasecmp (char *s1, char *s2);
--
1.9.0
On 4/26/14, 7:00 PM, Theodore Ts'o wrote:
> Very often people are creating file systems using regular files, so we
> shouldn't ask the user to confirm using the proceed question.
> Otherwise it encourages users to use the -F flag, which is a bad
> thing.
Yup, agreed - that'll be nice.
Acked-by: Eric Sandeen <[email protected]>
On 4/26/14, 7:00 PM, Theodore Ts'o wrote:
> Don't ask the user if it's OK that a regular file is smaller than the
> requested size. This test only makes sense if we are creating the
> file system on a block device. This allow users to not need to
> manually answer the "proceed?" question when creating a file system
> backed by a simple file.
I wonder if it'd be better to just attempt to write the last
block before we start marching through the block groups - then
we can just fail outright if the mkfs won't work, rather than
asking questions based on things we could find out programatically...?
-Eric
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> misc/mke2fs.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 3c62ede..fa61e7b 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1383,7 +1383,7 @@ static void PRS(int argc, char *argv[])
> unsigned long flex_bg_size = 0;
> double reserved_ratio = -1.0;
> int lsector_size = 0, psector_size = 0;
> - int show_version_only = 0;
> + int show_version_only = 0, is_device = 0;
> unsigned long long num_inodes = 0; /* unsigned long long to catch too-large input */
> errcode_t retval;
> char * oldpath = getenv("PATH");
> @@ -1749,7 +1749,7 @@ profile_error:
> if (optind < argc)
> usage();
>
> - if (!check_plausibility(device_name, 0, NULL) && !force)
> + if (!check_plausibility(device_name, 0, &is_device) && !force)
> proceed_question();
>
> check_mount(device_name, force, _("filesystem"));
> @@ -1793,7 +1793,7 @@ profile_error:
> fs_blocks_count &= ~((blk64_t) ((sys_page_size /
> EXT2_BLOCK_SIZE(&fs_param))-1));
> }
> - } else if (!force && (fs_blocks_count > dev_size)) {
> + } else if (!force && is_device && (fs_blocks_count > dev_size)) {
> com_err(program_name, 0, "%s",
> _("Filesystem larger than apparent device size."));
> proceed_question();
>
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
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" <[email protected]>
> ---
> 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);
>
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
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
>
On Mon, 28 Apr 2014, Eric Sandeen wrote:
> Date: Mon, 28 Apr 2014 19:32:23 -0500
> From: Eric Sandeen <[email protected]>
> To: Theodore Ts'o <[email protected]>
> Cc: Ext4 Developers List <[email protected]>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Sat, 26 Apr 2014, Theodore Ts'o wrote:
> Date: Sat, 26 Apr 2014 20:00:34 -0400
> From: Theodore Ts'o <[email protected]>
> To: Ext4 Developers List <[email protected]>
> Cc: Theodore Ts'o <[email protected]>
> Subject: [PATCH 7/7] mke2fs: only print the low-level file system stats in
> verbose mode
While I kind of like this, because mke2fs is really quite verbose as
it is. However I am afraid that this will break scripts for people.
Also there are actually some useful information in that output like
block size, size of the file system and file system label if
specified. Also maybe having UUID in there will be also useful.
Thanks!
-Lukas
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> misc/mke2fs.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 3694ce5..b274165 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -618,6 +618,9 @@ static void show_stats(ext2_filsys fs)
> dgrp_t i;
> int need, col_left;
>
> + if (!verbose)
> + goto skip_details;
> +
> if (ext2fs_blocks_count(&fs_param) != ext2fs_blocks_count(s))
> fprintf(stderr, _("warning: %llu blocks unused.\n\n"),
> ext2fs_blocks_count(&fs_param) - ext2fs_blocks_count(s));
> @@ -666,6 +669,7 @@ static void show_stats(ext2_filsys fs)
> s->s_blocks_per_group, s->s_clusters_per_group);
> printf(_("%u inodes per group\n"), s->s_inodes_per_group);
>
> +skip_details:
> if (fs->group_desc_count == 1) {
> printf("\n");
> return;
>
On Sat, 26 Apr 2014, Theodore Ts'o wrote:
> Date: Sat, 26 Apr 2014 20:00:31 -0400
> From: Theodore Ts'o <[email protected]>
> To: Ext4 Developers List <[email protected]>
> Cc: Theodore Ts'o <[email protected]>
> Subject: [PATCH 4/7] mke2fs: create a regular file if necessary
>
> This is useful when creating a filesystem for use with a VM, for
> example.
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> misc/mke2fs.c | 3 ++-
> misc/util.c | 20 ++++++++++++++++----
> misc/util.h | 1 +
> 3 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index fa61e7b..a2b1f65 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1749,7 +1749,8 @@ profile_error:
> if (optind < argc)
> usage();
>
> - if (!check_plausibility(device_name, 0, &is_device) && !force)
> + if (!check_plausibility(device_name, CREATE_FILE,
> + &is_device) && !force)
> proceed_question();
>
> check_mount(device_name, force, _("filesystem"));
> diff --git a/misc/util.c b/misc/util.c
> index 08ec761..f85942e 100644
> --- a/misc/util.c
> +++ b/misc/util.c
> @@ -13,6 +13,7 @@
> #define _LARGEFILE64_SOURCE
>
> #include "config.h"
> +#include <fcntl.h>
> #include <stdio.h>
> #include <string.h>
> #ifdef HAVE_ERRNO_H
> @@ -21,6 +22,7 @@
> #ifdef HAVE_LINUX_MAJOR_H
> #include <linux/major.h>
> #endif
> +#include <sys/types.h>
> #ifdef HAVE_SYS_STAT_H
> #include <sys/stat.h>
> #endif
> @@ -85,19 +87,29 @@ void proceed_question(void)
> */
> int check_plausibility(const char *device, int flags, int *ret_is_dev)
> {
> - int val, is_dev = 0;
> + int fd, is_dev = 0;
> ext2fs_struct_stat s;
> + int fl = O_RDONLY;
Good that it's read only, otherwise udev would rescan the block
device unnecessarily.
>
> - val = ext2fs_stat(device, &s);
> + if (flags & CREATE_FILE)
> + fl |= O_CREAT;
>
> - if(val == -1) {
> - fprintf(stderr, _("Could not stat %s --- %s\n"),
> + fd = open(device, fl, 0666);
> + if (fd < 0) {
> + fprintf(stderr, _("Could not open %s: %s\n"),
> device, error_message(errno));
> if (errno == ENOENT)
> fputs(_("\nThe device apparently does not exist; "
> "did you specify it correctly?\n"), stderr);
> exit(1);
> }
> +
> + if (ext2fs_fstat(fd, &s) < 0) {
> + perror("stat");
Maybe we can leave the old error printing code for consistency ?
fprintf(stderr, _("Could not stat %s --- %s\n"),
device, error_message(errno));
Otherwise it looks good.
Thanks!
-Lukas
> + exit(1);
> + }
> + close(fd);
> +
> if (S_ISBLK(s.st_mode))
> is_dev = 1;
> #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> diff --git a/misc/util.h b/misc/util.h
> index 867f4b0..b80d489 100644
> --- a/misc/util.h
> +++ b/misc/util.h
> @@ -19,6 +19,7 @@ extern char *journal_location_string;
> * Flags for check_plausibility()
> */
> #define CHECK_BLOCK_DEV 0x0001
> +#define CREATE_FILE 0x0002
>
> #ifndef HAVE_STRCASECMP
> extern int strcasecmp (char *s1, char *s2);
>
On Sat, 26 Apr 2014, Theodore Ts'o wrote:
> Date: Sat, 26 Apr 2014 20:00:33 -0400
> From: Theodore Ts'o <[email protected]>
> To: Ext4 Developers List <[email protected]>
> Cc: Theodore Ts'o <[email protected]>
> Subject: [PATCH 6/7] mke2fs: check for pre-existing file system
>
> Warn the system administrator if there is an existing file system on
> the block device, and give the administrator an opportunity to abort
> the mkfs operation.
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> misc/mke2fs.c | 4 +++-
> misc/util.c | 29 +++++++++++++++++++++++++++++
> misc/util.h | 1 +
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 799132a..3694ce5 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1753,7 +1753,9 @@ profile_error:
> profile_get_integer(profile, "options", "proceed_delay", 0, 5,
> &proceed_delay);
>
> - if (!check_plausibility(device_name, CREATE_FILE,
> + /* The isatty() test is so we don't break existing scripts */
> + if (!check_plausibility(device_name, CREATE_FILE |
> + (isatty(0) ? CHECK_FS_EXIST : 0),
> &is_device) && !force)
> proceed_question(proceed_delay);
>
> diff --git a/misc/util.c b/misc/util.c
> index afb0058..be16ebe 100644
> --- a/misc/util.c
> +++ b/misc/util.c
> @@ -113,6 +113,9 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev)
> int fd, is_dev = 0;
> ext2fs_struct_stat s;
> int fl = O_RDONLY;
> + blkid_cache cache = NULL;
> + char *fs_type = NULL;
> + char *fs_label = NULL;
>
> if (flags & CREATE_FILE)
> fl |= O_CREAT;
> @@ -148,6 +151,32 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev)
> return 0;
> }
>
> + if ((flags & CHECK_FS_EXIST) && blkid_get_cache(&cache, NULL) >= 0) {
> + fs_type = blkid_get_tag_value(cache, "TYPE", device);
> + if (fs_type)
> + fs_label = blkid_get_tag_value(cache, "LABEL", device);
> + blkid_put_cache(cache);
> + }
> +
> + if (fs_type) {
> + if (fs_label)
> + printf(_("%s contains a %s file system "
> + "labelled '%s'\n"), device, fs_type, fs_label);
> + else
> + printf(_("%s contains a %s file system\n"), device,
> + fs_type);
> + free(fs_type);
> + free(fs_label);
> + return 0;
> + }
> +
> + /*
> + * We should eventually replace this with a test for the
> + * presence of a partition table. Unfortunately the blkid
> + * library doesn't test for partition tabels, and checking for
> + * valid GPT and MBR and possibly others isn't quite trivial.
> + */
That is not true. libblkid definitely can scan for partition or any
other signature for that matter (lvm, mdraid, ...) and we should
definitely utilize that.
It is true that "our" blkid library can not do that, but we should
actually stop using that and possibly remove it from e2fsprogs since
it's really out-of-date comparing to upstream libblkid from
util-linux.
I have some patches to do that, I can revive and resent it.
Thanks!
-Lukas
> +
> #ifdef HAVE_LINUX_MAJOR_H
> #ifndef MAJOR
> #define MAJOR(dev) ((dev)>>8)
> diff --git a/misc/util.h b/misc/util.h
> index 9de3fbf..745568e 100644
> --- a/misc/util.h
> +++ b/misc/util.h
> @@ -20,6 +20,7 @@ extern char *journal_location_string;
> */
> #define CHECK_BLOCK_DEV 0x0001
> #define CREATE_FILE 0x0002
> +#define CHECK_FS_EXIST 0x0004
>
> #ifndef HAVE_STRCASECMP
> extern int strcasecmp (char *s1, char *s2);
>
On Wed, 30 Apr 2014, Luk?? Czerner wrote:
> Date: Wed, 30 Apr 2014 13:50:13 +0200 (CEST)
> From: Luk?? Czerner <[email protected]>
> To: Theodore Ts'o <[email protected]>
> Cc: Ext4 Developers List <[email protected]>
> Subject: Re: [PATCH 6/7] mke2fs: check for pre-existing file system
>
> On Sat, 26 Apr 2014, Theodore Ts'o wrote:
>
> > Date: Sat, 26 Apr 2014 20:00:33 -0400
> > From: Theodore Ts'o <[email protected]>
> > To: Ext4 Developers List <[email protected]>
> > Cc: Theodore Ts'o <[email protected]>
> > Subject: [PATCH 6/7] mke2fs: check for pre-existing file system
> >
> > Warn the system administrator if there is an existing file system on
> > the block device, and give the administrator an opportunity to abort
> > the mkfs operation.
> >
> > Signed-off-by: "Theodore Ts'o" <[email protected]>
> > ---
> > misc/mke2fs.c | 4 +++-
> > misc/util.c | 29 +++++++++++++++++++++++++++++
> > misc/util.h | 1 +
> > 3 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> > index 799132a..3694ce5 100644
> > --- a/misc/mke2fs.c
> > +++ b/misc/mke2fs.c
> > @@ -1753,7 +1753,9 @@ profile_error:
> > profile_get_integer(profile, "options", "proceed_delay", 0, 5,
> > &proceed_delay);
> >
> > - if (!check_plausibility(device_name, CREATE_FILE,
> > + /* The isatty() test is so we don't break existing scripts */
> > + if (!check_plausibility(device_name, CREATE_FILE |
> > + (isatty(0) ? CHECK_FS_EXIST : 0),
> > &is_device) && !force)
> > proceed_question(proceed_delay);
> >
> > diff --git a/misc/util.c b/misc/util.c
> > index afb0058..be16ebe 100644
> > --- a/misc/util.c
> > +++ b/misc/util.c
> > @@ -113,6 +113,9 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev)
> > int fd, is_dev = 0;
> > ext2fs_struct_stat s;
> > int fl = O_RDONLY;
> > + blkid_cache cache = NULL;
> > + char *fs_type = NULL;
> > + char *fs_label = NULL;
> >
> > if (flags & CREATE_FILE)
> > fl |= O_CREAT;
> > @@ -148,6 +151,32 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev)
> > return 0;
> > }
> >
> > + if ((flags & CHECK_FS_EXIST) && blkid_get_cache(&cache, NULL) >= 0) {
> > + fs_type = blkid_get_tag_value(cache, "TYPE", device);
> > + if (fs_type)
> > + fs_label = blkid_get_tag_value(cache, "LABEL", device);
> > + blkid_put_cache(cache);
> > + }
> > +
> > + if (fs_type) {
> > + if (fs_label)
> > + printf(_("%s contains a %s file system "
> > + "labelled '%s'\n"), device, fs_type, fs_label);
> > + else
> > + printf(_("%s contains a %s file system\n"), device,
> > + fs_type);
> > + free(fs_type);
> > + free(fs_label);
> > + return 0;
> > + }
> > +
> > + /*
> > + * We should eventually replace this with a test for the
> > + * presence of a partition table. Unfortunately the blkid
> > + * library doesn't test for partition tabels, and checking for
> > + * valid GPT and MBR and possibly others isn't quite trivial.
> > + */
>
> That is not true. libblkid definitely can scan for partition or any
> other signature for that matter (lvm, mdraid, ...) and we should
> definitely utilize that.
Take a look at how xfs is doing this:
pr = blkid_new_probe_from_filename(device);
if (!pr)
goto out;
ret = blkid_probe_enable_partitions(pr, 1);
if (ret < 0)
goto out;
ret = blkid_do_fullprobe(pr);
if (ret < 0)
goto out;
/*
* Blkid returns 1 for nothing found and 0 when it finds a signature,
* but we want the exact opposite, so reverse the return value here.
*
* In addition print some useful diagnostics about what actually is
* on the device.
*/
if (ret) {
ret = 0;
goto out;
}
if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
fprintf(stderr,
_("%s: %s appears to contain an existing "
"filesystem (%s).\n"), progname, device, type);
} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
fprintf(stderr,
_("%s: %s appears to contain a partition "
"table (%s).\n"), progname, device, type);
} else {
fprintf(stderr,
_("%s: %s appears to contain something weird "
"according to blkid\n"), progname, device);
}
ret = 1;
we should probably also add blkid_probe_enable_superblocks(pr, 1)
even though it is enabled by default but just to better readability.
I think that we probably want to do something similar to this.
Thanks!
-Lukas
>
> It is true that "our" blkid library can not do that, but we should
> actually stop using that and possibly remove it from e2fsprogs since
> it's really out-of-date comparing to upstream libblkid from
> util-linux.
>
> I have some patches to do that, I can revive and resent it.
>
> Thanks!
> -Lukas
>
> > +
> > #ifdef HAVE_LINUX_MAJOR_H
> > #ifndef MAJOR
> > #define MAJOR(dev) ((dev)>>8)
> > diff --git a/misc/util.h b/misc/util.h
> > index 9de3fbf..745568e 100644
> > --- a/misc/util.h
> > +++ b/misc/util.h
> > @@ -20,6 +20,7 @@ extern char *journal_location_string;
> > */
> > #define CHECK_BLOCK_DEV 0x0001
> > #define CREATE_FILE 0x0002
> > +#define CHECK_FS_EXIST 0x0004
> >
> > #ifndef HAVE_STRCASECMP
> > extern int strcasecmp (char *s1, char *s2);
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Wed, Apr 30, 2014 at 01:22:52PM +0200, Lukáš Czerner wrote:
> On Sat, 26 Apr 2014, Theodore Ts'o wrote:
>
> > Date: Sat, 26 Apr 2014 20:00:34 -0400
> > From: Theodore Ts'o <[email protected]>
> > To: Ext4 Developers List <[email protected]>
> > Cc: Theodore Ts'o <[email protected]>
> > Subject: [PATCH 7/7] mke2fs: only print the low-level file system stats in
> > verbose mode
>
> While I kind of like this, because mke2fs is really quite verbose as
> it is. However I am afraid that this will break scripts for people.
Are there scripts that are really trying to parse the output of
mke2fs? The output hasn't really been _that_ stable. I'm sure we've
added stuff in the past, althoughg admittedly the this is the first
time that we would be removing stuff.
> Also there are actually some useful information in that output like
> block size, size of the file system and file system label if
> specified. Also maybe having UUID in there will be also useful.
The block sizs is pretty much always 4k, and the file system label is
only there if the user specified one on the command line. I can see
how the size and UUID might be useful, though.
How about if we just print the size and UUID?
- Ted
On Wed, Apr 30, 2014 at 02:21:46PM +0200, Lukáš Czerner wrote:
> > + fd = open(device, fl, 0666);
> > + if (fd < 0) {
> > + fprintf(stderr, _("Could not open %s: %s\n"),
> > device, error_message(errno));
> > if (errno == ENOENT)
> > fputs(_("\nThe device apparently does not exist; "
> > "did you specify it correctly?\n"), stderr);
> > exit(1);
> > }
> > +
> > + if (ext2fs_fstat(fd, &s) < 0) {
> > + perror("stat");
>
> Maybe we can leave the old error printing code for consistency ?
>
> fprintf(stderr, _("Could not stat %s --- %s\n"),
> device, error_message(errno));
>
> Otherwise it looks good.
Well, it's very rare that ext2fs_fstat() would fail in
practice. Previously the most common situation where ext2fs_stat()
would fail would be due to the file not existing or if there was a
permission denied error.
So I had modified the "Could not stat..." message to "Could not open",
since it would now be the open that failed, and if the file doesn't
exist, we're going to try to create the file first.
Hmm, it occurs to me if the user typo's the file name in and the user
specifies the size explicitly (i.e., "mke2fs /dev/scd3 2T) , it could
result in the the root file system filling up. I'm not sure that's
big of a deal, since the user can always control-C the mke2fs and then
delete the typo'ed file name. Do we think this is a real problem?
I'm not too worried...
- Ted
On Wed, Apr 30, 2014 at 01:50:13PM +0200, Lukáš Czerner wrote:
> > + /*
> > + * We should eventually replace this with a test for the
> > + * presence of a partition table. Unfortunately the blkid
> > + * library doesn't test for partition tabels, and checking for
> > + * valid GPT and MBR and possibly others isn't quite trivial.
> > + */
>
> That is not true. libblkid definitely can scan for partition or any
> other signature for that matter (lvm, mdraid, ...) and we should
> definitely utilize that.
libblkid scan scan for LVM and mdraid, sure. But it doesn't scan for
GPT or MBR partition labels:
% dpkg -S /sbin/blkid
util-linux: /sbin/blkid
% fdisk -l /dev/sda
Disk /dev/sda: 1000.2 GB, 1000204886016 bytes
255 heads, 63 sectors/track, 121601 cylinders, total 1953525168 sectors
Units = sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disk identifier: 0xae7b6b2b
Device Boot Start End Blocks Id System
/dev/sda1 * 2048 206847 102400 7 HPFS/NTFS/exFAT
/dev/sda2 206848 167772159 83782656 7 HPFS/NTFS/exFAT
/dev/sda3 167772160 482344959 157286400 83 Linux
/dev/sda4 482344960 1953525167 735590104 8e Linux LVM
% sudo /sbin/blkid /dev/sda
%
Ohhh... this may be debian's fault for having a pre-historic
util-linux.
But anyway, if util-linux does detect partition labels, then this
patch will catch the problem.
- Ted
On Wed, 30 Apr 2014, Theodore Ts'o wrote:
> Date: Wed, 30 Apr 2014 10:06:57 -0400
> From: Theodore Ts'o <[email protected]>
> To: Lukáš Czerner <[email protected]>
> Cc: Ext4 Developers List <[email protected]>
> Subject: Re: [PATCH 4/7] mke2fs: create a regular file if necessary
>
> On Wed, Apr 30, 2014 at 02:21:46PM +0200, Lukáš Czerner wrote:
> > > + fd = open(device, fl, 0666);
> > > + if (fd < 0) {
> > > + fprintf(stderr, _("Could not open %s: %s\n"),
> > > device, error_message(errno));
> > > if (errno == ENOENT)
> > > fputs(_("\nThe device apparently does not exist; "
> > > "did you specify it correctly?\n"), stderr);
> > > exit(1);
> > > }
> > > +
> > > + if (ext2fs_fstat(fd, &s) < 0) {
> > > + perror("stat");
> >
> > Maybe we can leave the old error printing code for consistency ?
> >
> > fprintf(stderr, _("Could not stat %s --- %s\n"),
> > device, error_message(errno));
> >
> > Otherwise it looks good.
>
> Well, it's very rare that ext2fs_fstat() would fail in
> practice. Previously the most common situation where ext2fs_stat()
> would fail would be due to the file not existing or if there was a
> permission denied error.
>
> So I had modified the "Could not stat..." message to "Could not open",
> since it would now be the open that failed, and if the file doesn't
> exist, we're going to try to create the file first.
>
> Hmm, it occurs to me if the user typo's the file name in and the user
> specifies the size explicitly (i.e., "mke2fs /dev/scd3 2T) , it could
> result in the the root file system filling up. I'm not sure that's
> big of a deal, since the user can always control-C the mke2fs and then
> delete the typo'ed file name. Do we think this is a real problem?
> I'm not too worried...
Oops, yes that would be nasty :) I'm not too worried either, but
I've done my share of typos as well, so I am not sure. And since
we're already asking a lot of questions anyway maybe we can ask
about this one as well ?
-Lukas
>
> - Ted
>
On Wed, Apr 30, 2014 at 04:14:16PM +0200, Lukáš Czerner wrote:
> > Hmm, it occurs to me if the user typo's the file name in and the user
> > specifies the size explicitly (i.e., "mke2fs /dev/scd3 2T) , it could
> > result in the the root file system filling up. I'm not sure that's
> > big of a deal, since the user can always control-C the mke2fs and then
> > delete the typo'ed file name. Do we think this is a real problem?
> > I'm not too worried...
>
> Oops, yes that would be nasty :) I'm not too worried either, but
> I've done my share of typos as well, so I am not sure. And since
> we're already asking a lot of questions anyway maybe we can ask
> about this one as well ?
The problem is if we do this, then scripts will do "mke2fs -F ..." to
avoid the query, and I'd really like to avoid training script authors
to do this. It undoes the point of some of the other patches in this
patch series.
We could have some proceed_questions() fail to "yes", and some fail to
"no", but I'm wondering if we really want to go to that level of
complexity....
- Ted
On Wed, 30 Apr 2014, Theodore Ts'o wrote:
> Date: Wed, 30 Apr 2014 10:01:50 -0400
> From: Theodore Ts'o <[email protected]>
> To: Lukáš Czerner <[email protected]>
> Cc: Ext4 Developers List <[email protected]>
> Subject: Re: [PATCH 7/7] mke2fs: only print the low-level file system stats in
> verbose mode
>
> On Wed, Apr 30, 2014 at 01:22:52PM +0200, Lukáš Czerner wrote:
> > On Sat, 26 Apr 2014, Theodore Ts'o wrote:
> >
> > > Date: Sat, 26 Apr 2014 20:00:34 -0400
> > > From: Theodore Ts'o <[email protected]>
> > > To: Ext4 Developers List <[email protected]>
> > > Cc: Theodore Ts'o <[email protected]>
> > > Subject: [PATCH 7/7] mke2fs: only print the low-level file system stats in
> > > verbose mode
> >
> > While I kind of like this, because mke2fs is really quite verbose as
> > it is. However I am afraid that this will break scripts for people.
>
> Are there scripts that are really trying to parse the output of
> mke2fs? The output hasn't really been _that_ stable. I'm sure we've
> added stuff in the past, althoughg admittedly the this is the first
> time that we would be removing stuff.
Yes, that's what I worry about. Addition would most likely not break
grep, but removal will surely do. I am sure that there are admins
doing that.
It also does not help that other file systems are printing out
various "low level" information.
Despite the fact that parsing mkfs output is not the best thing to
do I am on the fence whether we want to break it or not.
>
> > Also there are actually some useful information in that output like
> > block size, size of the file system and file system label if
> > specified. Also maybe having UUID in there will be also useful.
>
> The block sizs is pretty much always 4k, and the file system label is
> only there if the user specified one on the command line. I can see
> how the size and UUID might be useful, though.
>
> How about if we just print the size and UUID?
Sounds good to me.
-Lukas
>
> - Ted
>
On Wed, 30 Apr 2014, Theodore Ts'o wrote:
> Date: Wed, 30 Apr 2014 10:18:15 -0400
> From: Theodore Ts'o <[email protected]>
> To: Lukáš Czerner <[email protected]>
> Cc: Ext4 Developers List <[email protected]>
> Subject: Re: [PATCH 4/7] mke2fs: create a regular file if necessary
>
> On Wed, Apr 30, 2014 at 04:14:16PM +0200, Lukáš Czerner wrote:
> > > Hmm, it occurs to me if the user typo's the file name in and the user
> > > specifies the size explicitly (i.e., "mke2fs /dev/scd3 2T) , it could
> > > result in the the root file system filling up. I'm not sure that's
> > > big of a deal, since the user can always control-C the mke2fs and then
> > > delete the typo'ed file name. Do we think this is a real problem?
> > > I'm not too worried...
> >
> > Oops, yes that would be nasty :) I'm not too worried either, but
> > I've done my share of typos as well, so I am not sure. And since
> > we're already asking a lot of questions anyway maybe we can ask
> > about this one as well ?
>
> The problem is if we do this, then scripts will do "mke2fs -F ..." to
> avoid the query, and I'd really like to avoid training script authors
> to do this. It undoes the point of some of the other patches in this
> patch series.
Yes, I do not like having to force people to use force either. Well,
there is a certain limit where we should go to correct user
mistakes and it seems that beyond that...
But having mkfs to print out the information that it's actually
creating the file is the least we can do.
-Lukas
>
> We could have some proceed_questions() fail to "yes", and some fail to
> "no", but I'm wondering if we really want to go to that level of
> complexity....
>
> - Ted
>
On Wed, Apr 30, 2014 at 04:35:12PM +0200, Lukáš Czerner wrote:
> But having mkfs to print out the information that it's actually
> creating the file is the least we can do.
Agreed, that sounds like a good compromise. I'll do that.
- Ted
On 4/26/14, 7:00 PM, Theodore Ts'o wrote:
> This is useful when creating a filesystem for use with a VM, for
> example.
Ok, backing up to here from the "print if we're creating a file" thread.
Considering the questions about typos in /dev/$FOO etc, why is it desirable
to create the file within mke2fs? If you're making it for a VM, wouldn't
you want to control the creation (i.e. fallocated vs. sparse, for example,
as well as permissions) and not just have things magically happen?
fallocate -l 8g myfile; mkfs.ext4 myfile
or
truncate -s 8589934592 myfile; mkfs.ext4 myfile
doesn't seem all that onerous.
-Eric
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> misc/mke2fs.c | 3 ++-
> misc/util.c | 20 ++++++++++++++++----
> misc/util.h | 1 +
> 3 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index fa61e7b..a2b1f65 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1749,7 +1749,8 @@ profile_error:
> if (optind < argc)
> usage();
>
> - if (!check_plausibility(device_name, 0, &is_device) && !force)
> + if (!check_plausibility(device_name, CREATE_FILE,
> + &is_device) && !force)
> proceed_question();
>
> check_mount(device_name, force, _("filesystem"));
> diff --git a/misc/util.c b/misc/util.c
> index 08ec761..f85942e 100644
> --- a/misc/util.c
> +++ b/misc/util.c
> @@ -13,6 +13,7 @@
> #define _LARGEFILE64_SOURCE
>
> #include "config.h"
> +#include <fcntl.h>
> #include <stdio.h>
> #include <string.h>
> #ifdef HAVE_ERRNO_H
> @@ -21,6 +22,7 @@
> #ifdef HAVE_LINUX_MAJOR_H
> #include <linux/major.h>
> #endif
> +#include <sys/types.h>
> #ifdef HAVE_SYS_STAT_H
> #include <sys/stat.h>
> #endif
> @@ -85,19 +87,29 @@ void proceed_question(void)
> */
> int check_plausibility(const char *device, int flags, int *ret_is_dev)
> {
> - int val, is_dev = 0;
> + int fd, is_dev = 0;
> ext2fs_struct_stat s;
> + int fl = O_RDONLY;
>
> - val = ext2fs_stat(device, &s);
> + if (flags & CREATE_FILE)
> + fl |= O_CREAT;
>
> - if(val == -1) {
> - fprintf(stderr, _("Could not stat %s --- %s\n"),
> + fd = open(device, fl, 0666);
> + if (fd < 0) {
> + fprintf(stderr, _("Could not open %s: %s\n"),
> device, error_message(errno));
> if (errno == ENOENT)
> fputs(_("\nThe device apparently does not exist; "
> "did you specify it correctly?\n"), stderr);
> exit(1);
> }
> +
> + if (ext2fs_fstat(fd, &s) < 0) {
> + perror("stat");
> + exit(1);
> + }
> + close(fd);
> +
> if (S_ISBLK(s.st_mode))
> is_dev = 1;
> #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> diff --git a/misc/util.h b/misc/util.h
> index 867f4b0..b80d489 100644
> --- a/misc/util.h
> +++ b/misc/util.h
> @@ -19,6 +19,7 @@ extern char *journal_location_string;
> * Flags for check_plausibility()
> */
> #define CHECK_BLOCK_DEV 0x0001
> +#define CREATE_FILE 0x0002
>
> #ifndef HAVE_STRCASECMP
> extern int strcasecmp (char *s1, char *s2);
>