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

Jump to bottom

Overhauled 900_clone_users_and_groups.sh #1471

Merged
merged 1 commit into from
Sep 5, 2017
Merged

Overhauled 900_clone_users_and_groups.sh #1471

Show file tree
Hide file tree
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 12 additions & 7 deletions usr/share/rear/conf/default.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,21 +748,26 @@ EXCLUDE_MODULES=( scsi_debug )
# cf. the conf/Linux-ppc64.conf and conf/Linux-ppc64le.conf scripts:
FIRMWARE_FILES=()

# files/dirs to copy as-is (with tar)
# Files and directories to copy as-is (with tar) into the ReaR recovery system.
COPY_AS_IS=( $SHARE_DIR $VAR_DIR )
# things to exclude from the copy
# Things to exclude from the copy:
# /dev/oracleasm contains internal files generated by Oracle ASM upon boot.
# See https://github.com/rear/rear/issues/721 for more details
# /dev/mapper contains links to real /dev/dm-X files managed and generated by device mapper.
# Copying the content of this directory makes ReaR avoid the migration process.
COPY_AS_IS_EXCLUDE=( dev/shm dev/shm/\* dev/oracleasm dev/mapper dev/.udev $VAR_DIR/output/\* )

# users and groups to copy to the rescue system
# Users and groups to copy into the ReaR recovery system.
# Users and groups must exist in the original system (i.e. one cannot create new ones).
# Users must be specified with names as in /etc/passwd (not with numerical user IDs):
CLONE_USERS=()
CLONE_GROUPS=(group disk cdrom floppy tape audio video lp tty dialout kmem uucp ssh_keys plugdev )
# copy all available users and groups on the rescue system.
# this variable overrides CLONE_USERS and CLONE_GROUPS
CLONE_ALL_USERS_GROUPS=n
# Groups must be specified with names as in /etc/group (not with numerical group IDs):
CLONE_GROUPS=( group disk cdrom floppy tape audio video lp tty dialout kmem uucp ssh_keys plugdev )
# Have users and groups that exist directly in /etc/passwd and /etc/group also in the ReaR recovery system.
# Users and groups that exist indirectly (e.g. via NIS) are not copied into the ReaR recovery system.
# This variable overrides CLONE_USERS and CLONE_GROUPS with the users in /etc/passwd and the groups in /etc/group,
# see the usr/share/rear/rescue/default/900_clone_users_and_groups.sh script:
CLONE_ALL_USERS_GROUPS="no"

# SSH_ROOT_PASSWORD defines a root password to allow SSH connection whithout a public/private key pair
# Be aware, the password is saved in hashed MD5 format (do not forget the password after months:)
Expand Down
87 changes: 53 additions & 34 deletions usr/share/rear/rescue/default/900_clone_users_and_groups.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,60 @@
# copy required system users and groups to the rescue system

if [[ "$CLONE_ALL_USERS_GROUPS" =~ ^[yY1] ]]; then
CLONE_USERS=($(cut -d ':' -f '1' /etc/passwd))
CLONE_GROUPS=($(cut -d ':' -f '1' /etc/group))
# Copy users and groups to the ReaR recovery system:

if is_true "$CLONE_ALL_USERS_GROUPS" ; then
# In particular exclude a possible NIS extension line '+::::::'
CLONE_USERS=( $( grep -o '^[[:alnum:]_][^:]*' /etc/passwd ) )
# In particular exclude a possible NIS extension line '+:::'
CLONE_GROUPS=( $( grep -o '^[[:alnum:]_][^:]*' /etc/group ) )
fi

Log "Cloning users: ${CLONE_USERS[@]}"
for u in "${CLONE_USERS[@]}" ; do
# go over all users
if pwd=$(getent passwd "$u") ; then
# pwd="daemon:x:2:2:Daemon:/sbin:/bin/bash"
# if the user exists, add to the passwd in rescue system
# skip if this user exists already in the rescue system
grep -q "^$pwd:" $ROOTFS_DIR/etc/passwd && continue
echo "$pwd" >>$ROOTFS_DIR/etc/passwd
# strip gid from passwd line
pwd="${pwd#*:*:*:}"
gid=${pwd%%:*}
# add gid to groups to collect
CLONE_GROUPS=( "${CLONE_GROUPS[@]}" "$gid" )
else
Debug "WARNING: Could not collect user info for '$u'"
fi
done
local user=""
local passwd_entry=""
local groupID=""
local group_entry=""
local group=""

Log "Cloning groups: ${CLONE_GROUPS[@]}"
for g in "${CLONE_GROUPS[@]}" ; do
# go over all users
if grp=$(getent group "$g") ; then
# grp="daemon:x:2:"
# if the group exists, add to the group in rescue system
# skip if this user exists already in the rescue system
grep -q "^$grp" $ROOTFS_DIR/etc/group && continue
echo "$grp" >>$ROOTFS_DIR/etc/group
else
Debug "WARNING: Could not collect group info for '$g'"
fi
# For the 'test' one must have all array members as a single word i.e. "${name[*]}"
# because it should succeed when there is any non-empty array member, not necessarily the first one:
test "${CLONE_USERS[*]}" && Log "Cloning users: ${CLONE_USERS[@]}"
for user in "${CLONE_USERS[@]}" ; do
# Skip if the user exists already in the ReaR recovery system:
grep "^$user:" $ROOTFS_DIR/etc/passwd 1>&2 && continue
Copy link
Member

Choose a reason for hiding this comment

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

Better grep -q

Copy link
Member Author

Choose a reason for hiding this comment

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

No "grep -q" because that deletes information.
With "1>&2" I have the info in the log in any case
even if redirection of STDOUT gets again postponed.

Copy link
Member

Choose a reason for hiding this comment

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

If grep fails then there is no output in STDOUT in any case. STDERR, where grep would complain about actual errors, is in any case already in the log. So I don't understand what you gain by this.

If grep found the line then I see no value in printing it because it is just the line that this script added there previously. If you want to dump the content of it then better do it with a suitable heading.

# Skip if the user does not exist in the current system:
if ! passwd_entry="$( getent passwd $user )" ; then
Debug "Cannot clone $user because it does not exist"
continue
fi
# Prepare to also add the user's group to the CLONE_GROUPS array:
# passwd_entry="user:password:UID:GID:description:HOMEdirectory:shell
groupID="$( echo "$passwd_entry" | cut -d ':' -f '4' )"
Copy link
Member

Choose a reason for hiding this comment

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

You could use cut ... <<<"$passwd_entry" to manage without the pipe

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks!
I am not yet used to use '<<<'.

if ! group_entry="$( getent group $groupID )" ; then
Debug "Cannot clone $user because its group $groupID does not exist"
continue
fi
# Add the user to /etc/passwd in the ReaR recovery system:
echo "$passwd_entry" >>$ROOTFS_DIR/etc/passwd
# Add the user's group to the CLONE_GROUPS array (unless already there):
# group_entry="group:passwd:GID:userlist"
group="$( echo "$group_entry" | cut -d ':' -f '1' )"
# Skip if the group is already in the CLONE_GROUPS array:
IsInArray "$group" "${CLONE_GROUPS[@]}" && continue
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer here

if !  IsInArray "$group" "${CLONE_GROUPS[@]}" ; then
    CLONE_GROUPS=( "${CLONE_GROUPS[@]}" "$group" )
fi

to make it easier to read. I would use continue only to denote skipping the entire while block for a certain item.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer "return early return often", cf.
https://github.com/rear/rear/wiki/Coding-Style

Copy link
Member Author

Choose a reason for hiding this comment

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

For me "continue" means "continue with the next item"
and that is exactly what it should do in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it does that. However, the next developer who will add more stuff below will be easily fooled by that. That's why I find the use of continue in this case not optimum. With the if clause it is clear that the array check is related to the array addition. A developer can add code before or after that without any worries.

In other words: I prefer locally understandable logic over logic that jumps far away. I agree with the coding style piece you refer to but I don't agree that the generalization for while loops is a conclusion from that.

# Add the user's group to the CLONE_GROUPS array:
CLONE_GROUPS=( "${CLONE_GROUPS[@]}" "$group" )
done

# For the 'test' one must have all array members as a single word i.e. "${name[*]}"
# because it should succeed when there is any non-empty array member, not necessarily the first one:
test "${CLONE_GROUPS[*]}" && Log "Cloning groups: ${CLONE_GROUPS[@]}"
for group in "${CLONE_GROUPS[@]}" ; do
# Skip if the group exists already in the ReaR recovery system:
grep "^$group:" $ROOTFS_DIR/etc/group 1>&2 && continue
Copy link
Member

Choose a reason for hiding this comment

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

grep -q

# Skip if the group does not exist in the current system:
if ! group_entry="$( getent group $group )" ; then
Debug "Cannot clone $group because it does not exist"
continue
fi
# Add the group to /etc/group in the ReaR recovery system:
echo "$group_entry" >>$ROOTFS_DIR/etc/group
done