From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <f.ebner@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 2969F70291
 for <pve-devel@lists.proxmox.com>; Mon, 13 Jun 2022 10:34:26 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 1D4CB854C
 for <pve-devel@lists.proxmox.com>; Mon, 13 Jun 2022 10:33:56 +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))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id 8C4C08541
 for <pve-devel@lists.proxmox.com>; Mon, 13 Jun 2022 10:33:55 +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 53FE142357
 for <pve-devel@lists.proxmox.com>; Mon, 13 Jun 2022 10:33:55 +0200 (CEST)
Message-ID: <a170eb53-2592-7f5f-144a-e33ece087017@proxmox.com>
Date: Mon, 13 Jun 2022 10:33:50 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Content-Language: en-US
To: pve-devel@lists.proxmox.com, d.tschlatscher@proxmox.com
References: <20220520132859.144041-1-d.tschlatscher@proxmox.com>
From: Fabian Ebner <f.ebner@proxmox.com>
In-Reply-To: <20220520132859.144041-1-d.tschlatscher@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.920 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           -1.732 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: [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes
 file when a backup is deleted
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 13 Jun 2022 08:34:26 -0000

Am 20.05.22 um 15:28 schrieb Daniel Tschlatscher:
> When a VM or Container backup was deleted, the .notes file was not
> removed, therefore, over time the dump folder would get polluted with
> notes for backups that no longer existed. As backup names contain a
> timestamp and as the notes cannot be reused because of this, I think
> it is safe to just delete them just like we do with the .log file.
> 
> Furthermore, I sourced the deletion of the log and notes file into a
> new function called "archive_auxiliaries_remove". Additionally, the
> archive_info object now returns one more field containing the name of
> the notes file. The test cases have to be adapted to expect this new
> value as the package will not compile otherwise.
> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>

Only found small nits, so consider the series:
Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>

> @@ -1585,16 +1586,25 @@ sub archive_remove {
>      die "cannot remove protected archive '$archive_path'\n"
>  	if -e protection_file_path($archive_path);
>  
> +    unlink $archive_path or die "removing archive $archive_path failed: $!\n";
> +
> +    archive_auxiliaries_remove($archive_path);
> +}
> +
> +sub archive_auxiliaries_remove {
> +    my ($archive_path) = @_;
> +
>      my $dirname = dirname($archive_path);
>      my $archive_info = eval { archive_info($archive_path) } // {};
>      my $logfn = $archive_info->{logfilename};
> +    my $notesfn = $archive_info->{notesfilename};

Both $logfn and $notesfn are unused.

>  
> -    unlink $archive_path or die "removing archive $archive_path failed: $!\n";
> +    for my $type (qw(log notes)) {
> +	my $filename = $archive_info->{"${type}filename"} or next;
> +	my $path = "$dirname/$filename";
>  
> -    if (defined($logfn)) {
> -	my $logpath = "$dirname/$logfn";
> -	if (-e $logpath) {
> -	    unlink $logpath or warn "removing log file $logpath failed: $!\n";
> +	if (-e $path) {
> +	    unlink $path or warn "Removing $type file failed: $!\n";
>  	}
>      }
>  }