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


-- 
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 <from...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com>
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

Reply via email to