Update dscheck_VMware's rpctool check by sshedi · Pull Request #970 · canonical/cloud-init

sshedi · 2021-08-11T19:31:01Z

Other minor tweaks.

Signed-off-by: Shreenidhi Shedi sshedi@vmware.com

Proposed Commit Message

Update dscheck_VMware's rpctool check

Update dscheck_VMware's rpctool check

This patch updates the dscheck_VMware function's use of "vmware-rpctool".

When checking to see if a "guestinfo" property is set.
Because a successful exit code can occur even if there is an empty
string returned, it is possible that the VMware datasource will be
loaded as a false-positive. This patch ensures that in addition to
validating the exit code, the emitted output is also examined to ensure
a non-empty value is returned by rpctool before returning "${DS_FOUND}"
from "dscheck_VMware()".

Other minor tweaks in ds-identify script.

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

sshedi · 2021-08-11T19:35:03Z

@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash


@TheRealFalcon @blackboxsw
This script is using local keyword in almost all functions & local is not POSIX compliant (shellcheck warns the same).
Can we use bash by default? Ubuntu links sh->dash and I'm unsure about other systems.
I'm assuming that bash is available in all distros by default.

I would focus this PR on improving dscheck_VMware and separate general script improvements into a separate change.

I think I got my answer by CI test results. It will be nice to know your clarification.

sshedi · 2021-08-11T19:42:42Z

    cached "$DI_VIRT" && return 0
    detect_virt
-    DI_VIRT=${_RET}
+    DI_VIRT="$(echo "${_RET}" | tr '[:upper:]' '[:lower:]')"


Just playing safe here, ensuring that DI_VIRT is lower case.

If the interpreter is BASH then the following should be possible:

DI_VIRT="${_RET,,}"

Nope, can't assume that. sh is bash only in some distros. So, we need to use POSIX complaint method.

Oh sure, I was not assuming it. I was saying if we could switch the interpreter explicitly to BASH as you requested elsewhere.

Do you happen to know if DASH supports the same pattern?

DI_VIRT="${_RET,,}" doesn't work on dash. Checked it. It will be nice if we can use bash but it seems like there are distros which doesn't use bash. #!/bin/sh is a symbolic link to a posix complaint shell, it can be bash/ksh/dash or anything. Using bash seems not feasible.

And yet local is used elsewhere, huh? Weird. Seems like if /bin/sh is used then the rest of the file should be strictly POSIX compliant...

Exactly, that's what I'm thinking as well. But I read somewhere that even though local is not posix complaint, it's a de facto standard.

akutz · 2021-08-11T19:48:53Z

Thank you for doing this @sshedi. However, can you please provide a more verbose and meaningful commit message?

akutz · 2021-08-11T19:49:30Z

@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash


I would focus this PR on improving dscheck_VMware and separate general script improvements into a separate change.

akutz · 2021-08-11T19:50:45Z

    cached "$DI_VIRT" && return 0
    detect_virt
-    DI_VIRT=${_RET}
+    DI_VIRT="$(echo "${_RET}" | tr '[:upper:]' '[:lower:]')"


If the interpreter is BASH then the following should be possible:

DI_VIRT="${_RET,,}"

sshedi · 2021-08-11T19:55:33Z

Thank you for doing this @sshedi. However, can you please provide a more verbose and meaningful commit message?

Okay, I will give a brief info in my commit message. If you have anything in mind, feel free to share :) I struggle a bit while writing commit messages and assigning names to variables.

akutz · 2021-08-11T20:03:43Z

Thank you for doing this @sshedi. However, can you please provide a more verbose and meaningful commit message?

Okay, I will give a brief info in my commit message. If you have anything in mind, feel free to share :) I struggle a bit while writing commit messages and assigning names to variables.

Sure. Well, on the whole of it:

The cleanup to be BASH specific shouldn't be part of this PR IMO. That should be an entirely separate PR.
The only change I see that should likely occur is with respect to the way the guest info data is checked and return codes. So I'd redact all other changes except that and make the commit message:

Update dscheck_VMware's rpctool check

This patch updates the dscheck_VMware function's use of "vmware-rpctool"
when checking to see if a "guestinfo" property is set. Because a successful
exit code can occur even if there is an empty string returned, it is possible
that the VMware datasource will be loaded as a false-positive. This patch
ensures that in addition to validating the exit code, the emitted output is
also examined to ensure a non-empty value is returned by rpctool before
returning "${DS_FOUND}" from "dscheck_VMware()".

This patch updates the dscheck_VMware function's use of "vmware-rpctool". When checking to see if a "guestinfo" property is set. Because a successful exit code can occur even if there is an empty string returned, it is possible that the VMware datasource will be loaded as a false-positive. This patch ensures that in addition to validating the exit code, the emitted output is also examined to ensure a non-empty value is returned by rpctool before returning "${DS_FOUND}" from "dscheck_VMware()". Other minor tweaks in ds-identify script. Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>

akutz · 2021-08-11T21:45:43Z

This looks good to me, thank you @sshedi! Consider this approved from my PoV.

smoser

I do realize that I'm not really active in cloud-init before, but I saw this go by and just want to mention that I really think this kind of software development makes things worse, not better.

smoser · 2021-08-13T16:00:13Z

    cached "$DI_VIRT" && return 0
    detect_virt
-    DI_VIRT=${_RET}
+    DI_VIRT="$(echo "${_RET}" | tr '[:upper:]' '[:lower:]')"


this is really bad. We shouldn't do things "just in case". That doesn't actually make things better, but makes them worse for a couple reasons:

systemd documents the outputs of 'systemd-detect-virt', and they are all lower case. Accepting upper case is just a waste.

it is dramatically slower than it was before (2 additional forks - admittedly forks are fast on linux, but the are much slower than string comparisons).

The general policy of "just in case" is silly. Should we also accept misspellings? in case something spells it 'vmwhere' ?

Hi @smoser,

I concur. I thought this was removed prior to merge (see #970 (comment) about reducing this PR to just the bits in dscheck_VMware). To be clear, this is not for misspelling, but your point is well received. Even if not systemd (FreeBSD), the mapping table ensures lower case as well.

I propose we revert this line to the former value.

Hi @smoser,

Please see #978.

Point taken. Thanks.

sshedi commented Aug 11, 2021

View reviewed changes

akutz suggested changes Aug 11, 2021

View reviewed changes

Comment thread tools/ds-identify Outdated

Comment thread tools/ds-identify Outdated

Comment thread tools/ds-identify Outdated

akutz reviewed Aug 11, 2021

View reviewed changes