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 <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
