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