Change in vdsm[master]: utils: add logged CPopen command execution

nsoffer at redhat.com nsoffer at redhat.com
Wed Mar 18 22:30:39 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: utils: add logged CPopen command execution
......................................................................


Patch Set 1:

(2 comments)

Logging does not belong to a library. It should be implemented by the caller, using the correct logger and log format for the context.

For example, logging the current working directory is needed only when we change it, and logging stderr when command succeeds is usually not needed.

Instead of this, I would make utils._list2cmdline public, and use it in client code (such as qemuimg).

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

Line 640:         parts.append(arg)
Line 641:     return ' '.join(parts)
Line 642: 
Line 643: 
Line 644: def loggedCPopen(args, close_fds=False, cwd=None, env=None, deathSignal=0,
Why did you change the default close_fds from True to False?
Line 645:                  childUmask=None, printable=None, logger=logging.root,
Line 646:                  level=logging.DEBUG):
Line 647:     if not printable:
Line 648:         printable = args


Line 684:         command = tuple(command)
Line 685: 
Line 686:     p = loggedCPopen(command, close_fds=True, cwd=cwd, env=env,
Line 687:                      deathSignal=deathSignal, childUmask=childUmask,
Line 688:                      printable=printable)
You need to pass execCmdLogger down to loggedCPopen, otherwise the log before the command will go to logging.root instead of the requested logger.
Line 689:     if not sync:
Line 690:         p = AsyncProc(p)
Line 691:         if data is not None:
Line 692:             p.stdin.write(data)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e04bde10be24457fe12362456e42528f9fd7196
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list