| Message ID | 1398556834-31913-4-git-send-email-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Sat, 26 Apr 2014, Theodore Ts'o wrote: > Date: Sat, 26 Apr 2014 20:00:31 -0400 > From: Theodore Ts'o <tytso@mit.edu> > To: Ext4 Developers List <linux-ext4@vger.kernel.org> > Cc: Theodore Ts'o <tytso@mit.edu> > 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" <tytso@mit.edu> > --- > 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); > -- 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 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 -- 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 Wed, 30 Apr 2014, Theodore Ts'o wrote: > Date: Wed, 30 Apr 2014 10:06:57 -0400 > From: Theodore Ts'o <tytso@mit.edu> > To: Lukáš Czerner <lczerner@redhat.com> > Cc: Ext4 Developers List <linux-ext4@vger.kernel.org> > 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 -- 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 Wed, 30 Apr 2014, Theodore Ts'o wrote: > Date: Wed, 30 Apr 2014 10:18:15 -0400 > From: Theodore Ts'o <tytso@mit.edu> > To: Lukáš Czerner <lczerner@redhat.com> > Cc: Ext4 Developers List <linux-ext4@vger.kernel.org> > 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 -- 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/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" <tytso@mit.edu> > --- > 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); > -- 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 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);
This is useful when creating a filesystem for use with a VM, for example. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- misc/mke2fs.c | 3 ++- misc/util.c | 20 ++++++++++++++++---- misc/util.h | 1 + 3 files changed, 19 insertions(+), 5 deletions(-)