Change in vdsm[master]: execCmd: Make loggign when successful optional

nsoffer at redhat.com nsoffer at redhat.com
Fri Nov 22 18:35:44 UTC 2013


Nir Soffer has posted comments on this change.

Change subject: execCmd: Make loggign when successful optional
......................................................................


Patch Set 2:

(3 comments)

I think this patch can be more useful if it allows to turn off all logging in execCmd. This should be under complete control of the caller.

....................................................
File lib/vdsm/utils.py
Line 489: 
Line 490: def execCmd(command, sudo=False, cwd=None, data=None, raw=False, logErr=True,
Line 491:             printable=None, env=None, sync=True, nice=None, ioclass=None,
Line 492:             ioclassdata=None, setsid=False, execCmdLogger=logging.root,
Line 493:             deathSignal=0, logWhenSuccessful=True):
I would call this "verbose"
Line 494:     """
Line 495:     Executes an external command, optionally via sudo.
Line 496: 
Line 497:     IMPORTANT NOTE: don't define a deathSignal when sync=False


Line 516:     if not printable:
Line 517:         printable = command
Line 518: 
Line 519:     cmdline = repr(subprocess.list2cmdline(printable))
Line 520:     execCmdLogger.debug("%s (cwd %s)", cmdline, cwd)
You should disable also this log:

    if verbose:
        # log command line here
Line 521: 
Line 522:     p = BetterPopen(command, close_fds=True, cwd=cwd, env=env,
Line 523:                     deathSignal=deathSignal)
Line 524:     p = AsyncProc(p)


Line 534:     if out is None:
Line 535:         # Prevent splitlines() from barfing later on
Line 536:         out = ""
Line 537: 
Line 538:     if logWhenSuccessful or p.returncode != 0:
You don't need to check error code here. The caller gets all the information needed to log the proper error if needed.

    if verbose:
        # log debug here

If you want to turn off only return value debug logs, you don't have to add a new argument, there is already a logErr argument. Note that Saggi rejected the last patch that tried to do that though.
Line 539:         execCmdLogger.debug(
Line 540:             "%s: <err> = %s; <rc> = %d",
Line 541:             {True: "SUCCESS", False: "FAILED"}[p.returncode == 0], repr(err),
Line 542:             p.returncode)


-- 
To view, visit http://gerrit.ovirt.org/21515
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c550aded704dd944f4f2af88b7925f9473f1a4a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list