Zhou Zheng Sheng has posted comments on this change.
Change subject: Extend vdsm-tool: moving configure libvirt to external shell
script
......................................................................
Patch Set 17: I would prefer that you didn't submit this
(4 inline comments)
I'm not comfortable with using sys.argv in libvirt_configure.py, because the
top level vdsm-tool has already prepared a clean arg list for each command. I
also find a disagreement on the command name of testing configuration conflict
in vdsmd.init.in and libvirt_configure.py.
....................................................
File lib/vdsm/tool/libvirt_configure.py
Line 23: from vdsm import utils
Line 24: import vdsm.tool
Line 25:
Line 26:
Line 27: def exec_libvirt_configure(action, force, *args):
It seems the "args" is not used, either use it, or delete it from the function
interfaces of exec_libvirt_configure and test_conflict_configurations.
Line 28: """
Line 29: Invoke libvirt_configure.sh script
Line 30: """
Line 31: if os.getuid() != 0:
Line 46: return 0
Line 47:
Line 48:
Line 49: @vdsm.tool.expose("libvirt-configure")
Line 50: def configure_libvirt(*args):
I think the "args" argument is better than sys.argv.
If len(args) >= 1 and args[0] == '--force':
force = 'force'
agrs = args[1:]
I see that the "*args" is not passed to exec_libvirt_configure(), does this
mean we just use the default conf paths and do not allow overriding the path
anymore? If it does, we can use "len(args) == 1" instead of "len(args) >= 1".
Line 51: """
Line 52: libvirt configuration (--force for reconfigure)
Line 53: """
Line 54: force = ''
....................................................
File lib/vdsm/tool/libvirt_configure.sh.in
Line 1: #! /bin/sh
Better in a dedicated patch.
Line 2: # Copyright 2013 Red Hat, Inc.
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
....................................................
File vdsm/vdsmd.init.in
Line 235: fi
Line 236:
Line 237: test_already_running && return 0
Line 238:
Line 239: if ! (test_space && test_lo && \
Does it need to be changed to "libvirt-test-conflicts"?
Line 240: "$VDSM_TOOL" test-conflicts); then
Line 241: return 1
Line 242: fi
Line 243:
--
To view, visit http://gerrit.ovirt.org/15216
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id58b129afbf141a47a85b421961bf5b1776b41e4
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Bala.FA <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
