Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-26 Thread danken 
Dan Kenigsberg has submitted this change and it was merged.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Revert "supervdsm: Decorator for supervdsm proxied calls."

I've merged commit 9fad76d2075f315ebe48212508fe9b677d74dfc8
prematurely, before proper discussion about its merits and
disadvantages.

One disadvantage is the relative ease of decorating a
function and then, implicitly, having it run as root in all
contexts.

Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Signed-off-by: Dan Kenigsberg 
Reviewed-on: https://gerrit.ovirt.org/45316
Reviewed-by: Piotr Kliczewski 
---
M lib/vdsm/supervdsm.py
M tests/functional/supervdsmFuncTests.py
M vdsm/supervdsmServer
3 files changed, 9 insertions(+), 76 deletions(-)

Approvals:
  Piotr Kliczewski: Looks good to me, approved
  Dan Kenigsberg: Verified; Passed CI tests



-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Dima Kuznetsov 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-26 Thread automation 
automat...@ovirt.org has posted comments on this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Dima Kuznetsov 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-26 Thread danken 
Dan Kenigsberg has posted comments on this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 2: Continuous-Integration+1

unrelated CI failure:

12:08:13 MountError: (32, ';mount: 
/tmp/tmp9C_d3L/vmId_iso.fc07980fb2a98d5871cf1c3ce8a83081.img: failed to setup 
loop device: No such file or directory\n')

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Dima Kuznetsov 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-26 Thread piotr . kliczewski 
Piotr Kliczewski has posted comments on this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 2: Code-Review+2

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Dima Kuznetsov 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-26 Thread automation 
automat...@ovirt.org has posted comments on this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dima Kuznetsov 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-26 Thread danken 
Dan Kenigsberg has posted comments on this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 2: Verified+1

minor rebase needed

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Dima Kuznetsov 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-26 Thread piotr . kliczewski 
Piotr Kliczewski has posted comments on this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 1: Code-Review+2

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dima Kuznetsov 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-25 Thread dkuznets 
Dima Kuznetsov has posted comments on this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 1:

I think the ease of use is not a vice. It faces only the developer of the 
function, and not the user, and allows you keep all the code related to some 
topic in the same place (vs another method in supervsdmServer).

IMO it's irrelevant for whoever uses a specific function, to know whether it 
should be called through supervdsm proxy or otherwise. One such example is 
reading hardware info, you care about getting the system data, implementation 
details (such that dmidecode requires privileges) should be part of the 
concerns. And I'd trust whoever writes the module to tag only the relevant 
parts for supervdsm invocation.

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dima Kuznetsov 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-25 Thread automation 
automat...@ovirt.org has posted comments on this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-25 Thread nsoffer 
Nir Soffer has posted comments on this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dima Kuznetsov 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-25 Thread danken 
Dan Kenigsberg has uploaded a new change for review.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..

Revert "supervdsm: Decorator for supervdsm proxied calls."

I've merged commit 9fad76d2075f315ebe48212508fe9b677d74dfc8
prematurely, before proper discussion about its merits and
disadvantages.

One disadvantage is the relative ease of decorating a
function and then, implicitly, having it run as root in all
contexts.

Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Signed-off-by: Dan Kenigsberg 
---
M lib/vdsm/supervdsm.py
M tests/functional/supervdsmFuncTests.py
M vdsm/supervdsmServer
3 files changed, 9 insertions(+), 77 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/16/45316/1

diff --git a/lib/vdsm/supervdsm.py b/lib/vdsm/supervdsm.py
index 018734f..8e42af2 100644
--- a/lib/vdsm/supervdsm.py
+++ b/lib/vdsm/supervdsm.py
@@ -19,7 +19,6 @@
 # Refer to the README and COPYING files for full details of the license
 #
 
-import functools
 import os
 from multiprocessing.managers import BaseManager, RemoteError
 import logging
@@ -94,20 +93,3 @@
 if _g_singletonSupervdsmInstance is None:
 _g_singletonSupervdsmInstance = SuperVdsmProxy()
 return _g_singletonSupervdsmInstance
-
-
-def proxied_call(func):
-func.allow_supervdsm_invocation = True
-
-@functools.wraps(func)
-def wrapper(*args, **kwargs):
-if os.geteuid() == 0:
-return func(*args, **kwargs)
-else:
-return getProxy().proxied_call(
-func.__module__,
-func.func_name,
-args,
-kwargs
-)
-return wrapper
diff --git a/tests/functional/supervdsmFuncTests.py 
b/tests/functional/supervdsmFuncTests.py
index af15433..93e7c54 100644
--- a/tests/functional/supervdsmFuncTests.py
+++ b/tests/functional/supervdsmFuncTests.py
@@ -17,45 +17,23 @@
 # Refer to the README and COPYING files for full details of the license
 #
 from testlib import VdsmTestCase as TestCaseBase
-import monkeypatch
-from vdsm import hwinfo
+import testValidation
 from vdsm import supervdsm
-from vdsm import utils
 from vdsm.constants import VDSM_USER
 from pwd import getpwnam
 import os
 
-_test_list = []
 
+class TestSuperVdsmRemotly(TestCaseBase):
 
-@supervdsm.proxied_call
-def foo(arg):
-_test_list.append(arg)
+def dropPrivileges(self):
+vdsm_uid, vdsm_gid = getpwnam(VDSM_USER)[2:4:]
+os.setgroups([])
+os.setgid(vdsm_gid)
+os.setuid(vdsm_uid)
 
-
-def dropPrivileges():
-vdsm_uid, vdsm_gid = getpwnam(VDSM_USER)[2:4:]
-os.setgroups([])
-os.setgid(vdsm_gid)
-os.setuid(vdsm_uid)
-dropPrivileges()
-
-
-class TestSuperVdsmRemotely(TestCaseBase):
+@testValidation.ValidateRunningAsRoot
 def testPingCall(self):
+self.dropPrivileges()
 proxy = supervdsm.getProxy()
 self.assertTrue(proxy.ping())
-
-@monkeypatch.MonkeyPatch(os, 'geteuid', lambda: 0)
-def test_proxied_call_run_as_root(self):
-o = object()
-foo(o)
-self.assertIn(o, _test_list)
-
-def test_proxied_call_fails_on_unknown_function(self):
-with self.assertRaises(ImportError):
-foo()
-
-def test_proxied_call_non_decorated_function(self):
-with self.assertRaises(RuntimeError):
-supervdsm.proxied_call(utils.tobool)(None)
diff --git a/vdsm/supervdsmServer b/vdsm/supervdsmServer
index 41d5590..e836368 100755
--- a/vdsm/supervdsmServer
+++ b/vdsm/supervdsmServer
@@ -18,7 +18,6 @@
 # Refer to the README and COPYING files for full details of the license
 #
 from pwd import getpwnam
-import importlib
 import platform
 import sys
 import os
@@ -474,33 +473,6 @@
 
 def __udevOperationReload(self):
 return self.__udevVersion() > self.UDEV_WITH_RELOAD_VERSION
-
-def proxied_call(self, mod_name, func_name, args, kwargs):
-self.log.debug(
-'proxied call of %s.%s with %s, %s',
-mod_name,
-func_name,
-args,
-kwargs,
-)
-
-if mod_name not in sys.modules:
-self.log.debug('importing module %s', mod_name)
-importlib.import_module(mod_name)
-
-mod = sys.modules.get(mod_name)
-func = getattr(mod, func_name)
-if not getattr(func, 'allow_supervdsm_invocation', False):
-raise RuntimeError('This function cannot be called by supervdsm')
-
-try:
-ret = func(*args, **kwargs)
-except:
-self.log.exception('error in %s.%s', mod_name, func_name)
-raise
-
-self.log.debug('%s.%s returned with %s', mod_name, func_name, ret)
-return ret
 
 
 def terminate(signo, frame):


-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange

Change in vdsm[master]: Revert "supervdsm: Decorator for supervdsm proxied calls."

2015-08-25 Thread danken 
Dan Kenigsberg has reverted this change.

Change subject: Revert "supervdsm: Decorator for supervdsm proxied calls."
..


Patch Set 7: Reverted

This patchset was reverted in change: I1f83b4a8820f3fef437641329ba514742434dab4

-- 
To view, visit https://gerrit.ovirt.org/45316
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: revert
Gerrit-Change-Id: I1f83b4a8820f3fef437641329ba514742434dab4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches