| Message ID | 201302120125.AA00019@FM-323941448.jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 2/11/13 7:25 PM, Tsutomu Itoh wrote: > Currently, the following commands succeed. > > # cat /proc/swaps > Filename Type Size Used Priority > /dev/sda3 partition 8388604 0 -1 > /dev/sdc8 partition 9765884 0 -2 > # mkfs.btrfs /dev/sdc8 > > WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL > WARNING! - see http://btrfs.wiki.kernel.org before using > > fs created label (null) on /dev/sdc8 > nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB > Btrfs v0.20-rc1-165-g82ac345 > # btrfs fi sh /dev/sdc8 > Label: none uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2 > Total devices 1 FS bytes used 28.00KB > devid 1 size 9.31GB used 989.62MB path /dev/sdc8 > > Btrfs v0.20-rc1-165-g82ac345 > # > > But we should check out the swap device. So fixed it. I guess it's nice to parse /proc/swaps to be able to offer the helpful error message in this case. (though I wonder how long /proc/swaps will be available, and in this format? Does it count as ABI?) Your implementation looks just like the one in e2fsprogs, so it should work fine. But I also wonder if overall it would be safest to open the device O_EXCL, which would fail with EBUSY if it were in use for swap, or mounted, or opened O_EXCL by another process for any other reason: [root@host tmp]# cat /proc/swaps Filename Type Size Used Priority /dev/sda3 partition 2048280 822616 -1 [root@host tmp]# strace -e open ./test open("/etc/ld.so.cache", O_RDONLY) = 3 open("/lib64/libc.so.6", O_RDONLY) = 3 open("/dev/sda3", O_RDWR|O_EXCL) = -1 EBUSY (Device or resource busy) open: Device or resource busy -Eric > Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> > --- > (this patch is based on Chris's raid56-experimental branch) > --- > mkfs.c | 18 ++++++++++++++++++ > utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > utils.h | 1 + > 3 files changed, 68 insertions(+) > > diff --git a/mkfs.c b/mkfs.c > index 2d3c2af..fdc3373 100644 > --- a/mkfs.c > +++ b/mkfs.c > @@ -1366,6 +1366,15 @@ int main(int ac, char **av) > > if (source_dir == 0) { > file = av[optind++]; > + ret = is_swap_device(file); > + if (ret < 0) { > + fprintf(stderr, "error checking %s status\n", file); > + exit(1); > + } > + if (ret == 1) { > + fprintf(stderr, "%s is a swap device\n", file); > + exit(1); > + } > ret = check_mounted(file); > if (ret < 0) { > fprintf(stderr, "error checking %s mount status\n", file); > @@ -1461,6 +1470,15 @@ int main(int ac, char **av) > int old_mixed = mixed; > > file = av[optind++]; > + ret = is_swap_device(file); > + if (ret < 0) { > + fprintf(stderr, "error checking %s status\n", file); > + exit(1); > + } > + if (ret == 1) { > + fprintf(stderr, "%s is a swap device\n", file); > + exit(1); > + } > ret = check_mounted(file); > if (ret < 0) { > fprintf(stderr, "error checking %s mount status\n", > diff --git a/utils.c b/utils.c > index f9ee812..0c551a0 100644 > --- a/utils.c > +++ b/utils.c > @@ -1386,3 +1386,52 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, > > return 0; > } > + > +/* > + * Checks if the swap device or not. > + * Returns 1 if the swap device, < 0 on error or 0 if not the swap device. > + */ > +int is_swap_device(const char *file) > +{ > + FILE *f; > + struct stat st_buf; > + char buf[1024]; > + char *cp; > + dev_t rdev; > + int ret = 0; > + > + if (stat(file, &st_buf) < 0) > + return -errno; > + if (!S_ISBLK(st_buf.st_mode)) > + return 0; > + > + rdev = st_buf.st_rdev; > + > + if ((f = fopen("/proc/swaps", "r")) == NULL) > + return -errno; > + > + /* skip the first line */ > + if (fgets(buf, sizeof(buf), f) == NULL) > + goto out; > + > + while (fgets(buf, sizeof(buf), f) != NULL) { > + if ((cp = strchr(buf, ' ')) != NULL) > + *cp = 0; > + if ((cp = strchr(buf, '\t')) != NULL) > + *cp = 0; > + if (strcmp(file, buf) == 0) { > + ret = 1; > + break; > + } > + if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) && > + rdev == st_buf.st_rdev) { > + ret = 1; > + break; > + } > + } > + > +out: > + fclose(f); > + > + return ret; > +} > diff --git a/utils.h b/utils.h > index bbcaf6a..60a0fea 100644 > --- a/utils.h > +++ b/utils.h > @@ -55,6 +55,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, > struct btrfs_ioctl_dev_info_args **di_ret); > > char *__strncpy__null(char *dest, const char *src, size_t n); > +int is_swap_device(const char *file); > /* Helper to always get proper size of the destination string */ > #define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest)) > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Eric, Thanks for your comment. On 2013/02/12 13:22, Eric Sandeen wrote: > On 2/11/13 7:25 PM, Tsutomu Itoh wrote: >> Currently, the following commands succeed. >> >> # cat /proc/swaps >> Filename Type Size Used Priority >> /dev/sda3 partition 8388604 0 -1 >> /dev/sdc8 partition 9765884 0 -2 >> # mkfs.btrfs /dev/sdc8 >> >> WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL >> WARNING! - see http://btrfs.wiki.kernel.org before using >> >> fs created label (null) on /dev/sdc8 >> nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB >> Btrfs v0.20-rc1-165-g82ac345 >> # btrfs fi sh /dev/sdc8 >> Label: none uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2 >> Total devices 1 FS bytes used 28.00KB >> devid 1 size 9.31GB used 989.62MB path /dev/sdc8 >> >> Btrfs v0.20-rc1-165-g82ac345 >> # >> >> But we should check out the swap device. So fixed it. > > I guess it's nice to parse /proc/swaps to be able to offer the > helpful error message in this case. (though I wonder how long > /proc/swaps will be available, and in this format? Does it count > as ABI?) Umm, I don't know how long /proc/swaps will be available, too... > > Your implementation looks just like the one in e2fsprogs, so it should > work fine. Yes. > > But I also wonder if overall it would be safest to open the device O_EXCL, > which would fail with EBUSY if it were in use for swap, or mounted, > or opened O_EXCL by another process for any other reason: But details of the error cannot be notified when O_EXCL is used. and, after is_swap_device(), check_mounted() check state of the mount or not. So, I chose this one. (read /proc/swaps) Thanks, Tsutomu > > [root@host tmp]# cat /proc/swaps > Filename Type Size Used Priority > /dev/sda3 partition 2048280 822616 -1 > > [root@host tmp]# strace -e open ./test > open("/etc/ld.so.cache", O_RDONLY) = 3 > open("/lib64/libc.so.6", O_RDONLY) = 3 > open("/dev/sda3", O_RDWR|O_EXCL) = -1 EBUSY (Device or resource busy) > open: Device or resource busy > > -Eric > >> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> >> --- >> (this patch is based on Chris's raid56-experimental branch) >> --- >> mkfs.c | 18 ++++++++++++++++++ >> utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >> utils.h | 1 + >> 3 files changed, 68 insertions(+) >> >> diff --git a/mkfs.c b/mkfs.c >> index 2d3c2af..fdc3373 100644 >> --- a/mkfs.c >> +++ b/mkfs.c >> @@ -1366,6 +1366,15 @@ int main(int ac, char **av) >> >> if (source_dir == 0) { >> file = av[optind++]; >> + ret = is_swap_device(file); >> + if (ret < 0) { >> + fprintf(stderr, "error checking %s status\n", file); >> + exit(1); >> + } >> + if (ret == 1) { >> + fprintf(stderr, "%s is a swap device\n", file); >> + exit(1); >> + } >> ret = check_mounted(file); >> if (ret < 0) { >> fprintf(stderr, "error checking %s mount status\n", file); >> @@ -1461,6 +1470,15 @@ int main(int ac, char **av) >> int old_mixed = mixed; >> >> file = av[optind++]; >> + ret = is_swap_device(file); >> + if (ret < 0) { >> + fprintf(stderr, "error checking %s status\n", file); >> + exit(1); >> + } >> + if (ret == 1) { >> + fprintf(stderr, "%s is a swap device\n", file); >> + exit(1); >> + } >> ret = check_mounted(file); >> if (ret < 0) { >> fprintf(stderr, "error checking %s mount status\n", >> diff --git a/utils.c b/utils.c >> index f9ee812..0c551a0 100644 >> --- a/utils.c >> +++ b/utils.c >> @@ -1386,3 +1386,52 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, >> >> return 0; >> } >> + >> +/* >> + * Checks if the swap device or not. >> + * Returns 1 if the swap device, < 0 on error or 0 if not the swap device. >> + */ >> +int is_swap_device(const char *file) >> +{ >> + FILE *f; >> + struct stat st_buf; >> + char buf[1024]; >> + char *cp; >> + dev_t rdev; >> + int ret = 0; >> + >> + if (stat(file, &st_buf) < 0) >> + return -errno; >> + if (!S_ISBLK(st_buf.st_mode)) >> + return 0; >> + >> + rdev = st_buf.st_rdev; >> + >> + if ((f = fopen("/proc/swaps", "r")) == NULL) >> + return -errno; >> + >> + /* skip the first line */ >> + if (fgets(buf, sizeof(buf), f) == NULL) >> + goto out; >> + >> + while (fgets(buf, sizeof(buf), f) != NULL) { >> + if ((cp = strchr(buf, ' ')) != NULL) >> + *cp = 0; >> + if ((cp = strchr(buf, '\t')) != NULL) >> + *cp = 0; >> + if (strcmp(file, buf) == 0) { >> + ret = 1; >> + break; >> + } >> + if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) && >> + rdev == st_buf.st_rdev) { >> + ret = 1; >> + break; >> + } >> + } >> + >> +out: >> + fclose(f); >> + >> + return ret; >> +} >> diff --git a/utils.h b/utils.h >> index bbcaf6a..60a0fea 100644 >> --- a/utils.h >> +++ b/utils.h >> @@ -55,6 +55,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, >> struct btrfs_ioctl_dev_info_args **di_ret); >> >> char *__strncpy__null(char *dest, const char *src, size_t n); >> +int is_swap_device(const char *file); >> /* Helper to always get proper size of the destination string */ >> #define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest)) >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 12, 2013 at 10:25:23AM +0900, Tsutomu Itoh wrote: > Currently, the following commands succeed. > > # cat /proc/swaps > Filename Type Size Used Priority > /dev/sda3 partition 8388604 0 -1 > /dev/sdc8 partition 9765884 0 -2 > # mkfs.btrfs /dev/sdc8 if a swap device is backed by a file, mkfs succeeds: Filename Type Size Used Priority /dev/sda15 partition 4232016 0 -2 /mnt/swap file 10236 0 -3 $ mkfs.btrfs /mnt/swap WARNING! - Btrfs 0.20 IS EXPERIMENTAL WARNING! - see http://btrfs.wiki.kernel.org before using SMALL VOLUME: forcing mixed metadata/data groups Created a data/metadata chunk of size 1048576 fs created label (null) on /mnt/swap nodesize 4096 leafsize 4096 sectorsize 4096 size 10.00MB Btrfs 0.20 --- Please add check for this as well. thanks, david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/11/13 11:50 PM, Tsutomu Itoh wrote: > Hi, Eric, > > Thanks for your comment. > > On 2013/02/12 13:22, Eric Sandeen wrote: >> On 2/11/13 7:25 PM, Tsutomu Itoh wrote: >>> Currently, the following commands succeed. >>> >>> # cat /proc/swaps >>> Filename Type Size Used Priority >>> /dev/sda3 partition 8388604 0 -1 >>> /dev/sdc8 partition 9765884 0 -2 >>> # mkfs.btrfs /dev/sdc8 >>> >>> WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL >>> WARNING! - see http://btrfs.wiki.kernel.org before using >>> >>> fs created label (null) on /dev/sdc8 >>> nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB >>> Btrfs v0.20-rc1-165-g82ac345 >>> # btrfs fi sh /dev/sdc8 >>> Label: none uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2 >>> Total devices 1 FS bytes used 28.00KB >>> devid 1 size 9.31GB used 989.62MB path /dev/sdc8 >>> >>> Btrfs v0.20-rc1-165-g82ac345 >>> # >>> >>> But we should check out the swap device. So fixed it. >> >> I guess it's nice to parse /proc/swaps to be able to offer the > >> helpful error message in this case. (though I wonder how long >> /proc/swaps will be available, and in this format? Does it count >> as ABI?) > > Umm, I don't know how long /proc/swaps will be available, too... I guess it is good enough for e2fsprogs :) >> >> Your implementation looks just like the one in e2fsprogs, so it should >> work fine. > > Yes. > >> >> But I also wonder if overall it would be safest to open the device O_EXCL, >> which would fail with EBUSY if it were in use for swap, or mounted, >> or opened O_EXCL by another process for any other reason: > > But details of the error cannot be notified when O_EXCL is used. and, > after is_swap_device(), check_mounted() check state of the mount or not. > > So, I chose this one. (read /proc/swaps) Sure, I think your change is good. I just think perhaps mkfs should also try to open O_EXCL after all those other tests, as a last safety check. I can take a look at the code & send that patch if it seems to make sense. Thanks, -Eric > Thanks, > Tsutomu > >> >> [root@host tmp]# cat /proc/swaps >> Filename Type Size Used Priority >> /dev/sda3 partition 2048280 822616 -1 >> >> [root@host tmp]# strace -e open ./test >> open("/etc/ld.so.cache", O_RDONLY) = 3 >> open("/lib64/libc.so.6", O_RDONLY) = 3 >> open("/dev/sda3", O_RDWR|O_EXCL) = -1 EBUSY (Device or resource busy) >> open: Device or resource busy >> >> -Eric >> >>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> >>> --- >>> (this patch is based on Chris's raid56-experimental branch) >>> --- >>> mkfs.c | 18 ++++++++++++++++++ >>> utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >>> utils.h | 1 + >>> 3 files changed, 68 insertions(+) >>> >>> diff --git a/mkfs.c b/mkfs.c >>> index 2d3c2af..fdc3373 100644 >>> --- a/mkfs.c >>> +++ b/mkfs.c >>> @@ -1366,6 +1366,15 @@ int main(int ac, char **av) >>> >>> if (source_dir == 0) { >>> file = av[optind++]; >>> + ret = is_swap_device(file); >>> + if (ret < 0) { >>> + fprintf(stderr, "error checking %s status\n", file); >>> + exit(1); >>> + } >>> + if (ret == 1) { >>> + fprintf(stderr, "%s is a swap device\n", file); >>> + exit(1); >>> + } >>> ret = check_mounted(file); >>> if (ret < 0) { >>> fprintf(stderr, "error checking %s mount status\n", file); >>> @@ -1461,6 +1470,15 @@ int main(int ac, char **av) >>> int old_mixed = mixed; >>> >>> file = av[optind++]; >>> + ret = is_swap_device(file); >>> + if (ret < 0) { >>> + fprintf(stderr, "error checking %s status\n", file); >>> + exit(1); >>> + } >>> + if (ret == 1) { >>> + fprintf(stderr, "%s is a swap device\n", file); >>> + exit(1); >>> + } >>> ret = check_mounted(file); >>> if (ret < 0) { >>> fprintf(stderr, "error checking %s mount status\n", >>> diff --git a/utils.c b/utils.c >>> index f9ee812..0c551a0 100644 >>> --- a/utils.c >>> +++ b/utils.c >>> @@ -1386,3 +1386,52 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, >>> >>> return 0; >>> } >>> + >>> +/* >>> + * Checks if the swap device or not. >>> + * Returns 1 if the swap device, < 0 on error or 0 if not the swap device. >>> + */ >>> +int is_swap_device(const char *file) >>> +{ >>> + FILE *f; >>> + struct stat st_buf; >>> + char buf[1024]; >>> + char *cp; >>> + dev_t rdev; >>> + int ret = 0; >>> + >>> + if (stat(file, &st_buf) < 0) >>> + return -errno; >>> + if (!S_ISBLK(st_buf.st_mode)) >>> + return 0; >>> + >>> + rdev = st_buf.st_rdev; >>> + >>> + if ((f = fopen("/proc/swaps", "r")) == NULL) >>> + return -errno; >>> + >>> + /* skip the first line */ >>> + if (fgets(buf, sizeof(buf), f) == NULL) >>> + goto out; >>> + >>> + while (fgets(buf, sizeof(buf), f) != NULL) { >>> + if ((cp = strchr(buf, ' ')) != NULL) >>> + *cp = 0; >>> + if ((cp = strchr(buf, '\t')) != NULL) >>> + *cp = 0; >>> + if (strcmp(file, buf) == 0) { >>> + ret = 1; >>> + break; >>> + } >>> + if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) && >>> + rdev == st_buf.st_rdev) { >>> + ret = 1; >>> + break; >>> + } >>> + } >>> + >>> +out: >>> + fclose(f); >>> + >>> + return ret; >>> +} >>> diff --git a/utils.h b/utils.h >>> index bbcaf6a..60a0fea 100644 >>> --- a/utils.h >>> +++ b/utils.h >>> @@ -55,6 +55,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, >>> struct btrfs_ioctl_dev_info_args **di_ret); >>> >>> char *__strncpy__null(char *dest, const char *src, size_t n); >>> +int is_swap_device(const char *file); >>> /* Helper to always get proper size of the destination string */ >>> #define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest)) >>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/12/2013 02:25 AM, Tsutomu Itoh wrote: > Currently, the following commands succeed. > > # cat /proc/swaps > Filename Type Size Used Priority > /dev/sda3 partition 8388604 0 -1 > /dev/sdc8 partition 9765884 0 -2 > # mkfs.btrfs /dev/sdc8 > > WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL > WARNING! - see http://btrfs.wiki.kernel.org before using > > fs created label (null) on /dev/sdc8 > nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB > Btrfs v0.20-rc1-165-g82ac345 > # btrfs fi sh /dev/sdc8 > Label: none uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2 > Total devices 1 FS bytes used 28.00KB > devid 1 size 9.31GB used 989.62MB path /dev/sdc8 > > Btrfs v0.20-rc1-165-g82ac345 > # > > But we should check out the swap device. So fixed it. > > Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> > --- > (this patch is based on Chris's raid56-experimental branch) > --- > mkfs.c | 18 ++++++++++++++++++ > utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > utils.h | 1 + > 3 files changed, 68 insertions(+) > > diff --git a/mkfs.c b/mkfs.c > index 2d3c2af..fdc3373 100644 > --- a/mkfs.c > +++ b/mkfs.c > @@ -1366,6 +1366,15 @@ int main(int ac, char **av) > > if (source_dir == 0) { > file = av[optind++]; > + ret = is_swap_device(file); > + if (ret < 0) { > + fprintf(stderr, "error checking %s status\n", file); > + exit(1); > + } The fact that it is not possible to perform a check shouldn't prohibit to run a mkfs.btrfs. It is possible to add a switch to bypass this kind of checks ? We should allow the user to be not limited by the fact that the check fails. I am thinking to a "rescue" scenario like boot in a single mode where not al filesystem are mounted. I am referring to all the "safety" check not this one only. BR G.Baroncelli [...]
> > > > So, I chose this one. (read /proc/swaps) > > Sure, I think your change is good. I just think perhaps mkfs should also try > to open O_EXCL after all those other tests, as a last safety check. I think mkfs should first try an O_EXCL open. If that works it doesn't need to do any of this work to try and predict if the open will fail. After it fails it can poke around a bit to try and give nice context for why it failed. But it might not be able to because /proc/swaps is fundamentally unreliable. > >>> file = av[optind++]; > >>> + ret = is_swap_device(file); The input string is a CWD-realtive path. You'd at least want to use realpath() to get it to a canonical name. So it's not full of junk like "./" and "../../.." which won't be present in /proc/swaps. > >>> + char buf[1024]; Use PATH_MAX so it doesn't fail on absurd but allowed file names. (Where on earth does 1024 come from?) > >>> + if ((f = fopen("/proc/swaps", "r")) == NULL) > >>> + return -errno; As was pointed out, there's no reason this failure should stop mkfs. /proc/swaps might not be available or allowed and I should still be able to do an unpriviledged "./mkfs ./myfile; ./btrfs-debug-tree ./myfile". > >>> + if (strcmp(file, buf) == 0) { > >>> + ret = 1; > >>> + break; > >>> + } The command line path that lead to the inode might not be the path for the inode that is in /proc/swaps. Bind mounts, chroot, name spaces, hard links, etc, all make it possible -- though unlikely -- that mkfs simply won't be able to tell if the path names are related. Also, /proc/swaps escapes whitespace in file names. So you could be comparing "swap file" with "swap\040file". > >>> + if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) && > >>> + rdev == st_buf.st_rdev) { > >>> + ret = 1; > >>> + break; > >>> + } One possible alternative is to then try and open every swap file path to see if it points to the same inode as the path mkfs was given. But you might not have access to the paths and we're back to the unpriviledged failure case. - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, All, Thanks for advice. On 2013/02/13 5:57, Zach Brown wrote: >>> >>> So, I chose this one. (read /proc/swaps) >> >> Sure, I think your change is good. I just think perhaps mkfs should also try >> to open O_EXCL after all those other tests, as a last safety check. > > I think mkfs should first try an O_EXCL open. If that works it doesn't > need to do any of this work to try and predict if the open will fail. > > After it fails it can poke around a bit to try and give nice context for > why it failed. But it might not be able to because /proc/swaps is > fundamentally unreliable. Then, how should we do? I have no idea... > >>>>> file = av[optind++]; >>>>> + ret = is_swap_device(file); > > The input string is a CWD-realtive path. You'd at least want to use > realpath() to get it to a canonical name. So it's not full of junk like > "./" and "../../.." which won't be present in /proc/swaps. > >>>>> + char buf[1024]; > > Use PATH_MAX so it doesn't fail on absurd but allowed file names. > (Where on earth does 1024 come from?) > >>>>> + if ((f = fopen("/proc/swaps", "r")) == NULL) >>>>> + return -errno; > > As was pointed out, there's no reason this failure should stop mkfs. > /proc/swaps might not be available or allowed and I should still be able > to do an unpriviledged "./mkfs ./myfile; ./btrfs-debug-tree ./myfile". > >>>>> + if (strcmp(file, buf) == 0) { >>>>> + ret = 1; >>>>> + break; >>>>> + } > > The command line path that lead to the inode might not be the path for > the inode that is in /proc/swaps. Bind mounts, chroot, name spaces, > hard links, etc, all make it possible -- though unlikely -- that mkfs > simply won't be able to tell if the path names are related. OK. I agree. > > Also, /proc/swaps escapes whitespace in file names. So you could be > comparing "swap file" with "swap\040file". I had forgotten this. Thank you for pointing it out. > >>>>> + if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) && >>>>> + rdev == st_buf.st_rdev) { >>>>> + ret = 1; >>>>> + break; >>>>> + } > > One possible alternative is to then try and open every swap file path to > see if it points to the same inode as the path mkfs was given. But you > might not have access to the paths and we're back to the unpriviledged > failure case. I want to think about a new patch later. and, I will post it again if it seems to make a sense. Thanks, Tsutomu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >why it failed. But it might not be able to because /proc/swaps is > >fundamentally unreliable. > > Then, how should we do? I have no idea... Hmm. I think I'd do something like: - First always open with O_EXCL. If it succeeds then there's no reason to check /proc/swaps at all. (Maybe it doesn't need to try check_mounted() there either? Not sure if it's protecting against accidentally mounting mounted shared storage or not.) - Try stat()ing the /proc/swaps paths and the command line path. If they point to the same inode then print a helpful message that the open might have failed because the file is an active swap file. - Use realpath() to resolve the relative path into an absolute path. Copy it and escape control chars ("\n\t\\") with their \0xxx octal equivalents. If the mangled absolute path matches the path in /proc/swaps (without opening), print the helpful message. - At no point is failure of any of the /proc/swaps parsing fatal. It'd carry on ignoring errors until it doesnt have work to do. It'd only ever print the nice message when it finds a match. That seems reasonable to me. It costs nothing in the vast majority of invocations when nothing goes wrong, it doesn't *cause* problems, and it'd print helpful messages on boring normal systems when someone really does accidentally try and mkfs a swapfile. In very rare cases /proc/swaps won't be of any help. The user would only see the open failure. That's fine, it's just not worth worrying about. - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/02/14 6:58, Zach Brown wrote: >>> why it failed. But it might not be able to because /proc/swaps is >>> fundamentally unreliable. >> >> Then, how should we do? I have no idea... > > Hmm. I think I'd do something like: > > - First always open with O_EXCL. If it succeeds then there's no reason > to check /proc/swaps at all. (Maybe it doesn't need to try > check_mounted() there either? Not sure if it's protecting against > accidentally mounting mounted shared storage or not.) check_mounted() is necessary for multiple devices, I think. > > - Try stat()ing the /proc/swaps paths and the command line path. If they > point to the same inode then print a helpful message that the open > might have failed because the file is an active swap file. > > - Use realpath() to resolve the relative path into an absolute path. > Copy it and escape control chars ("\n\t\\") with their \0xxx octal > equivalents. If the mangled absolute path matches the path in > /proc/swaps (without opening), print the helpful message. I think realpath() is unnecessary if it checks it by using only stat() information. (if do not compare path) Am I misunderstanding anything? Thanks, Tsutomu > > - At no point is failure of any of the /proc/swaps parsing fatal. It'd > carry on ignoring errors until it doesnt have work to do. It'd only > ever print the nice message when it finds a match. > > That seems reasonable to me. It costs nothing in the vast majority of > invocations when nothing goes wrong, it doesn't *cause* problems, and > it'd print helpful messages on boring normal systems when someone really > does accidentally try and mkfs a swapfile. > > In very rare cases /proc/swaps won't be of any help. The user would > only see the open failure. That's fine, it's just not worth worrying > about. > > - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I think realpath() is unnecessary if it checks it by using only > stat() information. (if do not compare path) > > Am I misunderstanding anything? Sure, that'd be fine, but then you'd want to try unescaping the paths before stati()ng them. - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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/mkfs.c b/mkfs.c index 2d3c2af..fdc3373 100644 --- a/mkfs.c +++ b/mkfs.c @@ -1366,6 +1366,15 @@ int main(int ac, char **av) if (source_dir == 0) { file = av[optind++]; + ret = is_swap_device(file); + if (ret < 0) { + fprintf(stderr, "error checking %s status\n", file); + exit(1); + } + if (ret == 1) { + fprintf(stderr, "%s is a swap device\n", file); + exit(1); + } ret = check_mounted(file); if (ret < 0) { fprintf(stderr, "error checking %s mount status\n", file); @@ -1461,6 +1470,15 @@ int main(int ac, char **av) int old_mixed = mixed; file = av[optind++]; + ret = is_swap_device(file); + if (ret < 0) { + fprintf(stderr, "error checking %s status\n", file); + exit(1); + } + if (ret == 1) { + fprintf(stderr, "%s is a swap device\n", file); + exit(1); + } ret = check_mounted(file); if (ret < 0) { fprintf(stderr, "error checking %s mount status\n", diff --git a/utils.c b/utils.c index f9ee812..0c551a0 100644 --- a/utils.c +++ b/utils.c @@ -1386,3 +1386,52 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, return 0; } + +/* + * Checks if the swap device or not. + * Returns 1 if the swap device, < 0 on error or 0 if not the swap device. + */ +int is_swap_device(const char *file) +{ + FILE *f; + struct stat st_buf; + char buf[1024]; + char *cp; + dev_t rdev; + int ret = 0; + + if (stat(file, &st_buf) < 0) + return -errno; + if (!S_ISBLK(st_buf.st_mode)) + return 0; + + rdev = st_buf.st_rdev; + + if ((f = fopen("/proc/swaps", "r")) == NULL) + return -errno; + + /* skip the first line */ + if (fgets(buf, sizeof(buf), f) == NULL) + goto out; + + while (fgets(buf, sizeof(buf), f) != NULL) { + if ((cp = strchr(buf, ' ')) != NULL) + *cp = 0; + if ((cp = strchr(buf, '\t')) != NULL) + *cp = 0; + if (strcmp(file, buf) == 0) { + ret = 1; + break; + } + if ((stat(buf, &st_buf) == 0) && S_ISBLK(st_buf.st_mode) && + rdev == st_buf.st_rdev) { + ret = 1; + break; + } + } + +out: + fclose(f); + + return ret; +} diff --git a/utils.h b/utils.h index bbcaf6a..60a0fea 100644 --- a/utils.h +++ b/utils.h @@ -55,6 +55,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, struct btrfs_ioctl_dev_info_args **di_ret); char *__strncpy__null(char *dest, const char *src, size_t n); +int is_swap_device(const char *file); /* Helper to always get proper size of the destination string */ #define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest))
Currently, the following commands succeed. # cat /proc/swaps Filename Type Size Used Priority /dev/sda3 partition 8388604 0 -1 /dev/sdc8 partition 9765884 0 -2 # mkfs.btrfs /dev/sdc8 WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL WARNING! - see http://btrfs.wiki.kernel.org before using fs created label (null) on /dev/sdc8 nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB Btrfs v0.20-rc1-165-g82ac345 # btrfs fi sh /dev/sdc8 Label: none uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2 Total devices 1 FS bytes used 28.00KB devid 1 size 9.31GB used 989.62MB path /dev/sdc8 Btrfs v0.20-rc1-165-g82ac345 # But we should check out the swap device. So fixed it. Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> --- (this patch is based on Chris's raid56-experimental branch) --- mkfs.c | 18 ++++++++++++++++++ utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ utils.h | 1 + 3 files changed, 68 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html