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

Don't crash if the UIC file can't be written (#1375702) #872

Merged

Conversation

M4rtinK
Copy link
Contributor

@M4rtinK M4rtinK commented Nov 17, 2016

Don't crash if the user interaction config file can't be written,
which can happen for example on Atomic (read-only rootfs by default).

Print and error message and log the exception instead.

Don't crash if the user interaction config file can't be written,
which can happen for example on Atomic (read-only rootfs by default).

Print and error message and log the exception instead.
@M4rtinK M4rtinK added f25 master Please, use the `f39` label instead. labels Nov 17, 2016
Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise.

f.write(self._get_new_config_header())
self._config.write(f)
except OSError:
log.exception("Can't write user interaction config file.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a warning instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The open() can generate quite a lot of various exceptions - so the general idea is to for now just to prevent the crash & log the exception. Later on (and possibly once we have more data) we can narrow down the scope of the except clause & log specific warnings/errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, but shouldn't this also log details about the exception then? Or does log.exception() do that somehow?

Copy link
Contributor Author

@M4rtinK M4rtinK Nov 23, 2016

Choose a reason for hiding this comment

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

Yep - log.exception() logs the message you give it and then the full traceback from the exception caught by the except block. From the docs:

Logger.exception(msg, *args, **kwargs)

Logs a message with level ERROR on this logger. 
The arguments are interpreted as for debug(). 
Exception info is added to the logging message. 
This method should only be called from an exception handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@M4rtinK M4rtinK merged commit 23cd84f into rhinstaller:master Nov 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f25 master Please, use the `f39` label instead.
2 participants