9:11 a.m.
Shahar Havivi has uploaded a new change for review.
Change subject: vm payload: add file injection to vm
......................................................................
vm payload: add file injection to vm
this patch adds new parameter to create verb vmPayload,
user can send file content and have them in the vm in floppy of iso
format.
Change-Id: I256475342c79690a95ad999335522f99714cdc8b
---
M configure.ac
M vdsm/constants.py.in
M vdsm/libvirtvm.py
M vdsm/supervdsmServer.py
M vdsm/vm.py
5 files changed, 99 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/21/2321/1
--
To view, visit http://gerrit.ovirt.org/2321
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I256475342c79690a95ad999335522f99714cdc8b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
2:54 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 2: (10 inline comments)
....................................................
File vdsm/supervdsmServer.py
Line 29: import tempfile
sure,
but please don't ask me later to join it again to this module
Line 187: def _commonMkFs(self, files):
what name do you mean?
Line 188: uid = resolveUid(VDSM_USER)
Done
Line 197: f = file(filename, 'w')
Done
Line 202: tmpFile = tempfile.NamedTemporaryFile(prefix=PAYLOAD_PATH_PREFIX)
Done
Line 207: uid = resolveUid(VDSM_USER)
Done
Line 215: os.rmdir(dirname)
Done
Line 224: raise OSError(errno.EINVAL, "could not create floppy file: \
is EIO better?
Line 229: rc, out, err = storage.misc.execCmd(command, raw=True, sudo=False)
Done
Line 234: for file in os.listdir(dirname):
what do you mean?
tmp is the mounted dir
--
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: 2
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>
5:25 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(15 inline comments)
Several general questions:
1. Could be same 'payload' used by several VM's?
If answer is 'yes', maybe we need to create file with
vmID 'prefix'/'trailer' in the name.
2. Is this survive migration and vdsm restart ?
3. Can you remember ne what we decided about reporting
back to rhev-M this huge payload data ?
....................................................
Commit Message
Line 10: user can send file content and have them in the vm in floppy of iso
floppy (.vmf) or cdrom (.iso)
....................................................
File vdsm/supervdsmServer.py
Line 40: import constants
Ah, I think we want to avoid import constants here. Ask Dan/Saggi.
Line 46: PAYLOAD_PREFIX_PATH = '/var/run/vdsm/payload/'
Why you called it xxx_PREFIX_xxx ?
Line 186: def _commonMkFs(self, files):
I am not a 'naming' police, but you just create a file here. Why xxxMkFs ?
Line 194: for name in entry.iterkeys():
hmmml, maybe I am missing something. Can you please explain me how is 'files'
looks like ?
Line 199: os.chown(filename, uid, gid)
grrr, trailing space
Line 205: def _commonCleanFs(self, files, dirname, media):
Again, I think a name a little misleading
Line 208: os.chown(media, uid, gid)
Why you need chown here? you already did it during creation
Line 224: code %s, out %s\nerr %s" % (str(rc), out, err))
Dan worked hard to clean all str() from the code.
Line 231: code %s, out %s\nerr %s" % (str(rc), out, err))
str() is redundant
Line 240: code %s, out %s\nerr %s" % (str(rc), out, err))
str() is redundant
Line 255: code %s, out %s\nerr %s" % (str(rc), out, err))
str() is redundant
....................................................
File vdsm/vm.py
Line 431: if self.conf.has_key('vmPayload'):
I don't like this global key. You can create particular device with specParams
"vmPaylod".
Line 437: drv['iface'] = 'ide'
This maybe a problem.
1. You can't be sure that you have empty ide slot (in case you already have 4 ide
devices)
2. Floppy disk not ide at all.
Line 438: for key, files in self.conf.get('vmPayload'):
What is files here? Can you plese explain what you want to pass here?
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
6:43 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 1: (14 inline comments)
....................................................
File vdsm/supervdsmServer.py
Line 40: import constants
Done
Line 46: PAYLOAD_PREFIX_PATH = '/var/run/vdsm/payload/'
Done
Line 186: def _commonMkFs(self, files):
why not
Line 194: for name in entry.iterkeys():
look at vm.py in the new patch
Line 199: os.chown(filename, uid, gid)
Done
Line 205: def _commonCleanFs(self, files, dirname, media):
suggestions?
Line 208: os.chown(media, uid, gid)
because mkisofs creating the file as root
Line 224: code %s, out %s\nerr %s" % (str(rc), out, err))
Done
Line 231: code %s, out %s\nerr %s" % (str(rc), out, err))
Done
Line 240: code %s, out %s\nerr %s" % (str(rc), out, err))
Done
Line 255: code %s, out %s\nerr %s" % (str(rc), out, err))
Done
....................................................
File vdsm/vm.py
Line 431: if self.conf.has_key('vmPayload'):
well we decided (me and Eduardo) that it will be better to make the device here.
Line 437: drv['iface'] = 'ide'
virtio will be sufficient here?
Line 438: for key, files in self.conf.get('vmPayload'):
Done
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
6:57 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 8: (7 inline comments)
....................................................
File vdsm/clientIF.py
Line 202: drive['specParams'].has_key('vmPayload'):
has_key is deprecated in favor of 'in':
'specParams' in drive and 'vmPayload' in drive['specParams']
....................................................
File vdsm/libvirtvm.py
Line 1978: drive['specParams'].has_key('vmPayload'):
Same here about has_key vs in.
....................................................
File vdsm/mkimage.py
Line 41: '''
Inconsistent indenting
Line 53: def _commonCleanFs(files, dirname, media):
Files seems to be an unused parameter.
Line 58: os.chown(media, uid, gid)
I would prefer you moved uid and gid assignments inside this if block. No need to resolve
them if you're not going to use them.
Line 82: os.rmdir(dirname)
Won't _commonCleanFs take care of this?
....................................................
File vdsm/supervdsmServer.py
Line 196: def removeFs(self, path):
Shouldn't this have a @logDecorator as well?
--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
2:46 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 4: (8 inline comments)
....................................................
File vdsm/libvirtvm.py
Line 1973: #TODO: delete the payload devices
its not postpone, as you see I never check the code (until I will implement the backend)
the releaseVm is in libvirtvm.py but the code need to be in vm.py,
any suggestions?
....................................................
File vdsm/mkimage.py
Line 32: _PAYLOAD_PATH_PREFIX = '/var/run/vdsm/payload/'
Done
Line 47: dirname = tempfile.mkdtemp(prefix=_PAYLOAD_PATH_PREFIX)
Dan,
I don't understand what do you mean by that.
Line 74: m = storage.mount.Mount()
Done
Line 76: m.mount(mntOpts='-o loop %s %s' % (floppy, tmp))
Done
Line 78: for file in os.listdir(dirname):
no I can't
the _decodeFilesInDir is common code for the floppy and iso,
the this copy is only in the floppy case
Line 92: isopath = tempfile.NamedTemporaryFile(prefix=_PAYLOAD_PATH_PREFIX,
delete=False)
sorry, what do you mean?
....................................................
File vdsm/vm.py
Line 448: devices[DISK_DEVICES].append(drv)
I did.
and I will change it to the new device model.
--
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>
2:25 p.m.
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(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>
6:53 a.m.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: vm payload: add file injection to vm
......................................................................
vm payload: add file injection to vm
this patch adds new parameter to create verb vmPayload,
user can send file content and have them in the vm in floppy (.vmf) or
cdrom (.iso) format.
vmPayload is in specParams
'vmPayload': {'file': {'filename': 'content' }}
for now we are only supporting file type in cdrom or floppy,
we intend to add more formats such as network url which vdsm will
download the file and place it in cdrom or floppy.
Change-Id: I256475342c79690a95ad999335522f99714cdc8b
Signed-off-by: Shahar Havivi <shaharh(a)redhat.com>
---
M configure.ac
M vdsm.spec.in
M vdsm/Makefile.am
M vdsm/clientIF.py
M vdsm/constants.py.in
M vdsm/libvirtvm.py
A vdsm/mkimage.py
M vdsm/supervdsmServer.py
8 files changed, 147 insertions(+), 0 deletions(-)
Approvals:
Shahar Havivi: Verified
Dan Kenigsberg: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/2321
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I256475342c79690a95ad999335522f99714cdc8b
Gerrit-PatchSet: 35
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
6:53 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 35: Looks good to me, approved
--
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: 35
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
6:53 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 35: Verified
--
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: 35
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
6:36 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 34: I would prefer that you didn't submit this
(1 inline comment)
....................................................
File vdsm/vm.py
Line 933: conf = deepcopy(self.conf)
I now see another problem: this changes what self.save() writes to disk and cause a
problem on vdsm restart.
please. avoid filtering at the moment, and have this discussed on another patch.
--
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: 34
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
6:26 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 34: Verified
--
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: 34
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
6:23 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 33: (2 inline comments)
....................................................
File vdsm/vm.py
Line 933: conf = self.conf.copy()
Done
Line 937: device['specParams'].pop('vmPayload')
Done
--
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: 33
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
6:20 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 33: I would prefer that you didn't submit this
(2 inline comments)
let's push this without filtering, first.
....................................................
File vdsm/vm.py
Line 933: conf = self.conf.copy()
this is not a deep copy, so the device dict would be changed by this.
Line 937: device['specParams'].pop('vmPayload')
would have been nicer:
del device['specParams']['vmPayload']
--
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: 33
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
5:48 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 32: (1 inline comment)
....................................................
File vdsm/clientIF.py
Line 213: drive['specParams']['vmPayload'][key] =
'[CONTENT TRUNCATE]'
Done
--
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: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
1:45 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 32: (1 inline comment)
....................................................
File vdsm/clientIF.py
Line 213: drive['specParams']['vmPayload'][key] =
'[CONTENT TRUNCATE]'
if it was up to me, you would not have removed it at all. at least not in the first patch.
but you can tweak whatever is returned by vdsm's list verb in Vm.vm.status(). Make
sure to change stuff only in a copy of self.conf.
--
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: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
1:33 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 32: (1 inline comment)
....................................................
File vdsm/clientIF.py
Line 213: drive['specParams']['vmPayload'][key] =
'[CONTENT TRUNCATE]'
where should I remove it then (in the report, which module?)
--
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: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
9:14 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 32: I would prefer that you didn't submit this
(1 inline comment)
....................................................
File vdsm/clientIF.py
Line 213: drive['specParams']['vmPayload'][key] =
'[CONTENT TRUNCATE]'
that's not good - the content should be truncated (better simply dropped) only on
report ("list" verb). With this, migration would fail - destination host would
try to create a payload file with '[CONTENT TRUNCATE]' content.
--
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: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
4:22 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 32: Looks good to me, but someone else must approve
--
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: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:35 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 32: Verified
--
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: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:35 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 27: (6 inline comments)
....................................................
File vdsm/clientIF.py
Line 200: #TODO: REMOVE payload from getVmStats
Done
....................................................
File vdsm/mkimage.py
Line 36: def _decodeFilesIntoDir(files, dirname=None):
Done
Line 43: :param files: [{'filename': 'content' }, {'filename':
'content'}]
Done
Line 48: dirname = tempfile.mkdtemp()
we had this discussion and it was ping pong from /tmp to /var
Line 81: m.mount(mntOpts='loop')
Done
....................................................
File vdsm.spec.in
Line 593: %ghost %dir %{_localstatedir}/run/%{vdsm_name}/pools
Done
--
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: 27
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
9:25 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 31: (1 inline comment)
....................................................
File vdsm/mkimage.py
Line 77: m.mount(mntOpts='loop')
I think he said that if I remove the 'loop' option there is no need to check the
isMounted() just use the unmount(), right?
--
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: 31
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
8:56 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 31: I would prefer that you didn't submit this
(2 inline comments)
....................................................
File vdsm/mkimage.py
Line 77: m.mount(mntOpts='loop')
Saggi noted that if m.mount() is moved above the "try"
you can simplify the "finally" and have there an unconditional umount().
Line 90: dirname = tempfile.mkdtemp()
good. I'm fine with using /tmp, certainly for vm payload.
--
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: 31
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
11:51 p.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
--
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: 30
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
11:46 p.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 28: (1 inline comment)
....................................................
Commit Message
Line 16: we intend to add more formats suck as network url which vdsm will
Done
--
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: 28
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:27 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 30: I would prefer that you didn't submit 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: 30
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:27 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 30:
Shahar, what about the "suck" in the commit message, and Saggi's comments?
You do not have to accept them all, but please relate to them and fix the obvious ones.
--
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: 30
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:12 p.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 29: (1 inline comment)
....................................................
File vdsm.spec.in
Line 594: %ghost %dir %{_localstatedir}/run/%{vdsm_name}
Done
--
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: 29
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
2:36 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 29: I would prefer that you didn't submit 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: 29
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
2:36 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 29: (1 inline comment)
Shahar, please note Motti and Saggi's valuable comments.
....................................................
File vdsm.spec.in
Line 594: %ghost %dir %{_localstatedir}/run/%{vdsm_name}
twice still
--
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: 29
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
2:09 a.m.
Moti Asayag has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 28: (1 inline comment)
....................................................
Commit Message
Line 16: we intend to add more formats suck as network url which vdsm will
s/suck/such
--
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: 28
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
2:41 p.m.
Saggi Mizrahi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 27: I would prefer that you didn't submit this
(7 inline comments)
Also, this whole system isn't scalable at all, but I imagine it wasn't meant to
be.
....................................................
File vdsm/clientIF.py
Line 200: #TODO: REMOVE payload from getVmStats
Did you do it?
....................................................
File vdsm/mkimage.py
Line 36: def _decodeFilesIntoDir(files, dirname=None):
Always have the caller create the dirname
Line 43: :param files: [{'filename': 'content' }, {'filename':
'content'}]
it's actually {'filename: 'content', ...}
Line 48: dirname = tempfile.mkdtemp()
Don't put things in temp, it's a free for all security wise and also more and more
distros are making tmp tmpfs so it has very limited space.
Line 51: with file(filename, 'w') as f:
f.write(base64.b64decode(content))
Lines don't cost money
Line 81: m.mount(mntOpts='loop')
If you move this out you don't need to check is mounted
....................................................
File vdsm.spec.in
Line 593: %ghost %dir %{_localstatedir}/run/%{vdsm_name}/pools
pools is here 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: 27
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
5:15 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 24: Looks good to me, but someone else must approve
--
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: 24
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
12:18 p.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 23: (2 inline comments)
....................................................
File vdsm/mkimage.py
Line 29: from vdsm.constants import P_PAYLOAD_IMAGES
Done
Line 107: raise Exception('Cannot remove Fs that is not exists in: ' +
P_PAYLOAD_IMAGES)
Done
--
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: 23
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
9:09 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 23: Looks good to me, but someone else must approve
(2 inline comments)
if this patch is verified, we can take it, and worry about cleanup in a following patch -
I know that you are tired of all these resends.
....................................................
File vdsm/mkimage.py
Line 29: from vdsm.constants import P_PAYLOAD_IMAGES
I'd prefer that P_PAYLOAD_IMAGES is defined here, in this module, and is kept local.
there is no point exposing this location in constants
_P_PAYLOAD_IMAGES = constants.P_VDSM_RUN + 'payload'
Line 107: raise Exception('Cannot remove Fs that is not exists in: ' +
P_PAYLOAD_IMAGES)
s/is not/does not/
--
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: 23
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
9:08 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 22: (1 inline comment)
....................................................
File vdsm/mkimage.py
Line 108: os.remove(path)
no one should be able to mess with stuff below P_PAYLOAD_IMAGES, we can trust this
dir's content.
--
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: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
1:51 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 22: (3 inline comments)
....................................................
File vdsm/mkimage.py
Line 32: PAYLOAD_IMAGES_P = '/var/run/vdsm/payload/'
done.
Line 63: os.makedirs(PAYLOAD_IMAGES_P)
Done
Line 64: path = os.path.join(PAYLOAD_IMAGES_P, vmId + '.img')
Done
--
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: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:41 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 22: (1 inline comment)
....................................................
File vdsm/mkimage.py
Line 108: os.remove(path)
I get the feeling this is raceful as well. Suppose path is a symlink pointing to a
'safe' location. On the other hand, then only the symlink is removed. So I'm
not sure if this is actually a problem.
--
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: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:13 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 22: I would prefer that you didn't submit 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: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:12 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 22: (3 inline comments)
using constants.P_VDSM_RUN is a hard requirement. the rest can wait for a later patch if
you add a #TODO comment.
....................................................
File vdsm/mkimage.py
Line 32: PAYLOAD_IMAGES_P = '/var/run/vdsm/payload/'
this must be based on
constants.P_VDSM_RUN
or else we cannot move to /run when we decide to.
Line 63: os.makedirs(PAYLOAD_IMAGES_P)
check-and-later-create ad directory is raceful. Better have this created in the rpm.spec
file
Line 64: path = os.path.join(PAYLOAD_IMAGES_P, vmId + '.img')
please consider adding a hash of the "files" to the path, to allow multiple
payloads per Vm.
--
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: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
9:58 a.m.
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>
7:32 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
--
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: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
6:36 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 19: (2 inline comments)
....................................................
File vdsm/mkimage.py
Line 32: PAYLOAD_IMAGES_P = '/tmp'
I personally did not understand the usefulness of the /var/run->/run change, but that
is besides the point for this patch.
Here, we should put everything under P_VDSM_RUN, and discuss in a different patch if we
should change our default value for that constant.
Line 106: if not path.startswith(PAYLOAD_IMAGES_P):
yep, we should use os.path.abspath() to avoid this problem.
--
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: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
5:50 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 19: (1 inline comment)
....................................................
File vdsm/mkimage.py
Line 32: PAYLOAD_IMAGES_P = '/tmp'
right, I forgot that...
why to use all that path and not /var/run/vdsm/payload/ ?
--
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: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
6:06 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 19: (2 inline comments)
....................................................
File vdsm/mkimage.py
Line 32: PAYLOAD_IMAGES_P = '/tmp'
I think /var/run is a bad place for this. FHS 3.0 draft 1 about /var/run
(http://www.linuxbase.org/betaspecs/fhs/fhs/ch05s13.html):
This directory was once intended for system information data describing the system since
it was booted. These functions have been moved to /run; this directory exists to ensure
compatibility with systems and software using an older version of this specification.
Then we have /run (http://www.linuxbase.org/betaspecs/fhs/fhs/ch03s15.html):
This directory contains system information data describing the system since it was booted.
Files under this directory must be cleared (removed or truncated as appropriate) at the
beginning of the boot process.
So it's going away in favor of /run and file payloads don't describe the system
state.
Line 106: if not path.startswith(PAYLOAD_IMAGES_P):
This is a string and string comparisons on paths is still not very secure. Consider
/tmp/../etc/passwd for example.
--
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: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
5:05 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 19: I would prefer that you didn't submit this
(1 inline comment)
....................................................
File vdsm/mkimage.py
Line 32: PAYLOAD_IMAGES_P = '/tmp'
it is impolite to trash /tmp. it also leaks info about payload existence to casual users.
worse - the file can be already there at the destination machine.
why not use /var/run/vdsm/payload/vmid/md5 (this would alow in the future multiple
payloads per vm)
--
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: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:43 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 18: (2 inline comments)
....................................................
File vdsm/mkimage.py
Line 104: def removeFs(path):
Done
....................................................
File vdsm/supervdsmServer.py
Line 216: mkimage.mkFloppyFs(vmId, files)
Done, sorry I miss that...
--
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: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
1:56 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 18: I would prefer that you didn't submit this
(2 inline comments)
It seems that you still let vdsm remove any root-owned file in the system. That's not
nice.
....................................................
File vdsm/mkimage.py
Line 104: def removeFs(path):
repeat: this is not safe enough. with this function, any buggy vdsm function can end up
removing any system file.
Please check here that 'path' starts with the basedir of media image.
/var/run/vdsm/images/vmid - not a random one.
....................................................
File vdsm/supervdsmServer.py
Line 216: mkimage.mkFloppyFs(vmId, files)
return forgotten
--
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: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
6:24 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 17: (3 inline comments)
....................................................
File vdsm/clientIF.py
Line 210: volPath = supervdsm.getProxy().mkIsoFs(files)
md5 is not good enough, other vms with the same files will get the same md5sum.
I will use some guid like the vmid
....................................................
File vdsm/supervdsmServer.py
Line 216: mkimage.mkFloppyFs(files)
Done
Line 223: def removeFs(self, path):
we said that it will be in /tmp/
so I will check that the path is starts with that
--
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: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 17: I would prefer that you didn't submit this
media location should include vmId (you do not want one Vm going down causing removal of
second Vm media)
sysprep media is currently put at
/var/run/vdsm/<vmId>.vfd
for vmpayload, i'd suggest to include either the <sha1hash> or simpler - the
alias of the payload device, such as fda or sdb, to allow multiple payloads per vm in the
future.
--
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: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:51 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 17: (3 inline comments)
I believe that the payload image must be put somewhere like
/var/tmp/vdsm/images/sha1.iso
where sha1 is calculated on the payload content.
Sorry for not having thought of this before, but this is quite serious - for security and
for migration.
....................................................
File vdsm/clientIF.py
Line 210: volPath = supervdsm.getProxy().mkIsoFs(files)
come to think of this - the location of the iso image must not be random, or else the
guest won't find it after migration.
Idea: put the image in /var/tmp/vdsm/images/md5(files)
....................................................
File vdsm/supervdsmServer.py
Line 216: mkimage.mkFloppyFs(files)
shouldn't you
return mkimage.mkFloppyFs(files)
instead?
Line 223: def removeFs(self, path):
that's not very safe, as it allows Vdsm to remove any system file whereever it wants
to.
We MUST add protection on the paths allowed to be removed.
How about putting all images somewhere under /var/tmp/vdsm/images/
and letting vdsm remove files only there?
--
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: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
1:53 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 16: (3 inline comments)
....................................................
Commit Message
Line 17: donwload the file and palce it in cdrom or floppy.
Done
....................................................
File vdsm/clientIF.py
Line 205: 'vmPayload': ('file': {'filename':
'content'}}
Done
Line 212: drive['path'] =
supervdsm.getProxy().mkFloppyFs(files)
Done
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:12 p.m.
Igor Lvovsky has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 16: (1 inline comment)
Ah, I should read the commit message befofre I comment patch.
Now I understand what is 'file' means
....................................................
Commit Message
Line 17: donwload the file and palce it in cdrom or floppy.
s/palse/place
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:09 p.m.
Igor Lvovsky has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 16: (2 inline comments)
I am sorry that it take so long time to review it
....................................................
File vdsm/clientIF.py
Line 205: 'vmPayload': ('file': {'filename':
'content'}}
Should it be '{' instead of '('.
what is 'file' here and why we need it? Can it be something else except the
'file'?
Line 212: drive['path'] =
supervdsm.getProxy().mkFloppyFs(files)
why this not consistent with rest function?
why you set drive['path'] here instead of volPath and return it?
--
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:57 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 15: (3 inline comments)
....................................................
File vdsm/clientIF.py
Line 205: 'vmPayload': (file: {'filename':
'content'}}
Done
Line 207: for key, files in
drive['specParams']['vmPayload']:
Done
....................................................
File vdsm/supervdsmServer.py
Line 213: def mkFloppyFs(self, files):
Done
--
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: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:09 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 15: (3 inline comments)
....................................................
File vdsm/clientIF.py
Line 205: 'vmPayload': (file: {'filename':
'content'}}
I think (file: should be {'file':
Line 207: for key, files in
drive['specParams']['vmPayload']:
Where did iteritems go?
....................................................
File vdsm/supervdsmServer.py
Line 213: def mkFloppyFs(self, files):
Missing a logDecorator here.
--
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: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
1:41 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 12: (3 inline comments)
....................................................
File vdsm/clientIF.py
Line 207: for key, files in
drive['specParams']['vmPayload']:
Done
....................................................
File vdsm/libvirtvm.py
Line 2029: for drive in self._devicesr[vm.DISK_DEVICES]:
Done
Line 2032: supervdsm.getProxy().mkIsoFs(drive['path'])
yes, thanks (never tested, waiting for engine patch)
--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
9:24 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 12: I would prefer that you didn't submit this
(3 inline comments)
....................................................
File vdsm/clientIF.py
Line 207: for key, files in
drive['specParams']['vmPayload']:
I think you need to use iteritems() on a dict to iterate it.
....................................................
File vdsm/libvirtvm.py
Line 2029: for drive in self._devicesr[vm.DISK_DEVICES]:
s/_devicesr/_devices/
Line 2032: supervdsm.getProxy().mkIsoFs(drive['path'])
I'm not sure what you're trying to do here. Did you mean removeFs?
--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
7:49 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 11:
still in the progress of integration testing with engine,
don't push until +1 verified from me
--
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: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
7:02 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 11: Looks good to me, but someone else must approve
--
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: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
5:58 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 10: (1 inline comment)
Except for 1 trivial issue it looks good.
....................................................
File vdsm/libvirtvm.py
Line 1979: supervdsm.getProxy().mkIsoFs(drive['path'])
Indenting here is off.
--
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: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
9:19 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 9: (1 inline comment)
Sounds like a bigger has_key-replacement-patch is needed.
....................................................
File vdsm/mkimage.py
Line 74: dirname = _decodeFilesIntoDir(files)
I think you meant this:
_decodeFilesIntoDir(files, dirname)
--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
6:10 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: (1 inline comment)
....................................................
File vdsm/constants.py.in
Line 54: P_PAYLOAD_PREFIX = '/var/run/vdsm/payload/'
Done
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:56 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 3: (8 inline comments)
....................................................
File vdsm/mkimage.py
Line 2: # Copyright 2008-2012 Red Hat, Inc.
so should I remove the copyright or just add the 2012?
Line 31: PAYLOAD_PATH_PREFIX = '/var/run/vdsm/payload/'
Done
Line 33: def _commonMkFs(files):
Done
Line 43: uid = resolveUid(DISKIMAGE_USER)
Done
Line 46: os.mkdir(PAYLOAD_PATH_PREFIX)
because of security issues, it should not be in temp
Line 70: rc, out, err = storage.misc.execCmd(command, raw=True, sudo=False)
Done
Line 79: raise OSError(errno.EIO, "could not mount floppy file: \
Done
Line 82: for file in os.listdir(dirname):
Sorry I don't get what you are saying here.
--
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(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>
6:44 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(10 inline comments)
....................................................
File vdsm/supervdsmServer.py
Line 29: import tempfile
you are adding 80 lines of a very specific code into a very general module.
I think it would be nicer to prepare a special module for the mkimage code (or abuse
util.py...)
Line 187: def _commonMkFs(self, files):
please document the structure of "files" (a dict of base64-encoded files
content)
and name the function according to what it does (decode files) and not where it is used.
Line 188: uid = resolveUid(VDSM_USER)
I think you'd need DISKIMAGE_USER, as the qemu process expected to read this.
Line 197: f = file(filename, 'w')
with file(filename, 'w') as f:
is cooler, no need for explicit close().
Line 202: tmpFile = tempfile.NamedTemporaryFile(prefix=PAYLOAD_PATH_PREFIX)
use delete=False instead of the close() hack.
(I think creation of the image file should not be done here, but in the calling
functions)
Line 207: uid = resolveUid(VDSM_USER)
I think you'd need DISKIMAGE_USER, as the qemu process expected to read this.
Line 215: os.rmdir(dirname)
rmdir must go out of the "for" loop.
but why not use shutil.rmtree() instead of inventing it?
Line 224: raise OSError(errno.EINVAL, "could not create floppy file: \
I'm not sure that all these cases are related to EINVAL (invalid argument).
Line 229: rc, out, err = storage.misc.execCmd(command, raw=True, sudo=False)
now that we have a sane default we can stop carrying this sudo=False around.
Line 234: for file in os.listdir(dirname):
it would be better to directly decode the files into this dir instead of this copying
--
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: 2
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>
8:14 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 8: (6 inline comments)
....................................................
File vdsm/clientIF.py
Line 202: drive['specParams'].has_key('vmPayload'):
we are currently using that all over the code,
we will need to change it in all code at once.
....................................................
File vdsm/libvirtvm.py
Line 1978: drive['specParams'].has_key('vmPayload'):
same here 2...
....................................................
File vdsm/mkimage.py
Line 41: '''
Done
Line 53: def _commonCleanFs(files, dirname, media):
Done
Line 82: os.rmdir(dirname)
Done
....................................................
File vdsm/supervdsmServer.py
Line 196: def removeFs(self, path):
Done
--
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: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
5:48 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: (1 inline comment)
....................................................
File vdsm/constants.py.in
Line 54: P_PAYLOAD_PREFIX = '/var/run/vdsm/payload/'
In that case I still think /var/run is a bad place for these temporary files. If we look
forward to the FHS 3.0 drafts (http://www.linuxbase.org/betaspecs/fhs/fhs/ch05s13.html):
This directory was once intended for system information data describing the
system since it was booted. These functions have been moved to /run; this
directory exists to ensure compatibility with systems and software using an
older version of this specification.
Given Fedora is leading this change choosing /var/run now doesn't sound very
future-proof.
I think I still missed the problem with /tmp. mkdtemp will create the directory 700 so I
fail to see the security aspect.
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
5:33 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: (2 inline comments)
....................................................
File vdsm/constants.py.in
Line 54: P_PAYLOAD_PREFIX = '/var/run/vdsm/payload/'
actually I do not want that to survive reboot,
sorry if I miss lead you.
....................................................
File vdsm/mkimage.py
Line 95: isopath = tempfile.NamedTemporaryFile(prefix=P_PAYLOAD_PREFIX, delete=False)
Done
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:21 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(2 inline comments)
....................................................
File vdsm/constants.py.in
Line 54: P_PAYLOAD_PREFIX = '/var/run/vdsm/payload/'
You said you wanted something more persistent between reboots so I think /var/run is a bad
place. From the FHS
(http://www.pathname.com/fhs/pub/fhs-2.3.html#VARRUNRUNTIMEVARIABLEDATA):
This directory contains system information data describing the system since it was booted.
Files under this directory must be cleared (removed or truncated as appropriate) at the
beginning of the boot process.
It sounds like you want to use /var/tmp
(http://www.pathname.com/fhs/pub/fhs-2.3.html#VARTMPTEMPORARYFILESPRESERVE...):
The /var/tmp directory is made available for programs that require temporary files or
directories that are preserved between system reboots. Therefore, data stored in /var/tmp
is more persistent than data in /tmp.
....................................................
File vdsm/mkimage.py
Line 95: isopath = tempfile.NamedTemporaryFile(prefix=P_PAYLOAD_PREFIX, delete=False)
The documentation (http://docs.python.org/library/tempfile.html) clearly specifies a dir
parameter.
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:09 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: (4 inline comments)
....................................................
File vdsm/constants.py.in
Line 54: P_PAYLOAD_PREFIX = '/var/run/vdsm/payload/'
Done
....................................................
File vdsm/mkimage.py
Line 46: dirname = tempfile.mkdtemp(prefix=P_PAYLOAD_PREFIX)
Done
Line 81: for file in os.listdir(dirname):
Done
Line 103: _commonCleanFs(files, dirname, isopath)
Done
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
10:53 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: (4 inline comments)
....................................................
File vdsm/constants.py.in
Line 54: P_PAYLOAD_PREFIX = '/var/run/vdsm/payload/'
Please do not add this very specific constant to the global file. It should be defined
internally in mkimage module.
Also, it should *refer* to P_VDSM_RUN, not copy its default content.
....................................................
File vdsm/mkimage.py
Line 46: dirname = tempfile.mkdtemp(prefix=P_PAYLOAD_PREFIX)
test-and-sometime-later-create is a bad practice:
if not os.path.exists(P_PAYLOAD_PREFIX):
os.mkdir(P_PAYLOAD_PREFIX)
another thread may create the directory after you check for its existence. Now, this is
not very probable. It is simply bad practice.
Line 81: for file in os.listdir(dirname):
no need to mount in _decodeFilesInDir, of course.
I'm suggesting to add a target directory argument to decodeFilesInDir. the floppy use
case would pass the mountpoint, and the iso use case would pass a tempdir it just
created.
Line 103: _commonCleanFs(files, dirname, isopath)
just like the "finally" of line 88.
finally does not swallow exceptions, it lets them pass through.
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
10:38 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: (4 inline comments)
....................................................
File vdsm/mkimage.py
Line 46: dirname = tempfile.mkdtemp(prefix=P_PAYLOAD_PREFIX)
one of the guys rhev-h told me not to use /tmp/ for that for security reasons (in other
patch)
and /var/vdsm/.... is for vdsm user and its in var so its not survive boot, so what is the
problem???
what race do you mean?
Line 81: for file in os.listdir(dirname):
in any case I must copy to the mounted point ie after mounting - this is true only to
floppy device since in iso we use mkisofs which get a dir as a prameter.
In floppy we first create the filesystem and mount it and copy files to it.
I don't want to mount in _decodeFilesInDir() because it not common code (if you refer
to that - which I think you didn't)
Line 95: isopath = tempfile.NamedTemporaryFile(prefix=P_PAYLOAD_PREFIX, delete=False)
what do you mean?
prefix= is NamedTemporaryFile() parameter
Line 103: _commonCleanFs(files, dirname, isopath)
and what will I do in finally?
I think this will "swallow" the error...
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
9:42 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 9: (1 inline comment)
....................................................
File vdsm/mkimage.py
Line 74: dirname = _decodeFilesIntoDir(files)
yes, thanks,
this code didn't tested yet (it send just for general review), I am still working on
the engine side...
--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:03 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: (2 inline comments)
....................................................
File vdsm/mkimage.py
Line 46: dirname = tempfile.mkdtemp(prefix=P_PAYLOAD_PREFIX)
why are you not content using plain tempfile.mkdtemp()? What is your security concern?
In any case, even if you keep dir=P_PAYLOAD_PREFIX, please avoid the race above, and wrap
os.mkdir() with try:except:.
Line 81: for file in os.listdir(dirname):
I suggest that you add an *optional* arg to _decodeFilesInDir(files, dir=None)
and create a temp dir only if this is required.
this needless copy is just another point of failure.
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:04 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: I would prefer that you didn't submit 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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
10:58 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 7: (2 inline comments)
....................................................
File vdsm/mkimage.py
Line 95: isopath = tempfile.NamedTemporaryFile(prefix=P_PAYLOAD_PREFIX, delete=False)
I think Dan suggested to use dir=P_PAYLOAD_PREFIX instead of prefix= and that might be
better for most other prefix= uses in this file.
Line 103: _commonCleanFs(files, dirname, isopath)
I think you want a finally: here as well
--
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: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
11:56 a.m.
Shahar Havivi has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 5: (3 inline comments)
....................................................
File vdsm/constants.py.in
Line 54: P_PAYLOAD_PATH_PREFIX = '/var/run/vdsm/payload/'
Done
....................................................
File vdsm/mkimage.py
Line 51: f.write(base64.b64decode(entry[name]))
Done
Line 99: _commonCleanFs(files, dirname, isopath)
Done
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
11:16 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: vm payload: add file injection to vm
......................................................................
Patch Set 5: (4 inline comments)
....................................................
File vdsm/constants.py.in
Line 54: P_PAYLOAD_PATH_PREFIX = '/var/run/vdsm/payload/'
Doesn't P_ already imply PATH? Thus P_PAYLOAD_PREFIX should suffice.
....................................................
File vdsm/mkimage.py
Line 51: f.write(base64.b64decode(entry[name]))
Why use iterkeys if you're using both the key and the value?
for name, content in entry.iteritems():
Line 85: _commonCleanFs(files, dirname, floppy)
If any exception was raised you're leaving stale files. Maybe this should be in some
finally clause?
Line 99: _commonCleanFs(files, dirname, isopath)
Same comment about stale files here.
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4372
days inactive
4443
days old
vdsm-patches@lists.fedorahosted.org
77 comments
6 participants
participants (6)
-
Dan Kenigsberg -
ewoud＠kohlvanwijngaarden.nl -
Igor Lvovsky -
masayag＠redhat.com -
shavivi＠redhat.com -
smizrahi＠redhat.com