Copilot commented on code in PR #13207:
URL: https://github.com/apache/cloudstack/pull/13207#discussion_r3286475838
##########
scripts/vm/hypervisor/ovm3/cloudstack.py:
##########
@@ -553,24 +553,24 @@ def ping(host, count=3):
if re.search("MODULES", line):
n = cs.getName()
line = "%s\n\t%s.%s," % (line, n.lower(), n)
- print line
- print "Api inserted, %s in %s" % (cs.getName(), api)
+ print(line)
+ print("Api inserted, %s in %s" % (cs.getName(), api))
else:
- print "Api missing, %s" % (api)
+ print("Api missing, %s" % (api))
else:
- print "Api present, %s in %s" % (cs.getName(), api)
+ print("Api present, %s in %s" % (cs.getName(), api))
# either way check our version and install if checksum differs
modfile = "%s/%s.py" % (modpath, cs.getName().lower())
me = os.path.abspath(__file__)
if os.path.isfile(modfile):
if hashlib.md5(open(me).read()).hexdigest() !=
hashlib.md5(open(modfile).read()).hexdigest():
Review Comment:
`hashlib.md5(...)` requires bytes in Python 3, but `open(...).read()`
returns `str`, so this checksum comparison will raise `TypeError`. Open the
files in binary mode (`'rb'`) or encode to bytes before hashing.
##########
scripts/vm/hypervisor/ovm3/storagehealth.py:
##########
@@ -72,9 +72,9 @@ def writehb(self,file=""):
def nfsoutput(self):
command="mount -v -t nfs"
p=subprocess.Popen(command, shell=True, stdout=subprocess.PIPE)
- lines=map(lambda line: line.split()[2], p.stdout.readlines())
+ lines=[line.split()[2] for line in p.stdout.readlines()]
test=re.compile("^%s" % (primary))
- lines=filter(test.search, lines)
+ lines=list(filter(test.search, lines))
return lines
Review Comment:
`p.stdout.readlines()` returns `bytes` in Python 3, so `line.split()[2]`
becomes `bytes` and `re.compile("^%s" % primary)` (a `str` pattern) will fail
when filtering. Run the command in text mode
(`text=True`/`universal_newlines=True`) or decode the output before regex
matching.
##########
scripts/vm/hypervisor/xenserver/xenserver56/InterfaceReconfigure.py:
##########
@@ -854,7 +848,7 @@ def DatapathFactory(pif):
network_conf = open(root_prefix() + "/etc/xensource/network.conf", 'r')
network_backend = network_conf.readline().strip()
network_conf.close()
- except Exception, e:
+ except Exception as e:
raise Error("failed to determine network backend:" + e)
Review Comment:
This raises `TypeError` because it concatenates a string with an exception
object. Convert the exception to text (e.g. `str(e)`) or use string formatting
when constructing the error message.
##########
scripts/storage/secondary/cloud-install-sys-tmplt.py:
##########
@@ -90,7 +90,7 @@ def populateOptions(self):
self.databaseuserpassword = ""
if self.args.templatesuffix:
self.templatesuffix = self.args.templatesuffix
- print 'Password for DB: %s'%self.databaseuserpassword
+ print('Password for DB: %s'%self.databaseuserpassword)
Review Comment:
This prints the DB password to stdout during template install. That leaks
credentials into logs/terminal history and is risky in production; consider
removing this output or masking the password (e.g. print only whether it was
provided).
##########
scripts/vm/hypervisor/xenserver/xenserver56/InterfaceReconfigure.py:
##########
@@ -589,19 +583,19 @@ def get_vlan_record(self, vlan):
def ethtool_settings(oc):
settings = []
- if oc.has_key('ethtool-speed'):
+ if 'ethtool-speed' in oc:
val = oc['ethtool-speed']
if val in ["10", "100", "1000"]:
settings += ['speed', val]
else:
log("Invalid value for ethtool-speed = %s. Must be 10|100|1000." %
val)
- if oc.has_key('ethtool-duplex'):
+ if 'ethtool-duplex' in oc:
val = oc['ethtool-duplex']
if val in ["10", "100", "1000"]:
settings += ['duplex', 'val']
Review Comment:
The duplex setting validation is incorrect: it checks for numeric speeds
(`10/100/1000`) and appends the literal string `'val'` instead of the variable
value. This will generate wrong `ethtool` arguments. Validate against expected
duplex values (e.g. `half|full`) and append the actual `val`.
##########
scripts/storage/secondary/cloud-install-sys-tmplt.py:
##########
@@ -117,11 +117,11 @@ def runCmd(self, cmds):
def runMysql(self, query):
try:
- print 'Running Query: %s' % query
+ print('Running Query: %s' % query)
mysqlCmds = ['mysql', '--user=%s'%self.databaseusername,
'--host=%s'%self.databasehostname, '--password=%s'%self.databaseuserpassword,
'--skip-column-names', '-U', 'cloud', '-e "%s"'%query]
templateId = self.runCmd(mysqlCmds)
- print 'TemplateId is : %s' % templateId
- except Exception, e:
+ print('TemplateId is : %s' % templateId)
+ except Exception as e:
err = '''Encountering an error when executing mysql script\n%s''' %
str(e)
self.errorAndExit(err)
return templateId
Review Comment:
`runCmd()` returns raw `bytes` from `subprocess.communicate()` in Python 3,
but later `runMysql()` passes that result into `int(...)` and string
formatting. This will raise `TypeError` (e.g. `int(b"123")`) and also logs
`b'...'` prefixes. Decode/strip stdout (and stderr) to text before
returning/using it.
##########
scripts/network/ping/prepare_kickstart_kernel_initrd.py:
##########
@@ -61,14 +61,14 @@ def copy_from_nfs(src, dst):
copy_from_nfs(kernel, copy_to)
copy_from_nfs(initrd, copy_to)
- except Exception, e:
- print e
+ except Exception as e:
+ print(e)
return 1
if __name__ == "__main__":
if len(sys.argv) < 4:
- print "Usage: prepare_kickstart_kerneal_initrd.py path_to_kernel
path_to_initrd path_kernel_initrd_copy_to"
- sys.exit(1)
+ print("Usage: prepare_kickstart_kerneal_initrd.py path_to_kernel
path_to_initrd path_kernel_initrd_copy_to")
Review Comment:
Typo in usage message: `prepare_kickstart_kerneal_initrd.py` should be
`prepare_kickstart_kernel_initrd.py`. This can confuse users when copy/pasting
the command.
##########
scripts/storage/secondary/cloud-install-sys-tmplt.py:
##########
@@ -117,11 +117,11 @@ def runCmd(self, cmds):
def runMysql(self, query):
try:
- print 'Running Query: %s' % query
+ print('Running Query: %s' % query)
mysqlCmds = ['mysql', '--user=%s'%self.databaseusername,
'--host=%s'%self.databasehostname, '--password=%s'%self.databaseuserpassword,
'--skip-column-names', '-U', 'cloud', '-e "%s"'%query]
templateId = self.runCmd(mysqlCmds)
- print 'TemplateId is : %s' % templateId
- except Exception, e:
+ print('TemplateId is : %s' % templateId)
Review Comment:
The mysql CLI arguments include `-U cloud`, which is not a valid `mysql`
option (looks like a `psql` flag) and will cause the command to fail. Use the
correct option for selecting the database (e.g. `cloud`) and keep `-u/--user`,
`-h/--host`, `-p/--password`, `-e` consistent with `mysql` syntax.
##########
scripts/vm/hypervisor/xenserver/mockxcpplugin.py:
##########
@@ -44,23 +44,23 @@ def callPlugin(pluginName, func, params):
def main():
if len(sys.argv) < 3:
- print "args: pluginName funcName params"
+ print("args: pluginName funcName params")
sys.exit(1)
pluginName = sys.argv[1]
funcName = sys.argv[2]
paramList = sys.argv[3:]
if (len(paramList) % 2) != 0:
- print "params must be name/value pair"
+ print("params must be name/value pair")
sys.exit(2)
params = {}
pos = 0;
for i in range(len(paramList) / 2):
Review Comment:
`range(len(paramList) / 2)` uses `/` which returns `float` in Python 3,
causing `TypeError: 'float' object cannot be interpreted as an integer`. Use
integer division (`// 2`) or iterate over indices/paired slices.
##########
debian/cloudstack-common.postinst:
##########
@@ -21,12 +21,12 @@ set -e
CLOUDUTILS_DIR="/usr/share/pyshared/"
DIST_DIR=$(python3 -c "from distutils.sysconfig import get_python_lib;
print(get_python_lib(1))")
-if which pycompile >/dev/null 2>&1; then
- pycompile -p cloudstack-common
+if which py3compile >/dev/null 2>&1; then
+ py3compile -p cloudstack-common
fi
-if which pycompile >/dev/null 2>&1; then
- pycompile -p cloudstack-common /usr/share/cloudstack-common
+if which py3compile >/dev/null 2>&1; then
+ py3compile -p cloudstack-common /usr/share/cloudstack-common
fi
Review Comment:
`py3compile -p cloudstack-common /usr/share/cloudstack-common` will still
try to byte-compile Python 2-only scripts shipped by this package (e.g.
`scripts/vm/hypervisor/xenserver/perfmon.py` uses `xrange`,
`scripts/vm/hypervisor/xenserver/xcpserver83/NFSSR.py` uses `print >>`, etc.),
which will raise `SyntaxError` and abort the postinst due to `set -e`. Consider
removing this compilation step or restricting byte-compilation to the Python 3
modules copied into `$DIST_DIR` (or explicitly excluding legacy XenServer
scripts).
##########
scripts/network/ping/prepare_tftp_bootfile.py:
##########
@@ -72,14 +72,14 @@ def prepare(is_restore):
f.write(stuff)
f.close()
return 0
- except Exception, e:
- print e
+ except Exception as e:
+ print(e)
return 1
if __name__ == "__main__":
if len(sys.argv) < 12:
- print "Usage: prepare_tftp_bootfile.py tftp_dir mac cifs_server share
directory image_to_restor cifs_username cifs_password ip netmask gateway"
+ print("Usage: prepare_tftp_bootfile.py tftp_dir mac cifs_server share
directory image_to_restor cifs_username cifs_password ip netmask gateway")
Review Comment:
Typo in usage message argument name: `image_to_restor` should be
`image_to_restore`. This can confuse users when following the usage output.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
