* [pmg-devel] [PATCH pmg-api] sa-custom: fix moving config across mountpoints
@ 2024-10-02 17:22 Stoiko Ivanov
2024-10-03 15:27 ` Thomas Lamprecht
2024-10-04 8:41 ` Dominik Csapak
0 siblings, 2 replies; 3+ messages in thread
From: Stoiko Ivanov @ 2024-10-02 17:22 UTC (permalink / raw)
To: pmg-devel
the custom scores are kept in /var/cache/pmg-scores.cf, until they are
applied, by moving them to /etc/mail/spamassassin/pmg-scores.cf.
This currently happens with perl's `rename` function [0], which is a
wrapper for rename(2). `rename` does not work across filesystem
boundaries (errors out with EXDEV).
having `/var` on another filesystem is becoming less common, but
should probably be supported.
following the recommendation of [0] switch over to `File::Copy::move`,
and add some error-checking. File::Copy is shipped with
perl-modules-5.36, thus it does not introduce a new dependency
reported in our community-forum:
https://forum.proxmox.com/threads/.154814/
tested in a container with a seperate volume for `/var/cache`.
other occurences of `rename` in the repo should not be affected
(moving files within the same directory, or e.g. from /etc/pmg/master
to /etc/pmg).
[0] https://perldoc.perl.org/functions/rename
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
src/PMG/SACustom.pm | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/PMG/SACustom.pm b/src/PMG/SACustom.pm
index f91ebf2..8b78aff 100644
--- a/src/PMG/SACustom.pm
+++ b/src/PMG/SACustom.pm
@@ -5,6 +5,7 @@ use warnings;
use PVE::INotify;
use Digest::SHA;
+use File::Copy qw(move);
my $shadow_path = "/var/cache/pmg-scores.cf";
my $conf_path = "/etc/mail/spamassassin/pmg-scores.cf";
@@ -14,7 +15,9 @@ sub get_shadow_path {
}
sub apply_changes {
- rename($shadow_path, $conf_path) if -f $shadow_path;
+ if (-f $shadow_path) {
+ move($shadow_path, $conf_path) || die 'Moving custom scores configuration failed!\n';
+ }
}
sub calc_digest {
--
2.39.5
_______________________________________________
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
* Re: [pmg-devel] [PATCH pmg-api] sa-custom: fix moving config across mountpoints
2024-10-02 17:22 [pmg-devel] [PATCH pmg-api] sa-custom: fix moving config across mountpoints Stoiko Ivanov
@ 2024-10-03 15:27 ` Thomas Lamprecht
2024-10-04 8:41 ` Dominik Csapak
1 sibling, 0 replies; 3+ messages in thread
From: Thomas Lamprecht @ 2024-10-03 15:27 UTC (permalink / raw)
To: Stoiko Ivanov, pmg-devel
On 02/10/2024 19:22, Stoiko Ivanov wrote:
> the custom scores are kept in /var/cache/pmg-scores.cf, until they are
> applied, by moving them to /etc/mail/spamassassin/pmg-scores.cf.
Is there a good reason to not keep them in "/etc/mail/spamassassin/"
instead? That would ensure a simple rename can work and be IMO a bit
more in line with what we do else where (like host network changes),
and using "/var/cache" here feels also a bit odd in general, but that
doesn't have to be the deciding factor.
I'm fine with switching to move, but IMO adding a short info for why
not changing the pending changes config file path to the same directory
might be warranted.
> @@ -14,7 +15,9 @@ sub get_shadow_path {
> }
>
> sub apply_changes {
> - rename($shadow_path, $conf_path) if -f $shadow_path;
> + if (-f $shadow_path) {
> + move($shadow_path, $conf_path) || die 'Moving custom scores configuration failed!\n';
is there no error variable?
With rename we could at least do
if (-f $shadow_path) {
rename($shadow_path, $conf_path) or die "failed to apply custom scores - $!\n";
}
(which would have made exposing that bug much easier in the first place ^^)
> + }
> }
>
> sub calc_digest {
_______________________________________________
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
* Re: [pmg-devel] [PATCH pmg-api] sa-custom: fix moving config across mountpoints
2024-10-02 17:22 [pmg-devel] [PATCH pmg-api] sa-custom: fix moving config across mountpoints Stoiko Ivanov
2024-10-03 15:27 ` Thomas Lamprecht
@ 2024-10-04 8:41 ` Dominik Csapak
1 sibling, 0 replies; 3+ messages in thread
From: Dominik Csapak @ 2024-10-04 8:41 UTC (permalink / raw)
To: Stoiko Ivanov, pmg-devel; +Cc: Thomas Lamprecht
On 10/2/24 19:22, Stoiko Ivanov wrote:
> the custom scores are kept in /var/cache/pmg-scores.cf, until they are
> applied, by moving them to /etc/mail/spamassassin/pmg-scores.cf.
> This currently happens with perl's `rename` function [0], which is a
> wrapper for rename(2). `rename` does not work across filesystem
> boundaries (errors out with EXDEV).
>
> having `/var` on another filesystem is becoming less common, but
> should probably be supported.
>
> following the recommendation of [0] switch over to `File::Copy::move`,
> and add some error-checking. File::Copy is shipped with
> perl-modules-5.36, thus it does not introduce a new dependency
well perldoc also says this:
> If an error occurs during this copy-and-delete process, you may be left with a (possibly partial)
> copy of the file under the destination name.
so i'd probably use something like get_contents/set_contents so we can be sure the write
is atomic
otherwise, i agree with thomas that we probably should keep the original in
/etc/mail/spamassassin if it's possible to have a file there that is not parsed
by spamassassin (not sure what logic it uses to read files from there)
>
> reported in our community-forum:
> https://forum.proxmox.com/threads/.154814/
>
> tested in a container with a seperate volume for `/var/cache`.
>
> other occurences of `rename` in the repo should not be affected
> (moving files within the same directory, or e.g. from /etc/pmg/master
> to /etc/pmg).
>
> [0] https://perldoc.perl.org/functions/rename
>
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
> src/PMG/SACustom.pm | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/PMG/SACustom.pm b/src/PMG/SACustom.pm
> index f91ebf2..8b78aff 100644
> --- a/src/PMG/SACustom.pm
> +++ b/src/PMG/SACustom.pm
> @@ -5,6 +5,7 @@ use warnings;
>
> use PVE::INotify;
> use Digest::SHA;
> +use File::Copy qw(move);
>
> my $shadow_path = "/var/cache/pmg-scores.cf";
> my $conf_path = "/etc/mail/spamassassin/pmg-scores.cf";
> @@ -14,7 +15,9 @@ sub get_shadow_path {
> }
>
> sub apply_changes {
> - rename($shadow_path, $conf_path) if -f $shadow_path;
> + if (-f $shadow_path) {
> + move($shadow_path, $conf_path) || die 'Moving custom scores configuration failed!\n';
> + }
> }
>
> sub calc_digest {
_______________________________________________
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:[~2024-10-04 8:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-02 17:22 [pmg-devel] [PATCH pmg-api] sa-custom: fix moving config across mountpoints Stoiko Ivanov
2024-10-03 15:27 ` Thomas Lamprecht
2024-10-04 8:41 ` Dominik Csapak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal