public inbox for pmg-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pmg-devel] [PATCH pmg-api] utils: check if file changed before reusing its hash
@ 2023-08-31 13:33 Maximiliano Sandoval
  2023-08-31 14:00 ` Thomas Lamprecht
  2023-08-31 14:17 ` Fabian Grünbichler
  0 siblings, 2 replies; 3+ messages in thread
From: Maximiliano Sandoval @ 2023-08-31 13:33 UTC (permalink / raw)
  To: pmg-devel

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





^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pmg-devel] [PATCH pmg-api] utils: check if file changed before reusing its hash
  2023-08-31 13:33 [pmg-devel] [PATCH pmg-api] utils: check if file changed before reusing its hash Maximiliano Sandoval
@ 2023-08-31 14:00 ` Thomas Lamprecht
  2023-08-31 14:17 ` Fabian Grünbichler
  1 sibling, 0 replies; 3+ messages in thread
From: Thomas Lamprecht @ 2023-08-31 14:00 UTC (permalink / raw)
  To: Maximiliano Sandoval, pmg-devel

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





^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pmg-devel] [PATCH pmg-api] utils: check if file changed before reusing its hash
  2023-08-31 13:33 [pmg-devel] [PATCH pmg-api] utils: check if file changed before reusing its hash Maximiliano Sandoval
  2023-08-31 14:00 ` Thomas Lamprecht
@ 2023-08-31 14:17 ` Fabian Grünbichler
  1 sibling, 0 replies; 3+ messages in thread
From: Fabian Grünbichler @ 2023-08-31 14:17 UTC (permalink / raw)
  To: Maximiliano Sandoval, pmg-devel

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




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-08-31 14:18 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-31 13:33 [pmg-devel] [PATCH pmg-api] utils: check if file changed before reusing its hash Maximiliano Sandoval
2023-08-31 14:00 ` Thomas Lamprecht
2023-08-31 14:17 ` Fabian Grünbichler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal