Change in vdsm[master]: vm payload: add file injection to vm
danken at redhat.com
danken at redhat.com
Thu Mar 8 20:25:45 UTC 2012
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 <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
More information about the vdsm-patches
mailing list