[Cluster-devel] [PATCH] libgfs2: Return more context from is_pathname_mounted and rename it
Andrew Price
anprice at redhat.com
Tue Nov 5 14:04:23 UTC 2013
On 05/11/13 11:46, Steven Whitehouse wrote:
> Hi,
>
> Since the path has already been transformed into an fd
This is a false assumption. For example, in fsck.gfs2's initialize() it
is called in order to find out whether the mount point (if any) is
read-only after open(path, O_RDWR | O_EXCL) fails so we don't have the
fd beforehand in that case. In the cases where we do have an fd it's not
guaranteed to be the correct one (e.g. user gives us the mount point but
we need to operate on the device).
>, we should be
> using the fd rather than sending the path to lgfs2_find_mnt in order to
> avoid any possible races and also issues wrt device names being
> different,
There's a dependency loop here:
1. To get the correct fd to work with we need to get the right mntent
2. To get the right mntent we need to know the stat info
3. To get the stat info without races we need to fstat the correct fd
(goto 1)
So I'm really not convinced that we can improve this code much more.
Even if there's a way to solve the loop without possible races, we'd
still need to send the path to lgfs2_find_mnt because we need to compare
it to the mount points and device names of each entry in /proc/mounts.
Btw, if we can solve it we should send a patch for e2fsck because it's
where the original is_pathname_mounted code came from and it's been
using the same algorithm for over ten years:
http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git/tree/lib/ext2fs/ismounted.c#n39
:)
Andy
> Steve.
>
> On Mon, 2013-11-04 at 15:38 +0000, Andrew Price wrote:
>> This one fell off my radar somehow. Reviews appreciated.
>>
>> Andy
>>
>> On 12/10/13 23:14, Andrew Price wrote:
>>> is_pathname_mounted is renamed lgfs2_find_mnt and now returns more
>>> context through the isdev and mnt parameters. It now only takes one path
>>> and returns whether it is the name of the device or the mount point
>>> using *isdev. It also sets *mnt to the struct mntent found by
>>> getmntent() so that the caller can check its contents itself.
>>>
>>> Callers have been updated and a bug where gfs2_grow wasn't working when
>>> the device name was provided by the user has been fixed.
>>>
>>> Signed-off-by: Andrew Price <anprice at redhat.com>
>>> ---
>>> gfs2/fsck/initialize.c | 9 ++++-----
>>> gfs2/libgfs2/libgfs2.h | 3 ++-
>>> gfs2/libgfs2/misc.c | 55 ++++++++++++++++++++++++++++++--------------------
>>> gfs2/mkfs/main_grow.c | 24 ++++++++++++++++------
>>> gfs2/mkfs/main_jadd.c | 23 ++++++++++++++++-----
>>> 5 files changed, 75 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
>>> index 6612fe7..1cc15ec 100644
>>> --- a/gfs2/fsck/initialize.c
>>> +++ b/gfs2/fsck/initialize.c
>>> @@ -1475,7 +1475,8 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
>>>
>>> sdp->device_fd = open(opts.device, open_flag);
>>> if (sdp->device_fd < 0) {
>>> - int is_mounted, ro;
>>> + int isdev;
>>> + struct mntent *mnt;
>>>
>>> if (open_flag == O_RDONLY || errno != EBUSY) {
>>> log_crit( _("Unable to open device: %s\n"),
>>> @@ -1490,18 +1491,16 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
>>> function checks for device as well. */
>>> strncpy(sdp->device_name, opts.device,
>>> sizeof(sdp->device_name));
>>> - sdp->path_name = sdp->device_name; /* This gets overwritten */
>>> - is_mounted = is_pathname_mounted(sdp->path_name, sdp->device_name, &ro);
>>> /* If the device is busy, but not because it's mounted, fail.
>>> This protects against cases where the file system is LVM
>>> and perhaps mounted on a different node. */
>>> - if (!is_mounted)
>>> + if (lgfs2_find_mnt(opts.device, &isdev, &mnt) || mnt == NULL)
>>> goto mount_fail;
>>> /* If the device is mounted, but not mounted RO, fail. This
>>> protects them against cases where the file system is
>>> mounted RW, but still allows us to check our own root
>>> file system. */
>>> - if (!ro)
>>> + if (!hasmntopt(mnt, MNTOPT_RO))
>>> goto mount_fail;
>>> /* The device is mounted RO, so it's likely our own root
>>> file system. We can only do so much to protect the users
>>> diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
>>> index f864a08..993f605 100644
>>> --- a/gfs2/libgfs2/libgfs2.h
>>> +++ b/gfs2/libgfs2/libgfs2.h
>>> @@ -12,6 +12,7 @@
>>> #include <linux/limits.h>
>>> #include <endian.h>
>>> #include <byteswap.h>
>>> +#include <mntent.h>
>>>
>>> #include <linux/gfs2_ondisk.h>
>>> #include "osi_list.h"
>>> @@ -714,7 +715,7 @@ extern int metafs_interrupted;
>>> extern int compute_heightsize(struct gfs2_sbd *sdp, uint64_t *heightsize,
>>> uint32_t *maxheight, uint32_t bsize1, int diptrs, int inptrs);
>>> extern int compute_constants(struct gfs2_sbd *sdp);
>>> -extern int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount);
>>> +extern int lgfs2_find_mnt(const char *path, int *isdev, struct mntent **mnt);
>>> extern int find_gfs2_meta(struct gfs2_sbd *sdp);
>>> extern int dir_exists(const char *dir);
>>> extern int mount_gfs2_meta(struct gfs2_sbd *sdp);
>>> diff --git a/gfs2/libgfs2/misc.c b/gfs2/libgfs2/misc.c
>>> index 7f500e6..6bc95a9 100644
>>> --- a/gfs2/libgfs2/misc.c
>>> +++ b/gfs2/libgfs2/misc.c
>>> @@ -102,20 +102,27 @@ int compute_constants(struct gfs2_sbd *sdp)
>>> return 0;
>>> }
>>>
>>> -int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
>>> +/**
>>> + * lgfs2_find_mnt - Find a mounted file system relating to a path.
>>> + * path: The path
>>> + * isdev: Will be non-zero if the path is the device or zero if the path is the mount directory
>>> + * mnt: When the return value is 0, this will either be NULL for "not found" or
>>> + * it will point to the found mount point.
>>> + * Returns 0 to indicate a successful scan of the mount points or non-zero on error with errno set.
>>> + * On success, *mnt will be NULL or a valid pointer as noted above.
>>> + */
>>> +int lgfs2_find_mnt(const char *path, int *isdev, struct mntent **mnt)
>>> {
>>> FILE *fp;
>>> - struct mntent *mnt;
>>> dev_t file_dev=0, file_rdev=0;
>>> ino_t file_ino=0;
>>> struct stat st_buf;
>>>
>>> - *ro_mount = 0;
>>> if ((fp = setmntent("/proc/mounts", "r")) == NULL) {
>>> perror("open: /proc/mounts");
>>> - return 0;
>>> + return 1;
>>> }
>>> - if (stat(path_name, &st_buf) == 0) {
>>> + if (stat(path, &st_buf) == 0) {
>>> if (S_ISBLK(st_buf.st_mode)) {
>>> #ifndef __GNU__ /* The GNU hurd is broken with respect to stat devices */
>>> file_rdev = st_buf.st_rdev;
>>> @@ -125,42 +132,46 @@ int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
>>> file_ino = st_buf.st_ino;
>>> }
>>> }
>>> - while ((mnt = getmntent (fp)) != NULL) {
>>> + while ((*mnt = getmntent(fp)) != NULL) {
>>> /* Check if they specified the device instead of mnt point */
>>> - if (strcmp(device_name, mnt->mnt_fsname) == 0) {
>>> - strcpy(path_name, mnt->mnt_dir); /* fix it */
>>> + if (strcmp(path, (*mnt)->mnt_fsname) == 0) {
>>> + *isdev = 1;
>>> break;
>>> }
>>> - if (strcmp(path_name, mnt->mnt_dir) == 0) {
>>> - strcpy(device_name, mnt->mnt_fsname); /* fix it */
>>> + if (strcmp(path, (*mnt)->mnt_dir) == 0) {
>>> + *isdev = 0;
>>> break;
>>> }
>>> - if (stat(mnt->mnt_fsname, &st_buf) == 0) {
>>> + if (stat((*mnt)->mnt_fsname, &st_buf) == 0) {
>>> if (S_ISBLK(st_buf.st_mode)) {
>>> #ifndef __GNU__
>>> - if (file_rdev && (file_rdev == st_buf.st_rdev))
>>> + if (file_rdev && (file_rdev == st_buf.st_rdev)) {
>>> + *isdev = 1;
>>> break;
>>> + }
>>> #endif /* __GNU__ */
>>> } else {
>>> if (file_dev && ((file_dev == st_buf.st_dev) &&
>>> - (file_ino == st_buf.st_ino)))
>>> + (file_ino == st_buf.st_ino))) {
>>> + *isdev = 0;
>>> break;
>>> + }
>>> }
>>> }
>>> }
>>> endmntent (fp);
>>> - if (mnt == NULL)
>>> - return 0;
>>> - if (stat(mnt->mnt_dir, &st_buf) < 0) {
>>> + if (*mnt == NULL)
>>> + return 0; /* Success. Answer is no. */
>>> + if (stat((*mnt)->mnt_dir, &st_buf) < 0) {
>>> if (errno == ENOENT)
>>> - return 0;
>>> + return 1;
>>> }
>>> /* Can't trust fstype because / has "rootfs". */
>>> - if (file_rdev && (st_buf.st_dev != file_rdev))
>>> - return 0;
>>> - if (hasmntopt(mnt, MNTOPT_RO))
>>> - *ro_mount = 1;
>>> - return 1; /* mounted */
>>> + if (file_rdev && (st_buf.st_dev != file_rdev)) {
>>> + errno = EINVAL;
>>> + return 1;
>>> + }
>>> + return 0; /* Success. Answer is yes. */
>>> }
>>>
>>> static int lock_for_admin(struct gfs2_sbd *sdp)
>>> diff --git a/gfs2/mkfs/main_grow.c b/gfs2/mkfs/main_grow.c
>>> index 5db91d9..4c8807e 100644
>>> --- a/gfs2/mkfs/main_grow.c
>>> +++ b/gfs2/mkfs/main_grow.c
>>> @@ -323,7 +323,6 @@ main_grow(int argc, char *argv[])
>>> int rgcount, rindex_fd;
>>> char rindex_name[PATH_MAX];
>>> int error = EXIT_SUCCESS;
>>> - int ro_mnt = 0;
>>>
>>> memset(sdp, 0, sizeof(struct gfs2_sbd));
>>> sdp->bsize = GFS2_DEFAULT_BSIZE;
>>> @@ -335,16 +334,29 @@ main_grow(int argc, char *argv[])
>>>
>>> while ((argc - optind) > 0) {
>>> int sane;
>>> + int isdev = 0;
>>> + struct mntent *mnt;
>>> struct rgrp_tree *last_rgrp;
>>>
>>> - strncpy(sdp->device_name, argv[optind], PATH_MAX - 1);
>>> - sdp->path_name = argv[optind++];
>>> -
>>> - if ((!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt))) {
>>> - perror(sdp->path_name);
>>> + if (lgfs2_find_mnt(argv[optind], &isdev, &mnt)) {
>>> + perror(argv[optind]);
>>> exit(EXIT_FAILURE);
>>> }
>>>
>>> + if (mnt == NULL) {
>>> + fprintf(stderr, "%s: not a mounted gfs2 file system\n", argv[optind++]);
>>> + continue;
>>> + }
>>> +
>>> + if (isdev) {
>>> + strncpy(sdp->device_name, argv[optind], PATH_MAX - 1);
>>> + sdp->path_name = mnt->mnt_dir;
>>> + } else {
>>> + strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
>>> + sdp->path_name = argv[optind];
>>> + }
>>> + optind++;
>>> +
>>> sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
>>> if (sdp->path_fd < 0){
>>> perror(sdp->path_name);
>>> diff --git a/gfs2/mkfs/main_jadd.c b/gfs2/mkfs/main_jadd.c
>>> index b6cd8e4..9240838 100644
>>> --- a/gfs2/mkfs/main_jadd.c
>>> +++ b/gfs2/mkfs/main_jadd.c
>>> @@ -490,8 +490,9 @@ add_j(struct gfs2_sbd *sdp)
>>> void main_jadd(int argc, char *argv[])
>>> {
>>> struct gfs2_sbd sbd, *sdp = &sbd;
>>> + struct mntent *mnt;
>>> unsigned int total;
>>> - int ro_mnt = 0;
>>> + int isdev;
>>>
>>> memset(sdp, 0, sizeof(struct gfs2_sbd));
>>> sdp->jsize = GFS2_DEFAULT_JSIZE;
>>> @@ -500,14 +501,26 @@ void main_jadd(int argc, char *argv[])
>>>
>>> decode_arguments(argc, argv, sdp);
>>> verify_arguments(sdp);
>>> -
>>> - sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
>>> - if (sdp->path_fd < 0){
>>> +
>>> + if (lgfs2_find_mnt(sdp->path_name, &isdev, &mnt)) {
>>> perror(sdp->path_name);
>>> exit(EXIT_FAILURE);
>>> }
>>>
>>> - if (!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt)) {
>>> + if (mnt == NULL) {
>>> + fprintf(stderr, "%s: not a mounted gfs2 file system\n", sdp->path_name);
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + if (isdev) {
>>> + strncpy(sdp->device_name, sdp->path_name, PATH_MAX - 1);
>>> + sdp->path_name = mnt->mnt_dir;
>>> + } else {
>>> + strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
>>> + }
>>> +
>>> + sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
>>> + if (sdp->path_fd < 0){
>>> perror(sdp->path_name);
>>> exit(EXIT_FAILURE);
>>> }
>>>
>>
>
>
More information about the Cluster-devel
mailing list