Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(10 inline comments)
....................................................
File vdsm/supervdsmServer.py
Line 29: import tempfile
you are adding 80 lines of a very specific code into a very general module.
I think it would be nicer to prepare a special module for the mkimage code (or abuse
util.py...)
Line 187: def _commonMkFs(self, files):
please document the structure of "files" (a dict of base64-encoded files
content)
and name the function according to what it does (decode files) and not where it is used.
Line 188: uid = resolveUid(VDSM_USER)
I think you'd need DISKIMAGE_USER, as the qemu process expected to read this.
Line 197: f = file(filename, 'w')
with file(filename, 'w') as f:
is cooler, no need for explicit close().
Line 202: tmpFile = tempfile.NamedTemporaryFile(prefix=PAYLOAD_PATH_PREFIX)
use delete=False instead of the close() hack.
(I think creation of the image file should not be done here, but in the calling
functions)
Line 207: uid = resolveUid(VDSM_USER)
I think you'd need DISKIMAGE_USER, as the qemu process expected to read this.
Line 215: os.rmdir(dirname)
rmdir must go out of the "for" loop.
but why not use shutil.rmtree() instead of inventing it?
Line 224: raise OSError(errno.EINVAL, "could not create floppy file: \
I'm not sure that all these cases are related to EINVAL (invalid argument).
Line 229: rc, out, err = storage.misc.execCmd(command, raw=True, sudo=False)
now that we have a sane default we can stop carrying this sudo=False around.
Line 234: for file in os.listdir(dirname):
it would be better to directly decode the files into this dir instead of this copying
--
To view, visit
http://gerrit.ovirt.org/2321
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I256475342c79690a95ad999335522f99714cdc8b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>