On 3/12/20 9:05 AM, Moayad Almalat wrote:
> Signed-off-by: Moayad Almalat <m.alma...@proxmox.com>
goes in the right direction, some style nits and comments below.
> ---
> PVE/VZDump.pm | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index ada3681e..df6f8df9 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -569,6 +569,10 @@ sub run_hook_script {
>
> my $script = $opts->{script};
>
> + unless (-X $script) {
1. We do not use "unless" in our code base, while it's valid perl we actively
choose against that, see our perl style guide:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices
2. Also, check out the difference between -X (upper case) and -x (lower case):
-X File is executable by real uid/gid.
-x File is executable by effective uid/gid.
The real user id is who you really are (the one who owns the process), and
the
effective user id is what the operating system looks at to make a decision
whether or not you are allowed to do something (most of the time, there are
some
exceptions).
Please use lower case "-x".
3. This needs to be moved below the "return if !$script;" below, as else you
test
even if no script is set, which can give you "use of uninitialized $variable"
messages, and is not good practice.
> + debugmsg('err', "$phase: The hook-script '$script' is not
> executable.");
There's no point in continuing as the run_command would die anyway - but without
clear error. So additionally please add a explicit die too
die "The hook-script '$script' is not executable.\n"
($phase is not relevant info here - it'd highly probably die on every phase)
> + }
> +
> return if !$script;
>
> my $cmd = "$script $phase";
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
