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

fcoe: replace fipvlan with fcoemon (#1085325) #200

Conversation

rvykydal
Copy link
Contributor

Same as dracut (#1129888), we are moving to use fcoemon which provides more
robust approach than fipvlan.

Resolves: rhbz#1085325

Same as dracut (#1129888), we are moving to use fcoemon which provides more
robust approach than fipvlan.

Resolves: rhbz#1085325
while timeout > 0:
rc, out = util.run_program_and_capture_output(
["lldptool", "-p"])
if not rc:
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer using if rc != 0 in these checks as I always has to think about it otherwise. Unless you're strongly against that, please replace these conditions with if rc != 0.

@vpodzime
Copy link
Contributor

Looks good to me otherwise.

["lldptool", "-p"])
if not rc:
break
log.info("Waiting for lldpad to be ready")
Copy link
Contributor

Choose a reason for hiding this comment

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

these are going to clutter up the logs, maybe only log if the timeout is exceeded. while / else could be used for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am going to what you suggest, please ignore the patch added to this branch, your way is nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, actually the patch added is the way I'd go, because eg if the timeout appears in 2nd wile loop, the else branches of the loop and all subsequent while loops blocks will get executed.

@rvykydal
Copy link
Contributor Author

I will, thanks.

@rvykydal
Copy link
Contributor Author

Pushed so we can get as early testing as possible.

I addressed these review remarks locally:

  • "if not rc" by vpodzime
  • comment for time.sleep(3) by vpodzime
  • cluttering log messages by bcl
  • log message % by clumens
  • missing timeout decrement by rvykydal
    and tested the patch in beaker.

@rvykydal rvykydal closed this Aug 13, 2015
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

5 participants