| Message ID | 1399295044-24489-3-git-send-email-tytso@mit.edu |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, 5 May 2014, Theodore Ts'o wrote: > Date: Mon, 5 May 2014 09:04:04 -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 3/3] mke2fs: check for a partition table and warn if present > > This supercedes the "whole disk" check, since it does a better job and > there are times when it is quite legitimate to want to use the whole > disk. > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > --- > configure | 2 +- > configure.in | 1 + > lib/config.h.in | 3 +++ > misc/util.c | 48 +++++++++++++++++++++++++++++++++++++++++------- > 4 files changed, 46 insertions(+), 8 deletions(-) > > diff --git a/configure b/configure > index 44664c3..6fe33f5 100755 > --- a/configure > +++ b/configure > @@ -11078,7 +11078,7 @@ if test "$ac_res" != no; then : > fi > > fi > -for ac_func in __secure_getenv backtrace blkid_probe_get_topology chflags fadvise64 fallocate fallocate64 fchown fdatasync fstat64 ftruncate64 futimes getcwd getdtablesize getmntinfo getpwuid_r getrlimit getrusage jrand48 llseek lseek64 mallinfo mbstowcs memalign mempcpy mmap msync nanosleep open64 pathconf posix_fadvise posix_fadvise64 posix_memalign prctl secure_getenv setmntent setresgid setresuid srandom stpcpy strcasecmp strdup strnlen strptime strtoull sync_file_range sysconf usleep utime valloc > +for ac_func in __secure_getenv backtrace blkid_probe_get_topology blkid_probe_enable_partitions chflags fadvise64 fallocate fallocate64 fchown fdatasync fstat64 ftruncate64 futimes getcwd getdtablesize getmntinfo getpwuid_r getrlimit getrusage jrand48 llseek lseek64 mallinfo mbstowcs memalign mempcpy mmap msync nanosleep open64 pathconf posix_fadvise posix_fadvise64 posix_memalign prctl secure_getenv setmntent setresgid setresuid srandom stpcpy strcasecmp strdup strnlen strptime strtoull sync_file_range sysconf usleep utime valloc > do : > as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh` > ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var" > diff --git a/configure.in b/configure.in > index e0e6d48..781b6f5 100644 > --- a/configure.in > +++ b/configure.in > @@ -1050,6 +1050,7 @@ AC_CHECK_FUNCS(m4_flatten([ > __secure_getenv > backtrace > blkid_probe_get_topology > + blkid_probe_enable_partitions > chflags > fadvise64 > fallocate > diff --git a/lib/config.h.in b/lib/config.h.in > index b575a5c..92b3c49 100644 > --- a/lib/config.h.in > +++ b/lib/config.h.in > @@ -55,6 +55,9 @@ > /* Define to 1 if you have the `backtrace' function. */ > #undef HAVE_BACKTRACE > > +/* Define to 1 if you have the `blkid_probe_enable_partitions' function. */ > +#undef HAVE_BLKID_PROBE_ENABLE_PARTITIONS > + > /* Define to 1 if you have the `blkid_probe_get_topology' function. */ > #undef HAVE_BLKID_PROBE_GET_TOPOLOGY > > diff --git a/misc/util.c b/misc/util.c > index d63e21b..d0e4c7e 100644 > --- a/misc/util.c > +++ b/misc/util.c > @@ -139,13 +139,50 @@ static void print_ext2_info(const char *device) > ext2fs_close(fs); > } > > +/* > + * return 1 if there is no partition table, 0 if a partition table is > + * detected, and -1 on an error. > + */ > +static int check_partition_table(const char *device) > +{ > +#ifdef HAVE_BLKID_PROBE_ENABLE_PARTITIONS > + blkid_probe pr; > + const char *value; > + int ret; > + > + pr = blkid_new_probe_from_filename(device); > + if (!pr) > + return -1; > + > + ret = blkid_probe_enable_partitions(pr, 1); > + if (ret < 0) > + goto errout; > + > + ret = blkid_do_fullprobe(pr); > + if (ret < 0) > + goto errout; > + > + ret = blkid_probe_lookup_value(pr, "PTTYPE", &value, NULL); > + if (ret == 0) > + fprintf(stderr, _("Found a %s partition table in %s\n"), > + value, device); > + else > + ret = 1; > + > +errout: > + blkid_free_probe(pr); > + return ret; > +#else > + return -1; > +#endif > +} > > /* > * return 1 if the device looks plausible, creating the file if necessary > */ > int check_plausibility(const char *device, int flags, int *ret_is_dev) > { > - int fd, is_dev = 0; > + int fd, ret, is_dev = 0; > ext2fs_struct_stat s; > int fl = O_RDONLY; > blkid_cache cache = NULL; > @@ -210,12 +247,9 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev) > 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. > - */ > + ret = check_partition_table(device); This can be actually used to check more than just partitions. So we can use this approach to check for all rather than having separate checks for file system signatures and partitions. Also in your check_partition_table() you do not disable probing for supeblocks even though you do not look to them afterwards so it's not a big deal. But again I think that we can use it to check for all signatures. Thanks! -Lukas > + if (ret >= 0) > + return ret; > > #ifdef HAVE_LINUX_MAJOR_H > #ifndef MAJOR > -- 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, May 05, 2014 at 03:52:05PM +0200, Lukáš Czerner wrote: > > + ret = check_partition_table(device); > > This can be actually used to check more than just partitions. So we > can use this approach to check for all rather than having separate > checks for file system signatures and partitions. The issue is that e2fsprogs gets compiled for systems other than just Linux. I don't want to be like the assholes who work on systemd and GNOME that simply screw over *BSD systems. This is why I keep our internal version of blkid in e2fsprogs, even if I do plan to use the system blkid by default for 1.43. If I had infinite amounts of free time, I'd backport the the new blkid_probe interfaces to our internal version of blkid, but since I don't, I prefer use the old interfaces for blkid as much as possible, since that's the path of least resistance in terms of continuing to support non-Linux users of e2fsprogs. > Also in your check_partition_table() you do not disable probing for > supeblocks even though you do not look to them afterwards so it's > not a big deal. But again I think that we can use it to check for > all signatures. How do you disable probing for superblocks with the new blkid interface? Cheers, - 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, 5 May 2014, Theodore Ts'o wrote: > Date: Mon, 5 May 2014 09:58:04 -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 3/3] mke2fs: check for a partition table and warn if > present > > On Mon, May 05, 2014 at 03:52:05PM +0200, Lukáš Czerner wrote: > > > + ret = check_partition_table(device); > > > > This can be actually used to check more than just partitions. So we > > can use this approach to check for all rather than having separate > > checks for file system signatures and partitions. > > The issue is that e2fsprogs gets compiled for systems other than just > Linux. I don't want to be like the assholes who work on systemd and > GNOME that simply screw over *BSD systems. This is why I keep our > internal version of blkid in e2fsprogs, even if I do plan to use the > system blkid by default for 1.43. > > If I had infinite amounts of free time, I'd backport the the new > blkid_probe interfaces to our internal version of blkid, but since I > don't, I prefer use the old interfaces for blkid as much as possible, > since that's the path of least resistance in terms of continuing to > support non-Linux users of e2fsprogs. Fair enough. But we should still make the use of system libblkid by default if you do not have any objections. Also it'll be great to mention that in the commit description that this is the reason why we still try to use the old approach. > > > Also in your check_partition_table() you do not disable probing for > > supeblocks even though you do not look to them afterwards so it's > > not a big deal. But again I think that we can use it to check for > > all signatures. > > How do you disable probing for superblocks with the new blkid interface? blkid_probe_enable_superblocks(pr, 0); Thanks! -Lukas > > Cheers, > > - Ted >
On Mon, May 05, 2014 at 04:11:41PM +0200, Lukáš Czerner wrote: > > Fair enough. But we should still make the use of system libblkid by > default if you do not have any objections. What I want to do is test to see if there is a system libblkid available at all, and if so, use it by default. I want to be able to use the internal libblkid for development testing purposes, but the goal is that when Andreas builds on MacOS, it should use the internal libblkid by default (since there is no system blkid), but on Linux systems, we should use the system blkid by default in the 1.43 branch. > Also it'll be great to mention that in the commit description that > this is the reason why we still try to use the old approach. Fair enough. Actually, we should probably put it in the code comments. > blkid_probe_enable_superblocks(pr, 0); Ok, thanks. - 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, May 05, 2014 at 10:20:56AM -0400, Theodore Ts'o wrote: > On Mon, May 05, 2014 at 04:11:41PM +0200, Lukáš Czerner wrote: > > > > Fair enough. But we should still make the use of system libblkid by > > default if you do not have any objections. > > What I want to do is test to see if there is a system libblkid > available at all, and if so, use it by default. I want to be able to > use the internal libblkid for development testing purposes, but the > goal is that when Andreas builds on MacOS, it should use the internal > libblkid by default (since there is no system blkid), but on Linux > systems, we should use the system blkid by default in the 1.43 branch. BTW, util-linux is not Linux only (although code portability is bonus rather than a primary goal :-). We have BSD, Solaris and GNU Hurd users. The plan for the next release (v2.25) is to support libs-only builds (for example build and install libblkid and libuuid, but nothing else). Karel
On Tue, 6 May 2014, Karel Zak wrote: > Date: Tue, 6 May 2014 14:20:31 +0200 > From: Karel Zak <kzak@redhat.com> > To: Theodore Ts'o <tytso@mit.edu> > Cc: Lukáš Czerner <lczerner@redhat.com>, > Ext4 Developers List <linux-ext4@vger.kernel.org> > Subject: Re: [PATCH 3/3] mke2fs: check for a partition table and warn if > present > > On Mon, May 05, 2014 at 10:20:56AM -0400, Theodore Ts'o wrote: > > On Mon, May 05, 2014 at 04:11:41PM +0200, Lukáš Czerner wrote: > > > > > > Fair enough. But we should still make the use of system libblkid by > > > default if you do not have any objections. > > > > What I want to do is test to see if there is a system libblkid > > available at all, and if so, use it by default. I want to be able to > > use the internal libblkid for development testing purposes, but the > > goal is that when Andreas builds on MacOS, it should use the internal > > libblkid by default (since there is no system blkid), but on Linux > > systems, we should use the system blkid by default in the 1.43 branch. > > BTW, util-linux is not Linux only (although code portability is bonus > rather than a primary goal :-). We have BSD, Solaris and GNU Hurd users. > > The plan for the next release (v2.25) is to support libs-only builds > (for example build and install libblkid and libuuid, but nothing > else). > > Karel Nice, so we're a step further to actually get rid of the libblkid in e2fsprogs tree ? :) -Lukas
diff --git a/configure b/configure index 44664c3..6fe33f5 100755 --- a/configure +++ b/configure @@ -11078,7 +11078,7 @@ if test "$ac_res" != no; then : fi fi -for ac_func in __secure_getenv backtrace blkid_probe_get_topology chflags fadvise64 fallocate fallocate64 fchown fdatasync fstat64 ftruncate64 futimes getcwd getdtablesize getmntinfo getpwuid_r getrlimit getrusage jrand48 llseek lseek64 mallinfo mbstowcs memalign mempcpy mmap msync nanosleep open64 pathconf posix_fadvise posix_fadvise64 posix_memalign prctl secure_getenv setmntent setresgid setresuid srandom stpcpy strcasecmp strdup strnlen strptime strtoull sync_file_range sysconf usleep utime valloc +for ac_func in __secure_getenv backtrace blkid_probe_get_topology blkid_probe_enable_partitions chflags fadvise64 fallocate fallocate64 fchown fdatasync fstat64 ftruncate64 futimes getcwd getdtablesize getmntinfo getpwuid_r getrlimit getrusage jrand48 llseek lseek64 mallinfo mbstowcs memalign mempcpy mmap msync nanosleep open64 pathconf posix_fadvise posix_fadvise64 posix_memalign prctl secure_getenv setmntent setresgid setresuid srandom stpcpy strcasecmp strdup strnlen strptime strtoull sync_file_range sysconf usleep utime valloc do : as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh` ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var" diff --git a/configure.in b/configure.in index e0e6d48..781b6f5 100644 --- a/configure.in +++ b/configure.in @@ -1050,6 +1050,7 @@ AC_CHECK_FUNCS(m4_flatten([ __secure_getenv backtrace blkid_probe_get_topology + blkid_probe_enable_partitions chflags fadvise64 fallocate diff --git a/lib/config.h.in b/lib/config.h.in index b575a5c..92b3c49 100644 --- a/lib/config.h.in +++ b/lib/config.h.in @@ -55,6 +55,9 @@ /* Define to 1 if you have the `backtrace' function. */ #undef HAVE_BACKTRACE +/* Define to 1 if you have the `blkid_probe_enable_partitions' function. */ +#undef HAVE_BLKID_PROBE_ENABLE_PARTITIONS + /* Define to 1 if you have the `blkid_probe_get_topology' function. */ #undef HAVE_BLKID_PROBE_GET_TOPOLOGY diff --git a/misc/util.c b/misc/util.c index d63e21b..d0e4c7e 100644 --- a/misc/util.c +++ b/misc/util.c @@ -139,13 +139,50 @@ static void print_ext2_info(const char *device) ext2fs_close(fs); } +/* + * return 1 if there is no partition table, 0 if a partition table is + * detected, and -1 on an error. + */ +static int check_partition_table(const char *device) +{ +#ifdef HAVE_BLKID_PROBE_ENABLE_PARTITIONS + blkid_probe pr; + const char *value; + int ret; + + pr = blkid_new_probe_from_filename(device); + if (!pr) + return -1; + + ret = blkid_probe_enable_partitions(pr, 1); + if (ret < 0) + goto errout; + + ret = blkid_do_fullprobe(pr); + if (ret < 0) + goto errout; + + ret = blkid_probe_lookup_value(pr, "PTTYPE", &value, NULL); + if (ret == 0) + fprintf(stderr, _("Found a %s partition table in %s\n"), + value, device); + else + ret = 1; + +errout: + blkid_free_probe(pr); + return ret; +#else + return -1; +#endif +} /* * return 1 if the device looks plausible, creating the file if necessary */ int check_plausibility(const char *device, int flags, int *ret_is_dev) { - int fd, is_dev = 0; + int fd, ret, is_dev = 0; ext2fs_struct_stat s; int fl = O_RDONLY; blkid_cache cache = NULL; @@ -210,12 +247,9 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev) 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. - */ + ret = check_partition_table(device); + if (ret >= 0) + return ret; #ifdef HAVE_LINUX_MAJOR_H #ifndef MAJOR
This supercedes the "whole disk" check, since it does a better job and there are times when it is quite legitimate to want to use the whole disk. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- configure | 2 +- configure.in | 1 + lib/config.h.in | 3 +++ misc/util.c | 48 +++++++++++++++++++++++++++++++++++++++++------- 4 files changed, 46 insertions(+), 8 deletions(-)