Dan Kenigsberg has posted comments on this change. Change subject: vm payload: add file injection to vm ......................................................................
Patch Set 3: I would prefer that you didn't submit this (8 inline comments) .................................................... File vdsm/mkimage.py Line 2: # Copyright 2008-2012 Red Hat, Inc. it's a new file! Line 31: PAYLOAD_PATH_PREFIX = '/var/run/vdsm/payload/' this should begine with _ in my opinion, and be based on constants.P_VDSM_RUN or whatever. Line 33: def _commonMkFs(files): maybe _decodeFilesIntoDir() ? Line 43: uid = resolveUid(DISKIMAGE_USER) somehow I got my former comment written twice. I think that this uid and gid can (and should?) be 0. qemu and vdsm users have no meaning for the guest that needs to read this image. Line 46: os.mkdir(PAYLOAD_PATH_PREFIX) is this mkdir and its raceful test really needed? why not use whatever tempfile.mkdtemp gives us? Line 70: rc, out, err = storage.misc.execCmd(command, raw=True, sudo=False) please drop this annoying sudo=False Line 79: raise OSError(errno.EIO, "could not mount floppy file: \ I'm not sure that EIO is any better. the failure may be due to a host of other reasons. How about raising a RuntimeError of your own? hmmm. come to think of it - you should use the new storage.mount.Mount object. Line 82: for file in os.listdir(dirname): I'm saying that you could have _decodeIntoDir() accept the tmp path, and have the file written into there. no need to write them twice. -- 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: 3 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
