| Message ID | 1380229204-32526-1-git-send-email-cmaiolino@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 9/26/13 4:00 PM, Carlos Maiolino wrote: > Several users use to run e2image over a mounted filesystem, providing > inconsistent, useless e2images. > This patch adds a warning in such cases, notifying the user and also adds a > force option making e2image able to run over Read-only filesystems. Good idea. ;) But I think it needs a manpage update as well. If you use ext2fs_check_if_mounted() can you find out whether it's mounted readonly, and allow it to proceed in that case? (But I suppose that only checks one mount point, I'm not sure?) -Eric > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > misc/Makefile.in | 2 +- > misc/e2image.c | 18 ++++++++++++++++-- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/misc/Makefile.in b/misc/Makefile.in > index 8a69855..2e20b25 100644 > --- a/misc/Makefile.in > +++ b/misc/Makefile.in > @@ -49,7 +49,7 @@ UUIDGEN_OBJS= uuidgen.o > UUIDD_OBJS= uuidd.o > DUMPE2FS_OBJS= dumpe2fs.o > BADBLOCKS_OBJS= badblocks.o > -E2IMAGE_OBJS= e2image.o > +E2IMAGE_OBJS= e2image.o ismounted.o > FSCK_OBJS= fsck.o base_device.o ismounted.o > BLKID_OBJS= blkid.o > FILEFRAG_OBJS= filefrag.o > diff --git a/misc/e2image.c b/misc/e2image.c > index 885a794..6f6329a 100644 > --- a/misc/e2image.c > +++ b/misc/e2image.c > @@ -49,6 +49,8 @@ extern int optind; > > #define QCOW_OFLAG_COPIED (1LL << 63) > > +/* ismounted.h */ > +extern int is_mounted(const char *file); > > const char * program_name = "e2image"; > char * device_name = NULL; > @@ -87,7 +89,7 @@ static int get_bits_from_size(size_t size) > > static void usage(void) > { > - fprintf(stderr, _("Usage: %s [-rsIQa] device image_file\n"), > + fprintf(stderr, _("Usage: %s [-rsIQaf] device image_file\n"), > program_name); > exit (1); > } > @@ -1255,6 +1257,7 @@ int main (int argc, char ** argv) > int qcow2_fd = 0; > int fd = 0; > int ret = 0; > + int ignore_mounted = 0; > struct stat st; > > #ifdef ENABLE_NLS > @@ -1269,7 +1272,7 @@ int main (int argc, char ** argv) > if (argc && *argv) > program_name = *argv; > add_error_table(&et_ext2_error_table); > - while ((c = getopt(argc, argv, "rsIQa")) != EOF) > + while ((c = getopt(argc, argv, "rsIQaf")) != EOF) > switch (c) { > case 'I': > flags |= E2IMAGE_INSTALL_FLAG; > @@ -1290,6 +1293,9 @@ int main (int argc, char ** argv) > case 'a': > all_data = 1; > break; > + case 'f': > + ignore_mounted = 1; > + break; > default: > usage(); > } > @@ -1305,6 +1311,14 @@ int main (int argc, char ** argv) > device_name = argv[optind]; > image_fn = argv[optind+1]; > > + if (is_mounted(device_name) && !ignore_mounted) { > + fprintf(stderr, "\nWarning: Running e2image on a mounted " > + "filesystem can result in an inconsistent " > + "image which will not be useful. Use -f " > + "option if you really want to do that.\n"); > + exit(1); > + } > + > if (flags & E2IMAGE_INSTALL_FLAG) { > install_image(device_name, image_fn, img_type); > exit (0); > -- 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 Thu, Sep 26, 2013 at 06:00:04PM -0300, Carlos Maiolino wrote: > Several users use to run e2image over a mounted filesystem, providing > inconsistent, useless e2images. > This patch adds a warning in such cases, notifying the user and also adds a > force option making e2image able to run over Read-only filesystems. It should be perfectly safe to run e2image on a read-only mounted file system option, so it's not obvious to me why the force option would be needed in that case. Also, if we are saving a "normal" (not a raw or qcow) e2image file, we are only backing up the statically located metadata blocks (i.e., superblock, block group descriptors, inode table, and allocation bitmaps). If we do this on a mounted file system, the e2image file is less useful, but I'm not sure I'd call it completely useless. If the goal is to backup critical metadata, it will do that just fine. So maybe it's worthy of a warning, but I'm not sure it should require a force option. If the user is trying to capture a raw or qcow image file, I agree that requiring that the file systme either be mounted read-only, not mounted at all, or that a force option be specified, makes sense. - 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 9/26/13 6:56 PM, Theodore Ts'o wrote: > On Thu, Sep 26, 2013 at 06:00:04PM -0300, Carlos Maiolino wrote: >> Several users use to run e2image over a mounted filesystem, providing >> inconsistent, useless e2images. >> This patch adds a warning in such cases, notifying the user and also adds a >> force option making e2image able to run over Read-only filesystems. > > It should be perfectly safe to run e2image on a read-only mounted file > system option, so it's not obvious to me why the force option would be > needed in that case. right now Carlos' test isn't checking for readonly; just mounted, right? But I think it should check for ro, and allow it by default in that case, I agree. > Also, if we are saving a "normal" (not a raw or qcow) e2image file, we > are only backing up the statically located metadata blocks (i.e., > superblock, block group descriptors, inode table, and allocation > bitmaps). If we do this on a mounted file system, the e2image file is > less useful, but I'm not sure I'd call it completely useless. If the > goal is to backup critical metadata, it will do that just fine. So > maybe it's worthy of a warning, but I'm not sure it should require a > force option. I asked Carlos to do this after getting the 2nd customer filesystem image in a week which was useless for triage due to having been run on a live, mounted fs... I fear that a warning would be ignored, but *shrug* - at least something so we have some hope of getting something useful out the other end of e2image -r or -Q... TBH I've never used a "normal" image; if you want to allow it to run on an RW filesystem that won't bother me at all. ;) > If the user is trying to capture a raw or qcow image file, I agree > that requiring that the file systme either be mounted read-only, not > mounted at all, or that a force option be specified, makes sense. cool. :) -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
Hey, sorry my late answer, I'm experimenting some new mutt configuration. > > If the user is trying to capture a raw or qcow image file, I agree > > that requiring that the file systme either be mounted read-only, not > > mounted at all, or that a force option be specified, makes sense. > > cool. :) > > -Eric I'd suggest then to need a force when using -r or -q on a RW image. In case of a normal image, just a warning but continue. Ted, as Eric said, I've got some useless images in the past too and just a warning won't really avoid the problem IMHO since, most of users don't even read what's being printed :-) so, request a force when getting at least a raw or QCOW2 image is very useful IMHO. any comments? > > > - Ted
On Thu, Sep 26, 2013 at 09:48:33PM -0300, Carlos Maiolino wrote: > > I'd suggest then to need a force when using -r or -q on a RW image. In case of a > normal image, just a warning but continue. (Sorry for not repsonding earlier; I was on vacation in at Yellowstone / Grand Teton National Park from Thursday through today.) Yes, that's what I was suggesting. Requiring force for the case of -r or -q, but just a warning otherwise. > Ted, as Eric said, I've got some useless images in the past too and just a > warning won't really avoid the problem IMHO since, most of users don't even read > what's being printed :-) so, request a force when getting at least a raw or > QCOW2 image is very useful IMHO. The useless images are primarily when we are trying to use -r or -q to get the dynamic metadata (i.e., the directory blocks and extent tree blocks). That's what we generally need when are doing debugging, and so yes, I'm fully in agreement with requiring force in the case of e2image -r and e2image -q. Regards, - 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
diff --git a/misc/Makefile.in b/misc/Makefile.in index 8a69855..2e20b25 100644 --- a/misc/Makefile.in +++ b/misc/Makefile.in @@ -49,7 +49,7 @@ UUIDGEN_OBJS= uuidgen.o UUIDD_OBJS= uuidd.o DUMPE2FS_OBJS= dumpe2fs.o BADBLOCKS_OBJS= badblocks.o -E2IMAGE_OBJS= e2image.o +E2IMAGE_OBJS= e2image.o ismounted.o FSCK_OBJS= fsck.o base_device.o ismounted.o BLKID_OBJS= blkid.o FILEFRAG_OBJS= filefrag.o diff --git a/misc/e2image.c b/misc/e2image.c index 885a794..6f6329a 100644 --- a/misc/e2image.c +++ b/misc/e2image.c @@ -49,6 +49,8 @@ extern int optind; #define QCOW_OFLAG_COPIED (1LL << 63) +/* ismounted.h */ +extern int is_mounted(const char *file); const char * program_name = "e2image"; char * device_name = NULL; @@ -87,7 +89,7 @@ static int get_bits_from_size(size_t size) static void usage(void) { - fprintf(stderr, _("Usage: %s [-rsIQa] device image_file\n"), + fprintf(stderr, _("Usage: %s [-rsIQaf] device image_file\n"), program_name); exit (1); } @@ -1255,6 +1257,7 @@ int main (int argc, char ** argv) int qcow2_fd = 0; int fd = 0; int ret = 0; + int ignore_mounted = 0; struct stat st; #ifdef ENABLE_NLS @@ -1269,7 +1272,7 @@ int main (int argc, char ** argv) if (argc && *argv) program_name = *argv; add_error_table(&et_ext2_error_table); - while ((c = getopt(argc, argv, "rsIQa")) != EOF) + while ((c = getopt(argc, argv, "rsIQaf")) != EOF) switch (c) { case 'I': flags |= E2IMAGE_INSTALL_FLAG; @@ -1290,6 +1293,9 @@ int main (int argc, char ** argv) case 'a': all_data = 1; break; + case 'f': + ignore_mounted = 1; + break; default: usage(); } @@ -1305,6 +1311,14 @@ int main (int argc, char ** argv) device_name = argv[optind]; image_fn = argv[optind+1]; + if (is_mounted(device_name) && !ignore_mounted) { + fprintf(stderr, "\nWarning: Running e2image on a mounted " + "filesystem can result in an inconsistent " + "image which will not be useful. Use -f " + "option if you really want to do that.\n"); + exit(1); + } + if (flags & E2IMAGE_INSTALL_FLAG) { install_image(device_name, image_fn, img_type); exit (0);
Several users use to run e2image over a mounted filesystem, providing inconsistent, useless e2images. This patch adds a warning in such cases, notifying the user and also adds a force option making e2image able to run over Read-only filesystems. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- misc/Makefile.in | 2 +- misc/e2image.c | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)