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 <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Igor Lvovsky <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://fedorahosted.org/mailman/listinfo/vdsm-patches
