automation@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'])

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:

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

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

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.

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

automation@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'])

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

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

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

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

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: