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.

Sign up for GitHub

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

rename: adjust all mount points #666

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+1 −18
Open

rename: adjust all mount points #666

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions usr/local/share/bastille/rename.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,24 +81,7 @@ update_fstab() {
# Update fstab to use the new name
FSTAB_CONFIG="${bastille_jailsdir}/${NEWNAME}/fstab"
if [ -f "${FSTAB_CONFIG}" ]; then
# Skip if fstab is empty, e.g newly created thick or clone jails
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you removing all these lines?

Copy link
Author

Choose a reason for hiding this comment

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

I don't have much familiarity with this code, I just fixed it to fit my use case, so let's go through it.

My proposal is to change occurrences of ${bastille_jailsdir}/${TARGET} into ${bastille_jailsdir}/${NEWNAME} in the fstab file. I think this is hardly debatable: we are renaming the jail ${TARGET} into the jail ${NEWNAME}, so all mount points that were pointing to the former now need to point to the latter.

So why did I remove all other lines? I think the old code was trying to match a particular mount point into fstab, namely, the ..../root/.bastille mountpoint. The original bug is that it left all other custom mount points alone, unrenamed.

So, my take is to sed them all, which makes the rest of the code useless.

The linprocfs/linsysfs special case should also be covered, although I don't understand why the leading dot in the string matched in sed -i '' "s|.${bastille_jailsdir}/${TARGET}/|${bastille_jailsdir}/${NEWNAME}/|" "${FSTAB_CONFIG}".

Hope this helps clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I see what you mean.
However, what happens if a jail is named the same as a directory in the path specified?

Say you have a directory mounted into a jail named test, and the jail is also named test?

What do you think would be the best way to solve this?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a problem for the whole of the bastille project. Rename.sh and clone.sh use the approach you do to rename fstab and jail.conf files.

I suppose we could document that you should try to never name your jails in a similar manner as your directories.

Copy link
Author

Choose a reason for hiding this comment

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

How would that match ${bastille_jailsdir}/${TARGET} or ${bastille_jailsdir}/${NEWNAME}? You'd have to have a path, say, /usr/local/bastille/jails/foo under bastlle_jailsdir, so your mount point could be:

/host/path /usr/local/bastille/jails/TARGET/usr/local/bastille/jails/TARGET nullfs ro 0 0

In this case, yes, my approach would rename both TARGET to NEW, while probably you only want to rename the first.

I'd say it's still safer than what we're doing now...

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the only place it's called is here. I'm good with this change, unless @cedwards has a good reason to retain the original code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tschettervictor did you test this code on your system also??

Copy link
Contributor

Choose a reason for hiding this comment

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

Still working on that...

Copy link
Contributor

Choose a reason for hiding this comment

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

My testing confirms that the old code does not rename the mount points at all.
However, I would keep the old code.

In the "clone.sh" function this line
sed -i '' "s|${bastille_jailsdir}/${TARGET}/root/|${bastille_jailsdir}/${NEWNAME}/root/|" "${FSTAB_CONFIG}"
is added, which is not present in the "rename.sh"

I would argue that we should just make the "rename" code identical to the "clone" code as they attempt to do the exact same thing.
The current code does in fact change the RELEASE mount point, but does not touch any other custom mount points.

Here is the code from the "clone.sh"

 update_fstab() {
    # Update fstab to use the new name
    FSTAB_CONFIG="${bastille_jailsdir}/${NEWNAME}/fstab"
    if [ -f "${FSTAB_CONFIG}" ]; then
        FSTAB_RELEASE=$(grep -owE '([1-9]{2,2})\.[0-9](-RELEASE|-RELEASE-i386|-RC[1-9]|-BETA[1-9]|-CURRENT)|([0-9]{1,2}(-stable-build-[0-9]{1,3}|-stable-LAST))|(current-build)-([0-9]{1,3})|(current-BUILD-LATEST)|([0-9]{1,2}-stable-BUILD-LATEST)' "${FSTAB_CONFIG}" | uniq)
        FSTAB_CURRENT=$(grep -w ".*/releases/.*/jails/${TARGET}/root/.bastille" "${FSTAB_CONFIG}")
        FSTAB_NEWCONF="${bastille_releasesdir}/${FSTAB_RELEASE} ${bastille_jailsdir}/${NEWNAME}/root/.bastille nullfs ro 0 0"
        if [ -n "${FSTAB_CURRENT}" ] && [ -n "${FSTAB_NEWCONF}" ]; then
            # If both variables are set, update as needed
            if ! grep -qw "${bastille_releasesdir}/${FSTAB_RELEASE}.*${bastille_jailsdir}/${NEWNAME}/root/.bastille" "${FSTAB_CONFIG}"; then
                sed -i '' "s|${FSTAB_CURRENT}|${FSTAB_NEWCONF}|" "${FSTAB_CONFIG}"
            fi
        fi
        # Update additional fstab paths with new jail path
        sed -i '' "s|${bastille_jailsdir}/${TARGET}/root/|${bastille_jailsdir}/${NEWNAME}/root/|" "${FSTAB_CONFIG}"
    fi
}

I would just add the last line here to the rename function, but keep the old code in place just in case.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the same time, the original code was trying to do something as simple as rename the entire RELEASE fstab, when it is far easier to just rename all occurrences of the old jail to the new jail.

Unless @cedwards was trying to do something specific with it...

if [ -s "${FSTAB_CONFIG}" ]; then
FSTAB_RELEASE=$(grep -owE '([1-9]{2,2})\.[0-9](-RELEASE|-RC[1-9])|([0-9]{1,2}-stable-build-[0-9]{1,3})|(current-build)-([0-9]{1,3})|(current-BUILD-LATEST)|([0-9]{1,2}-stable-BUILD-LATEST)|(current-BUILD-LATEST)' "${FSTAB_CONFIG}")
FSTAB_CURRENT=$(grep -w ".*/releases/.*/jails/${TARGET}/root/.bastille" "${FSTAB_CONFIG}")
FSTAB_NEWCONF="${bastille_releasesdir}/${FSTAB_RELEASE} ${bastille_jailsdir}/${NEWNAME}/root/.bastille nullfs ro 0 0"
if [ -n "${FSTAB_CURRENT}" ] && [ -n "${FSTAB_NEWCONF}" ]; then
# If both variables are set, update as needed
if ! grep -qw "${bastille_releasesdir}/${FSTAB_RELEASE}.*${bastille_jailsdir}/${NEWNAME}/root/.bastille" "${FSTAB_CONFIG}"; then
sed -i '' "s|${FSTAB_CURRENT}|${FSTAB_NEWCONF}|" "${FSTAB_CONFIG}"
fi
fi

# Update linuxjail fstab name entries
# Search for either linprocfs/linsysfs, if true assume is a linux jail
if grep -qwE "linprocfs|linsysfs" "${FSTAB_CONFIG}"; then
sed -i '' "s|.${bastille_jailsdir}/${TARGET}/|${bastille_jailsdir}/${NEWNAME}/|" "${FSTAB_CONFIG}"
fi
fi
sed -i '' "s|${bastille_jailsdir}/${TARGET}|${bastille_jailsdir}/${NEWNAME}|g" "${FSTAB_CONFIG}"
fi
}

Expand Down