Change in vdsm[master]: vm payload: add file injection to vm

danken at redhat.com danken at redhat.com
Sun Mar 11 14:58:27 UTC 2012


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 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