From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 665336DCBC
for <pbs-devel@lists.proxmox.com>; Mon, 28 Mar 2022 08:30:38 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 5467C2298F
for <pbs-devel@lists.proxmox.com>; Mon, 28 Mar 2022 08:30:08 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id 2140A22982
for <pbs-devel@lists.proxmox.com>; Mon, 28 Mar 2022 08:30:07 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id ECA2D425F0
for <pbs-devel@lists.proxmox.com>; Mon, 28 Mar 2022 08:30:06 +0200 (CEST)
Message-ID: <013ab17d-2ebf-aaca-0ce5-9eeabccba9c4@proxmox.com>
Date: Mon, 28 Mar 2022 08:30:04 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:99.0) Gecko/20100101
Thunderbird/99.0
Content-Language: en-US
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>, Dominik Csapak <d.csapak@proxmox.com>
References: <20220325123816.3865356-1-d.csapak@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
In-Reply-To: <20220325123816.3865356-1-d.csapak@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.056 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
NICE_REPLY_A -0.001 Looks like a legit reply (A)
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
T_SCC_BODY_TEXT_LINE -0.01 -
Subject: Re: [pbs-devel] [PATCH proxmox-backup] rest-server:
cleanup_old_tasks: improve error handling
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 28 Mar 2022 06:30:38 -0000
On 25.03.22 13:38, Dominik Csapak wrote:
> by not bubbling up most errors, and continuing on. this avoids that we
> stop cleaning up because e.g. one directory was missing.
continuing is OK but if an those errors are encountered I'd still return an Err(),
and not an Ok(), at the end to make any caller easily distinguish the full OK or
error case.
If we expect such errors to happen there's something wrong with the general approach,
then the known false positives should be filtered.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> proxmox-rest-server/src/worker_task.rs | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
> index 643e1872..619b8e9d 100644
> --- a/proxmox-rest-server/src/worker_task.rs
> +++ b/proxmox-rest-server/src/worker_task.rs
> @@ -270,18 +270,33 @@ pub fn cleanup_old_tasks(compressed: bool) -> Result<(), Error> {
> for i in 0..256 {
> let mut path = setup.taskdir.clone();
> path.push(format!("{:02X}", i));
> - for file in std::fs::read_dir(path)? {
> - let file = file?;
> + let files = match std::fs::read_dir(path) {
> + Ok(files) => files,
> + Err(_) => continue, // does not exist or no permissions
hmm, silently ignoring ENOENT is one thing, but I'd not do so for other errors, at least
print a note/warning?
> + };
> + for file in files {
> + let file = match file {
> + Ok(file) => file,
> + Err(err) => {
> + log::error!("task cleanup: could not check some file in {}: {}", i, err);
> + continue;
> + }
> + };
> let path = file.path();
>
> - let modified = get_modified(file)
> - .map_err(|err| format_err!("error getting mtime for {:?}: {}", path, err))?;
> + let modified = match get_modified(file) {
> + Ok(modified) => modified,
> + Err(err) => {
> + log::error!("task cleanup: error getting mtime for {:?}: {}", path, err);
> + continue;
> + }
> + };
>
> if modified < cutoff_time {
> match std::fs::remove_file(path) {
> Ok(()) => {},
> Err(err) if err.kind() == std::io::ErrorKind::NotFound => {},
> - Err(err) => bail!("could not remove file: {}", err),
> + Err(err) => log::error!("task cleanup: could not remove file: {}", err),
> }
> }
> }