Copilot commented on code in PR #11318:
URL: https://github.com/apache/cloudstack/pull/11318#discussion_r2433326009
##########
python/lib/cloudutils/networkConfig.py:
##########
@@ -45,8 +45,11 @@ def getDefaultNetwork():
if not cmd.isSuccess():
logging.debug("Failed to get default route")
raise CloudRuntimeException("Failed to get default route")
-
- result = cmd.getStdout().split(" ")
+ output = cmd.getStdout().strip()
+ result = output.split(" ")
+ if len(result) < 2:
+ logging.debug("Output for the default route incomplete: %s. It
should have been '<GATEWAY> <DEVICE>'" % output)
+ raise CloudRuntimeException("Output for the default route
incomplete")
gateway = result[0]
dev = result[1]
Review Comment:
Splitting by a literal space can yield empty tokens when the output contains
multiple spaces or tabs (common in shell output), causing dev to become an
empty string despite len(result) >= 2. Use whitespace splitting to normalize
tokens, e.g., parts = output.split(), then gateway = parts[0], dev = parts[1],
or use a regex like m = re.match(r'(\\S+)\\s+(\\S+)', output).
##########
python/lib/cloudutils/networkConfig.py:
##########
@@ -45,8 +45,11 @@ def getDefaultNetwork():
if not cmd.isSuccess():
logging.debug("Failed to get default route")
raise CloudRuntimeException("Failed to get default route")
-
- result = cmd.getStdout().split(" ")
+ output = cmd.getStdout().strip()
+ result = output.split(" ")
+ if len(result) < 2:
+ logging.debug("Output for the default route incomplete: %s. It
should have been '<GATEWAY> <DEVICE>'" % output)
+ raise CloudRuntimeException("Output for the default route
incomplete")
Review Comment:
The raised exception lacks context for troubleshooting. Consider including
the actual output and the expected format in the exception message, e.g., raise
CloudRuntimeException(\"Output for the default route incomplete: '%s' (expected
'<GATEWAY> <DEVICE>')\" % output).
```suggestion
raise CloudRuntimeException("Output for the default route
incomplete: '%s' (expected '<GATEWAY> <DEVICE>')" % output)
```
##########
python/lib/cloudutils/configFileOps.py:
##########
@@ -68,14 +68,14 @@ def save(self):
for entry in self.entries:
if entry.op == "add":
if entry.separator == "=":
- matchString = "^\ *" + entry.name + ".*"
+ matchString = r"^\ *" + entry.name + ".*"
elif entry.separator == " ":
- matchString = "^\ *" + entry.name + "\ *" +
entry.value
+ matchString = r"^\ *" + entry.name + r"\ *" +
entry.value
else:
if entry.separator == "=":
- matchString = "^\ *" + entry.name + "\ *=\ *" +
entry.value
+ matchString = r"^\ *" + entry.name + r"\ *=\ *" +
entry.value
else:
- matchString = "^\ *" + entry.name + "\ *" +
entry.value
+ matchString = r"^\ *" + entry.name + r"\ *" +
entry.value
Review Comment:
These regexes should use \\s* (or \\s+) for whitespace and escape dynamic
components to prevent unintended regex metacharacter interpretation. Example:
name = re.escape(entry.name); value = re.escape(entry.value); then build
patterns with r'^\\s*' + name + r'\\s*=\\s*' + value (or r'^\\s*' + name +
r'\\s+' + value). Also, r'\\ *' is unusual; prefer clear whitespace classes.
##########
python/lib/cloudutils/utilities.py:
##########
@@ -63,7 +63,7 @@ def getStdout(self):
return self.stdout.decode('utf-8').strip('\n')
def getLines(self):
- return self.stdout.decode('utf-8').strip('\n')
+ return self.stdout.decode('utf-8').strip('\n').split('\n')
Review Comment:
Use splitlines() to robustly handle different newline conventions and avoid
edge cases with trailing newlines: return
self.stdout.decode('utf-8').splitlines().
```suggestion
return self.stdout.decode('utf-8').splitlines()
```
--
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]
