Re: [pmg-devel] [PATCH pmg-api] utils: check if file changed before reusing its hash

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On August 31, 2023 3:33 pm, Maximiliano Sandoval wrote:
> We cache the hash of this file, it makes sense to first check if the
> file changed via `stat` and recompute the hash if needed.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  src/PMG/Utils.pm | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index c19b31f..f8e6b7c 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -49,6 +49,8 @@ postgres_admin_cmd
>  try_decode_utf8
>  );
>  
> +my $host_rsa_key_path = '/etc/ssh/ssh_host_rsa_key.pub';
> +
>  my $valid_pmg_realms = ['pam', 'pmg', 'quarantine'];
>  
>  PVE::JSONSchema::register_standard_option('realm', {
> @@ -1353,14 +1355,32 @@ sub scan_journal_for_rbl_rejects {
>  }
>  
>  my $hwaddress;
> +my $hwaddress_st = {};
> +
> +sub get_server_id {
> +    my $sshkey = PVE::Tools::file_get_contents($host_rsa_key_path);
> +    return uc(Digest::MD5::md5_hex($sshkey));
> +}
>  
>  sub get_hwaddress {
> +    my $st = stat($host_rsa_key_path);
>  
> -    return $hwaddress if defined ($hwaddress);
> +    if (! defined($hwaddress)) {

FWIW, this condition

> +	$hwaddress_st->{mtime} = $st->mtime;
> +	$hwaddress_st->{ino} = $st->ino;
> +	$hwaddress_st->{dev} = $st->dev;
> +	$hwaddress = get_server_id();
> +    }
> +
> +    if ($hwaddress_st->{mtime} != $st->mtime
> +	|| $hwaddress_st->{ino} != $st->ino
> +	|| $hwaddress_st->{dev} != $st->dev) {

and this one can be combined, since the executed code is the same, and
as long as the check for $hwaddress comes first, the condition will
short-circuit on the first execution (filling both variables), and
subsequent executions will compare the stat metadata.

> +	$hwaddress_st->{mtime} = $st->mtime;
> +	$hwaddress_st->{ino} = $st->ino;
> +	$hwaddress_st->{dev} = $st->dev;
>  
> -    my $fn = '/etc/ssh/ssh_host_rsa_key.pub';
> -    my $sshkey = PVE::Tools::file_get_contents($fn);
> -    $hwaddress = uc(Digest::MD5::md5_hex($sshkey));
> +	$hwaddress = get_server_id();

this change would then not be needed anymore ;)

I am not sure how often we have this pattern, and whether it's worth to
have a generic "read_cached_file" helper? e.g., like this:

my $cached = {};

sub something {
  ..
  my $raw = read_cached_file($path, $cached);
  ..
}

where both (the original copy of?) $raw and the stat metadata are stored
in $cached, with the user not needing to know about the implementation
details?

just food for thought, most such things go through pmxcfs (which has its
own caching) and INotify (same) anyway..

> +    }
>  
>      return $hwaddress;
>  }
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> 
> 
>