-
Notifications
You must be signed in to change notification settings - Fork 371
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
Fixes for SCVMM #219
Fixes for SCVMM #219
Conversation
hglkrijger
commented
Jun 7, 2016
- correct scvmm references
- add unit tests for scvmm detection
- ensure all matching devices are mounted and examined for scvmm configuration
- exit once vmm startup script is found and executed
- fixes 2.1 scvmm detection and support broken #185
def mount_dvd(self, max_retry=6, chk_err=True): | ||
dvd = self.get_dvd_device() | ||
mount_point = conf.get_dvd_mount_point() | ||
def mount_dvd(self, max_retry=6, chk_err=True, dvd=None, mount_point=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a variable name better than dvd
would be better? like dvd_device
?
bf1d113
to
3dd7fd2
Compare
addressed feedback - @ahmetalpbalkan, is this what you had in mind for |
self.start_scvmm_agent() | ||
else: | ||
self.osutil.umount_dvd(chk_err=False) | ||
for devices in filter(self.isNotEmpty, [re.match(r'(sr[0-9]|hd[c-z]|cdrom[0-9]?|cd[0-9]+)', dev) for dev in os.listdir(dev_dir)]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm kind of.. probably can be done without self.isNotEmpty
. let me try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you just need to do this:
filter(None.__ne__, ...)
and it will eliminate None
values from the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also you can do the group(0)
in the for
line:
for device in [d.group(0) for d in filter(...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a Python 3 feature. The additional method seems reasonably clean to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hglkrijger we only support python 3, though?
I find extra method not so clean, you can also do lambda x: x is not None
in the filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
3dd7fd2
to
a5e5012
Compare
addressed feedback |
/cc @szarkos |
try: | ||
self.load_atapiix_mod() | ||
except Exception as e: | ||
logger.warn("Could not load ATAPI driver: {0}".format(e.message)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you'll get AttributeError: 'Exception' object has no attribute 'message'
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works in Python 2, but you are right, this was deprecated in Python 3. Fixed - thanks for catching
- correct scvmm references - add unit tests for scvmm detection - ensure all matching devices are mounted and examined for scvmm configuration - exit once vmm startup script is found and executed - fixes Azure#185
a5e5012
to
8e96299
Compare
@ahmetalpbalkan - any more feedback, or can we merge this one? |
LGTM |