Code review comment for lp:~johannes.erdfelt/nova/bug723996

Revision history for this message
Rick Harris (rconradharris) wrote :

Looks good, thanks Johannes.

Some thoughts:

* I noticed that FLAGS seem to be split between vm_utils.py and xenapi_conn.py. Is there a semantic distinction or was that more-or-less arbitrary. If possible, I'd like to see all of the flags go in one place; in this case, xenapi_conn since that's where most already exist.

Should we move the newly-created flag there and create a bug to move the other two flags?

* Should we log the fact that an excessively large kernel was attempted? Basically, extracting the msg part of the exception and then logging it before raising:

  msg = _("helpful message here")
  LOG.error(msg)
  raise exception.Error(msg)

This is a pretty common pattern; wonder if a util.raise_with_logging(exc_class, msg) helper would be useful?

Just a couple of suggestions, will leave up to you whether to update the patch. Thanks again.

review: Approve

« Back to merge proposal