stag7824 commented on code in PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#discussion_r4297367034
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -217,6 +217,7 @@ private String mountBackupDirectory(String
backupRepoAddress, String backupRepoT
mountCmd.add(backupRepoType);
mountCmd.add(backupRepoAddress);
mountCmd.add(mountDirectory);
+ mountOptions = normalizeMountOptions(mountOptions);
Review Comment:
Yes, that's the better place. I've moved it to `NASBackupProvider`
(4b4dd3f): the five places that pass `backupRepository.getMountOptions()` to a
command (take, both restores, delete, stats) now go through one helper, and the
change in the KVM restore wrapper is reverted. So backup, delete and stats get
the same option list as restore, and old rows keep working once #14009 quotes
the options.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -242,6 +243,31 @@ private String mountBackupDirectory(String
backupRepoAddress, String backupRepoT
return mountDirectory;
}
+ /**
+ * Removes the whitespace around each option of a comma-separated mount
option list.
+ *
+ * mount(8) is handed the list as a single argument and libmount splits it
on commas only, so a
+ * blank next to a comma, or at either end of the list, stays glued to the
neighbouring option
+ * and is rejected as part of its name or value.
+ *
+ * Only the whitespace around the delimiters is removed. The option text
itself is passed
+ * through unchanged, as rejecting unsafe or malformed options belongs to
the API layer, not
+ * to the agent.
+ */
+ protected static String normalizeMountOptions(String mountOptions) {
+ if (StringUtils.isBlank(mountOptions)) {
+ return mountOptions;
+ }
+ List<String> options = new ArrayList<>();
+ for (String option : mountOptions.split(",")) {
+ String trimmedOption = option.trim();
+ if (!trimmedOption.isEmpty()) {
Review Comment:
Good catch, dropping the empty option would turn `pa,,ss` into `pa,ss`. Now
only options that are whitespace are dropped; an empty one is kept, with a test
for a password containing a double comma.
For what it's worth, `mount.cifs` itself splits on every comma before the
kernel sees the list, and its man page notes that a password containing a comma
fails on the command line, so this mostly matters for the kernel path, but the
normalisation shouldn't make it worse either way.
--
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]
