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

Cosmetic fix for RHEL 6 and missing name_assign_type for LAN interfac… #2231

Closed
wants to merge 1 commit into from
Closed

Cosmetic fix for RHEL 6 and missing name_assign_type for LAN interfac… #2231

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
6 changes: 5 additions & 1 deletion usr/share/rear/lib/network-functions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,11 @@ function is_persistent_ethernet_name () {
local _name_assign_type="0"

[ -f "/sys/class/net/$_netif/name_assign_type" ] \
&& _name_assign_type=$(cat "/sys/class/net/$_netif/name_assign_type")
&& _name_assign_type=$(cat "/sys/class/net/$_netif/name_assign_type" 2>/dev/null)
Copy link
Member

@jsmeix jsmeix Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

To me that whole code part looks needlessly complicated
and works somewhat indirectly (cf. RFC 1925 item 6a)
and therefore it cannot work really fail safe.

Why testing for existence of a file when reading is intended
i.e. why 'test -f' when 'test -r' is needed?

Why testing "something else" at all and then blindly run a command
instead of just trying the actually wanted command
and checking the return code of the actual command?

By the way:

Why do we use variable names that start with an underscore?

There is no need to redirect STDERR to /dev/null because STDERR
is redirected to the log and the log is there to log in particular errors
so that we can see later by analyzing a log where things start to go wrong.
I see no reason to hide STDERR messages from the log, except exceptions
(e.g. when a command is needlessly oververbose) but then the
exceptional case where an error is hidden needs a comment
that explains the reason behind why STDERR is suppressed.

I think something like

if ! _name_assign_type=$( cat "/sys/class/net/$_netif/name_assign_type" ) ; then
    # On RHEL 6 "name_assign_type" exists but cannot be read
    # see https://github.com/rear/rear/pull/2231#discussion_r322655091
    # therefore we do the following fallback behaviour:
    ...
fi

is more simple and straightforward.

The return code of an assignent of the form
var=$( COMMAND ) is the return code of COMMAND, see
https://stackoverflow.com/questions/20157938/exit-code-of-variable-assignment-to-command-substitution-in-bash
which also shows how to do things correctly for local variables.


# On RHEL 6 "name_assign_type" does not exist, therefore, the value of _name_assign_type=""
# Read https://github.com/rear/rear/issues/2197 for the details
[ "$_name_assign_type" = "" ] && return 1
gdha marked this conversation as resolved.
Show resolved Hide resolved

# NET_NAME_ENUM 1
[ "$_name_assign_type" = "1" ] && return 1
Expand Down