Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(8 inline comments)
....................................................
File vdsm/libvirtvm.py
Line 1973: #TODO: delete the payload devices
what does think mean, exactly? remove the iso image from /var/run/vdsm? why postpone this
cleanup?
....................................................
File vdsm/mkimage.py
Line 32: _PAYLOAD_PATH_PREFIX = '/var/run/vdsm/payload/'
you forgot to base this on constants.P_VDSM_RUN
Line 47: dirname = tempfile.mkdtemp(prefix=_PAYLOAD_PATH_PREFIX)
I do not understand the "security" issue - this directory should be created with
limited accessibility anyway. Could you elaborate?
in any case, I think dir=_PAYLOAD_PATH_PREFIX arg should be used to specify the directory
in which the tempdir is created.
Line 74: m = storage.mount.Mount()
you are not using this object correctly. you should pass the (image, mntpoint) as its
args. Current code would explode.
Line 76: m.mount(mntOpts='-o loop %s %s' % (floppy, tmp))
mntOpts='loop' only
Line 78: for file in os.listdir(dirname):
I mean that you should call _decodeFilesIntoDir(files, dirname)
only now.
and have _decodeFilesIntoDir write the files to dirname (if specified)
Line 92: isopath = tempfile.NamedTemporaryFile(prefix=_PAYLOAD_PATH_PREFIX,
delete=False)
again, I believe "dir" is the right arg to use.
....................................................
File vdsm/vm.py
Line 448: devices[DISK_DEVICES].append(drv)
who defined this API with Engine?
It would make more sense to let Engine choose the exact address of this device. Is it
still possible to change this?
--
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: 4
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>