Change in vdsm[master]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 7:

Waiting for Dan

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has submitted this change and it was merged.

Change subject: lib: utils: consolidate Error class in one place
..


lib: utils: consolidate Error class in one place

Few utilities code have a duplicate Error exception, that
they raise when one command run through utils.execCmd fails.

Since this Error is closely related to utils.execCmd, we
remove the duplicate definition of Error and we move it
in one place.

Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Signed-off-by: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/47964
Reviewed-by: Piotr Kliczewski 
Reviewed-by: Nir Soffer 
Continuous-Integration: Jenkins CI
---
M lib/vdsm/cmdutils.py
M lib/vdsm/taskset.py
M lib/vdsm/udevadm.py
M tests/tasksetTests.py
M vdsm/supervdsmServer
5 files changed, 25 insertions(+), 33 deletions(-)

Approvals:
  Piotr Kliczewski: Looks good to me, but someone else must approve
  Nir Soffer: Looks good to me, approved
  Jenkins CI: Passed CI tests
  Francesco Romani: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[automation]

gerrit-hooks has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 8:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[automation]

gerrit-hooks has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 7:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[fromani]

Francesco Romani has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/5/lib/vdsm/cmdutils.py
File lib/vdsm/cmdutils.py:

Line 29: 
Line 30: 
Line 31: class Error(Exception):
Line 32: 
Line 33: def __init__(self, rc, out, err):
> Since this is a common error, we need now more context about the failure.
Done in patch https://gerrit.ovirt.org/48628

I now see that we have a pattern:

# do other stuff, build `command`

rc, out, err = utils.execCmd(command, ...)

if rc != 0:
raise cmdutils.Error(command, rc, out, err)

# continue as usual

it would be much nicer and friendlier to wrap this in an utility helper. 
execCmd is already too bulky, so I wonder how properly do that.

Maybe some kind of magic decorator wrapper around execCmd?
Line 34: self.rc = rc
Line 35: self.out = out
Line 36: self.err = err
Line 37: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/5/lib/vdsm/cmdutils.py
File lib/vdsm/cmdutils.py:

Line 29: 
Line 30: 
Line 31: class Error(Exception):
Line 32: 
Line 33: def __init__(self, rc, out, err):
> Done in patch https://gerrit.ovirt.org/48628
I think we should have standard way to invoke commands, like subprocess module:

- check_call() - raise cmdutils.Error on failures - this should be the default 
for most code, so errors are never ignored
- call() - return rc, out, err for the odd commands when non-zero error code is 
 not always an error.

Lets tackle this later.
Line 34: self.rc = rc
Line 35: self.out = out
Line 36: self.err = err
Line 37: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[automation]

gerrit-hooks has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 6:

* 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.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 6: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[fromani]

Francesco Romani has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 6: Verified+1

trivial patches that moves code and fixes import. Did basic sanity check 
setting the affinity to inexistent CPU, VDSM raised as expected.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 6: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 5:

* 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/47964
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: lib: utils: consolidate Error class in one place

[fromani]

Francesco Romani has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/4//COMMIT_MSG
Commit Message:

Line 10: they raise when one command run through utils.execCmd fails.
Line 11: 
Line 12: Since this Error is closely related to utils.execCmd, we
Line 13: remove the duplicate definition of Error and we move it
Line 14: in one place, alongside execCmd itself.
> You did not move execCmd in this patch (and lets keep it simple and not mov
Good point. Removed.
Line 15: 
Line 16: Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/5/lib/vdsm/cmdutils.py
File lib/vdsm/cmdutils.py:

Line 31: Error
As in the commit message this Error is closely related to utils.execCmd so 
maybe more meaningful name would give more information in which context it is 
used.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/5/lib/vdsm/cmdutils.py
File lib/vdsm/cmdutils.py:

Line 27: 
Line 28: SUDO_NON_INTERACTIVE_FLAG = "-n"
Line 29: 
Line 30: 
Line 31: class Error(Exception):
> As in the commit message this Error is closely related to utils.execCmd so 
The name is meaningful - cmdutils.Error

You are supposed to do:

import cmdutils

try:
...
except cmdutils.Error:
...

Just like socket.error, thread.error.
Line 32: 
Line 33: def __init__(self, rc, out, err):
Line 34: self.rc = rc
Line 35: self.out = out


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/5/lib/vdsm/cmdutils.py
File lib/vdsm/cmdutils.py:

Line 27: 
Line 28: SUDO_NON_INTERACTIVE_FLAG = "-n"
Line 29: 
Line 30: 
Line 31: class Error(Exception):
> The name is meaningful - cmdutils.Error
is that socket.error or socket.Error? Do we want to follow the same practices?
Line 32: 
Line 33: def __init__(self, rc, out, err):
Line 34: self.rc = rc
Line 35: self.out = out


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/5/lib/vdsm/cmdutils.py
File lib/vdsm/cmdutils.py:

Line 27: 
Line 28: SUDO_NON_INTERACTIVE_FLAG = "-n"
Line 29: 
Line 30: 
Line 31: class Error(Exception):
> is that socket.error or socket.Error? Do we want to follow the same practic
socket.error is not pep8 compatible (class name should be CamelCase), we should 
not follow this detail.
Line 32: 
Line 33: def __init__(self, rc, out, err):
Line 34: self.rc = rc
Line 35: self.out = out


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 5: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 5: Code-Review-1

(1 comment)

One issue that may need little more work, we can also do this in a separate 
patch.

-1 for visibility.

https://gerrit.ovirt.org/#/c/47964/5/lib/vdsm/cmdutils.py
File lib/vdsm/cmdutils.py:

Line 29: 
Line 30: 
Line 31: class Error(Exception):
Line 32: 
Line 33: def __init__(self, rc, out, err):
Since this is a common error, we need now more context about the failure.

Lets add the command that failed:

def __init__(self, cmd, rc, out, err):
self.cmd = cmd
...

def __str__(self):
return "Command %s failed ..." % (self.cmd, ...)

These error will be very verbose, but debugging will be a joy.
Line 34: self.rc = rc
Line 35: self.out = out
Line 36: self.err = err
Line 37: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[fromani]

Francesco Romani has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/udevadm.py
File lib/vdsm/udevadm.py:

Line 23
Line 24
Line 25
Line 26
Line 27
> Good point. Lets avoid this issue by dropping udevadm.Error and taskset.Err
Good point indeed. So, done, no more cutting of corners :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 3:

* 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/47964
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/4//COMMIT_MSG
Commit Message:

Line 10: they raise when one command run through utils.execCmd fails.
Line 11: 
Line 12: Since this Error is closely related to utils.execCmd, we
Line 13: remove the duplicate definition of Error and we move it
Line 14: in one place, alongside execCmd itself.
You did not move execCmd in this patch (and lets keep it simple and not move it 
now), so better remove that part of the sentence.
Line 15: 
Line 16: Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 3:

Fine for me now except the "alogside" typo in the commit message.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: lib: utils: consolidate Error class in one place

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 4:

* 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/47964
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: lib: utils: consolidate Error class in one place

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 4: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: lib: utils: consolidate Error class in one place

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 1:

(2 comments)

https://gerrit.ovirt.org/#/c/47964/1//COMMIT_MSG
Commit Message:

Line 10: they raise when one command run through utils.execCmd fails.
Line 11: 
Line 12: Since this Error is closely related to utils.execCmd, we
Line 13: remove the duplicate definition of Error and we move it
Line 14: in one place, alogside execCmd itself.
... alongside ...
Line 15: 
Line 16: Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5


https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/udevadm.py
File lib/vdsm/udevadm.py:

Line 23
Line 24
Line 25
Line 26
Line 27
> Could be good solution for a future patch, when we have a clear need. For t
Referring the same class via different paths raises some red flags in my head. 
I also can't say it's an ultimately bad thing to do, but I can e.g. imagine 
someone, once having the clear need without realizing the aliases, debugging 
his code for a while and then hitting his head against a wall. Not a big issue 
for me, just thinking aloud about what "simplest" actually means here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[fromani]

Francesco Romani has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 1:

(6 comments)

https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/taskset.py
File lib/vdsm/taskset.py:

Line 19: #
Line 20: 
Line 21: from __future__ import absolute_import
Line 22: 
Line 23: from .utils import Error
> Francesco, I don't think you cut corners, having an alias of utils.Error he
OK, then let's have the alias as you showed, which avoids one from ... import.
Line 24: from . import constants
Line 25: from . import utils
Line 26: 
Line 27: 


Line 32: this is the only usecase VDSM cares about - and requires.
Line 33: Return a frozenset of ints, each one being a cpu indices on which 
the
Line 34: process can run.
Line 35: Example: frozenset([0, 1, 2, 3])
Line 36: Raise Error on failure.
> I think it is better to keep the module name, it make it clear what is the 
will drop this change, since we will have aliases.
Line 37: """
Line 38: command = [constants.EXT_TASKSET, '--pid', str(pid)]
Line 39: 
Line 40: rc, out, err = utils.execCmd(command, resetCpuAffinity=False)


Line 52: the target process.
Line 53:  must be an iterable whose items are ints which represent
Line 54: cpu indices, on which the process will be allowed to run; the 
format
Line 55: is the same as what the get() function returns.
Line 56: Raise Error on failure.
> Same
Done
Line 57: """
Line 58: command = [constants.EXT_TASKSET]
Line 59: if all_tasks:
Line 60: command.append("--all-tasks")


https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/udevadm.py
File lib/vdsm/udevadm.py:

Line 23
Line 24
Line 25
Line 26
Line 27
> I wonder if we should do intead:
Could be good solution for a future patch, when we have a clear need. For this 
patch I'll stick with aliases, being the simplest solution that works.


Line 19: #
Line 20: 
Line 21: from __future__ import absolute_import
Line 22: import logging
Line 23: from .utils import Error
> Same as taskset.
Done
Line 24: from . import utils
Line 25: 
Line 26: _UDEVADM = utils.CommandPath("udevadm", "/sbin/udevadm", 
"/usr/sbin/udevadm")
Line 27: 


https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 674: 
Line 675: return p.returncode, out, err
Line 676: 
Line 677: 
Line 678: class Error(Exception):
> Lets move this into cmdutils.
way better, thanks. execCmd will be moved in a future patch.
Line 679: 
Line 680: def __init__(self, rc, out, err):
Line 681: self.rc = rc
Line 682: self.out = out


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


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/47964
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/udevadm.py
File lib/vdsm/udevadm.py:

Line 23
Line 24
Line 25
Line 26
Line 27
> Referring the same class via different paths raises some red flags in my he
Good point. Lets avoid this issue by dropping udevadm.Error and taskset.Error, 
since we don't have a need for these error currently.

Instead, lets raise and handle cmdutils.Error everywhere. If we have a need 
later to handle specific errors differently, we can always add a subclass in 
some module.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[fromani]

Francesco Romani has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/taskset.py
File lib/vdsm/taskset.py:

Line 19: #
Line 20: 
Line 21: from __future__ import absolute_import
Line 22: 
Line 23: from .utils import Error
> Not really needed due to utils imported later - I'd say raise utils.Error r
Yes, here I cut one corner. We have one problem in client code:

- now existing code is catching taskset.Error (or udevadm.Error, or whatever), 
which is nice and expressive
- after this change, we either add bogus import (ugly and repetitive) or we 
catch utils.Error, which is a little ugly and reads worse.
Line 24: from . import constants
Line 25: from . import utils
Line 26: 
Line 27: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[piotr . kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
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]: lib: utils: consolidate Error class in one place

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 1:

(5 comments)

https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/taskset.py
File lib/vdsm/taskset.py:

Line 19: #
Line 20: 
Line 21: from __future__ import absolute_import
Line 22: 
Line 23: from .utils import Error
> Yes, here I cut one corner. We have one problem in client code:
Francesco, I don't think you cut corners, having an alias of utils.Error here 
seems like a good solution.

If you want to make it more clear that we want to have this alias, we can do:

Error = utils.Error
Line 24: from . import constants
Line 25: from . import utils
Line 26: 
Line 27: 


Line 32: this is the only usecase VDSM cares about - and requires.
Line 33: Return a frozenset of ints, each one being a cpu indices on which 
the
Line 34: process can run.
Line 35: Example: frozenset([0, 1, 2, 3])
Line 36: Raise Error on failure.
I think it is better to keep the module name, it make it clear what is the 
error raised. And this change is also not related to using the same Error for 
taskset and udevadm.
Line 37: """
Line 38: command = [constants.EXT_TASKSET, '--pid', str(pid)]
Line 39: 
Line 40: rc, out, err = utils.execCmd(command, resetCpuAffinity=False)


Line 52: the target process.
Line 53:  must be an iterable whose items are ints which represent
Line 54: cpu indices, on which the process will be allowed to run; the 
format
Line 55: is the same as what the get() function returns.
Line 56: Raise Error on failure.
Same
Line 57: """
Line 58: command = [constants.EXT_TASKSET]
Line 59: if all_tasks:
Line 60: command.append("--all-tasks")


https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/udevadm.py
File lib/vdsm/udevadm.py:

Line 23
Line 24
Line 25
Line 26
Line 27
I wonder if we should do intead:

class Error(utils.Error):
pass

So code can handle differently errors from different tools.

I don't see a need for this in current code, so maybe it is better to have an 
alias like we have here (udevadm.Error is utils.Error).


https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 674: 
Line 675: return p.returncode, out, err
Line 676: 
Line 677: 
Line 678: class Error(Exception):
Lets move this into cmdutils.

We should also move execCmd there later - anything related to running commands 
should be there. Will also solve circular deps between utils and cmdutils.
Line 679: 
Line 680: def __init__(self, rc, out, err):
Line 681: self.rc = rc
Line 682: self.out = out


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[fromani]

Francesco Romani has uploaded a new change for review.

Change subject: lib: utils: consolidate Error class in one place
..

lib: utils: consolidate Error class in one place

Few utilities code have a duplicate Error exception, that
they raise when one command run through utils.execCmd fails.

Since this Error is closely related to utils.execCmd, we
remove the duplicate definition of Error and we move it
in one place, alogside execCmd itself.

Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Signed-off-by: Francesco Romani 
---
M lib/vdsm/taskset.py
M lib/vdsm/udevadm.py
M lib/vdsm/utils.py
3 files changed, 16 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/64/47964/1

diff --git a/lib/vdsm/taskset.py b/lib/vdsm/taskset.py
index 089d37b..2ec6a16 100644
--- a/lib/vdsm/taskset.py
+++ b/lib/vdsm/taskset.py
@@ -20,20 +20,9 @@
 
 from __future__ import absolute_import
 
+from .utils import Error
 from . import constants
 from . import utils
-
-
-class Error(Exception):
-
-def __init__(self, rc, out, err):
-self.rc = rc
-self.out = out
-self.err = err
-
-def __str__(self):
-return "Process failed with rc=%d out=%r err=%r" % (
-self.rc, self.out, self.err)
 
 
 def get(pid):
@@ -44,7 +33,7 @@
 Return a frozenset of ints, each one being a cpu indices on which the
 process can run.
 Example: frozenset([0, 1, 2, 3])
-Raise taskset.Error on failure.
+Raise Error on failure.
 """
 command = [constants.EXT_TASKSET, '--pid', str(pid)]
 
@@ -64,7 +53,7 @@
  must be an iterable whose items are ints which represent
 cpu indices, on which the process will be allowed to run; the format
 is the same as what the get() function returns.
-Raise taskset.Error on failure.
+Raise Error on failure.
 """
 command = [constants.EXT_TASKSET]
 if all_tasks:
diff --git a/lib/vdsm/udevadm.py b/lib/vdsm/udevadm.py
index 35970fe..e4e3a9c 100644
--- a/lib/vdsm/udevadm.py
+++ b/lib/vdsm/udevadm.py
@@ -20,21 +20,10 @@
 
 from __future__ import absolute_import
 import logging
+from .utils import Error
 from . import utils
 
 _UDEVADM = utils.CommandPath("udevadm", "/sbin/udevadm", "/usr/sbin/udevadm")
-
-
-class Error(Exception):
-
-def __init__(self, rc, out, err):
-self.rc = rc
-self.out = out
-self.err = err
-
-def __str__(self):
-return "Process failed with rc=%d out=%r err=%r" % (
-self.rc, self.out, self.err)
 
 
 def settle(timeout, exit_if_exists=None):
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py
index 31d8529..d16f059 100644
--- a/lib/vdsm/utils.py
+++ b/lib/vdsm/utils.py
@@ -675,6 +675,18 @@
 return p.returncode, out, err
 
 
+class Error(Exception):
+
+def __init__(self, rc, out, err):
+self.rc = rc
+self.out = out
+self.err = err
+
+def __str__(self):
+return "Process failed with rc=%d out=%r err=%r" % (
+self.rc, self.out, self.err)
+
+
 def stripNewLines(lines):
 return [l[:-1] if l.endswith('\n') else l for l in lines]
 


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: utils: consolidate Error class in one place

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


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/47964
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
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]: lib: utils: consolidate Error class in one place

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: lib: utils: consolidate Error class in one place
..


Patch Set 1: Code-Review+1

(2 comments)

A little concern regarding importing Error directly to module's namespaces - 
I'd rather see 'utils' namespace used. Opinionated, therefore not -1 worthy.

On the other hand, big thanks for taking the initiative - this is very needed 
effort.

https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/taskset.py
File lib/vdsm/taskset.py:

Line 19: #
Line 20: 
Line 21: from __future__ import absolute_import
Line 22: 
Line 23: from .utils import Error
Not really needed due to utils imported later - I'd say raise utils.Error 
rather then directly importing it. Minor.
Line 24: from . import constants
Line 25: from . import utils
Line 26: 
Line 27: 


https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/udevadm.py
File lib/vdsm/udevadm.py:

Line 19: #
Line 20: 
Line 21: from __future__ import absolute_import
Line 22: import logging
Line 23: from .utils import Error
Same as taskset.
Line 24: from . import utils
Line 25: 
Line 26: _UDEVADM = utils.CommandPath("udevadm", "/sbin/udevadm", 
"/usr/sbin/udevadm")
Line 27: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches