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

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

Closed
wants to merge 1 commit into from
Closed

Conversation

gdha
Copy link
Member

@gdha gdha commented Sep 10, 2019

Pull Request Details:
  • Type: Enhancement

  • Impact: Low

  • Reference to related issue (URL): cat: /sys/class/net/eth0/name_assign_type: Invalid argument #2197

  • How was this pull request tested? Manual

  • Brief description of the changes in this pull request: cosmetic fix to prevent the error message "/sys/class/net/eth0/name_assign_type: Invalid argument" - however, the return code is 1 (with the fix, but also with the code untouched). Therefore, it is a cosmetic fix only.

@gdha gdha added the cleanup label Sep 10, 2019
@gdha gdha added this to the ReaR v2.6 milestone Sep 10, 2019
@gdha gdha requested a review from jsmeix September 10, 2019 08:03
@gdha gdha self-assigned this Sep 10, 2019
@gdha
Copy link
Member Author

gdha commented Sep 10, 2019

Not merging the PR as it doesn't fix anything

@gdha gdha closed this Sep 10, 2019
@pcahyna
Copy link
Member

pcahyna commented Sep 10, 2019

@gdha Well, the first part still gets rid of the error message in the log, no?

@@ -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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants