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

fromani at redhat.com fromani at redhat.com
Wed Nov 4 08:47:15 UTC 2015


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


-- 
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 <fromani at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list