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

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

[Maximiliano Sandoval]

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)) {
+	$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) {
+	$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();
+    }
 
     return $hwaddress;
 }
-- 
2.39.2

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

[Thomas Lamprecht]

Am 31/08/2023 um 15:33 schrieb Maximiliano Sandoval:
> 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';

meh, why is that a global module variable now?

I'd rather keep definition and usage together..

> +
>  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)) {

style nit: please drop the extra space between ! and defined

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


can we do this such that we still early return if OK, i.e., if HW address
is set and cache still valid, and otherwise update both cache value and
validity metadata unconditionally afterwards?

Would save a bit of code and also the newly added get_server_id method,
which is also a bit confusing, as it competes with get_hwaddress, so
should be either private, or (slightly better) just return the raw host
key, or IMO even better get dropped (see below).

Also, use hash slices for setting the cache validity keys can shorten
things, i.e., in summary something like:

my $st = stat($host_rsa_key_path);

if (
    defined($hwaddress)
    && $hwaddress_st->{ino} == $st->ino
    && $hwaddress_st->{mtime} == $st->mtime
    && $hwaddress_st->{dev} == $st->dev
) {
    return $hwaddress;
}

# else update cache
my $sshkey = PVE::Tools::file_get_contents('/etc/ssh/ssh_host_rsa_key.pub'); 
$hwaddress = uc(Digest::MD5::md5_hex($sshkey));
$hwaddress_st->@{'mtime', 'ino', 'dev'} = ($st->mtime, $st->ino, $st->dev);

return $hwaddress;


Also, is this possibly worth a log? as this happening is something odd for
most setups (at least, after intial provisioning).

> +    }
> +
> +    if ($hwaddress_st->{mtime} != $st->mtime
> +	|| $hwaddress_st->{ino} != $st->ino
> +	|| $hwaddress_st->{dev} != $st->dev) {
> +	$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();
> +    }
>  
>      return $hwaddress;
>  }

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

[Fabian Grünbichler]

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
> 
> 
>