Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

umount: Add -d option to detach md devices #972

Closed
wants to merge 1 commit into from

Conversation

ricardobranco777
Copy link
Contributor

Add a -d option to umount(8) to detach a memory device. A similar option exists in Linux's umount(8).

Follow-up to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=114468

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's some first pass review

i did find a few things that need changing

sbin/umount/umount.8 Show resolved Hide resolved
sbin/umount/umount.8 Outdated Show resolved Hide resolved
typedef enum { FIND, REMOVE, CHECKUNIQUE } dowhat;

static struct addrinfo *nfshost_ai = NULL;
static int fflag, vflag;
static int dflag, fflag, vflag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is not for you, just in general:
all of these flags should be converted to bool at some point

sbin/umount/umount.c Outdated Show resolved Hide resolved
sbin/umount/umount.c Outdated Show resolved Hide resolved
sbin/umount/umount.c Outdated Show resolved Hide resolved
mdio.md_options = fflag ? MD_FORCE : 0;

if (strncmp(device, DEV_MD, sizeof(DEV_MD) - 1)) {
if (!multiple)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We checked this before entering the function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We checked this before entering the function

I don't see a proper check for DEV_MD.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, i meant for multiple

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, i meant for multiple

In this case we need the check to avoid a warning when using -a.

sbin/umount/umount.c Outdated Show resolved Hide resolved
(void)printf("%s: detached\n",
sfs->f_mntfromname);
} else if (!all)
return (-1);
Copy link
Member

@bsdimp bsdimp Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this means umount will return a failure code if -d is specified on non /dev/md backed devices. Is this what you want? And should the error checking be done before you unmount or after when you notice you can't detach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this means umount will return a failure code if -d is specified on non /dev/md backed devices. Is this what you want?

Yes, but only if the -a wasn't specified for obvious reasons.

@@ -70,24 +80,28 @@ int checkname (char *, char **);
int umountfs(struct statfs *sfs);
void usage (void);
int xdr_dir (XDR *, char *);
int md_detach(const char *);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only used in umount.c, it should be static... but all the other functions above it should be too, so consistency wins out over standard practice. Just fyi for next time.

@bsdimp
Copy link
Member

bsdimp commented Feb 3, 2024

OK. I think this is ready. The style script spotted a non-static function used only in umount... but it is consistent with the others, so I'm going to let the consistency rule trump the static rule.

@bsdimp bsdimp self-assigned this Feb 3, 2024
@bsdimp bsdimp closed this Feb 3, 2024
freebsd-git pushed a commit that referenced this pull request Feb 3, 2024
Reviewed by: imp
Pull Request: #972
dragonflybot pushed a commit to DragonFlyBSD/DragonFlyBSD that referenced this pull request Mar 22, 2024
The '-d' option tells umount(8) to detach the underlying vn(4) device if
the filesystem was mounted from it.  Note that vn(4) is a virtual disk
and can provides multiple filesystems, so the vn(4) detaching can only
succeed when all the filesystems are umounted.

For example:
$ vnconfig -c vn dfly.img
vn4
$ mount_msdos /dev/vn4s1 /mnt/dfly/boot
$ mount_ufs /dev/vn4s2a /mnt/dfly/root
$ umount -d /mnt/dfly/boot
umount: VNIOCDETACH: /dev/vn4: Device busy
umount: detach of /dev/vn4s1 failed
$ umount -d /mnt/dfly/root
(now vn4 is detached)

A similar option exists in Linux's and FreeBSD's umount(8).

GitHub PR: #24
See Also: freebsd/freebsd-src#972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants