From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 129D21FF15E
for <inbox@lore.proxmox.com>; Tue, 25 Mar 2025 11:52:22 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 6B73B6ECF;
Tue, 25 Mar 2025 11:52:17 +0100 (CET)
Message-ID: <7a2f7241-0ec9-44fe-89dd-909a6d818852@proxmox.com>
Date: Tue, 25 Mar 2025 11:51:44 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
References: <20250321134852.103871-1-f.ebner@proxmox.com>
<20250321134852.103871-4-f.ebner@proxmox.com>
<a5jxlhqlzclwqljdg5jz7inrweovnfkihij3c27twgidlqq2dz@bxd37656fuht>
Content-Language: en-US
From: Fiona Ebner <f.ebner@proxmox.com>
In-Reply-To: <a5jxlhqlzclwqljdg5jz7inrweovnfkihij3c27twgidlqq2dz@bxd37656fuht>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.040 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH qemu v5 03/32] PVE backup: implement backup
access setup and teardown API for external providers
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>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: pve-devel@lists.proxmox.com
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
Am 24.03.25 um 14:02 schrieb Wolfgang Bumiller:
> On Fri, Mar 21, 2025 at 02:48:23PM +0100, Fiona Ebner wrote:
>> + if (setup_snapshot_access(di, &local_err) < 0) {
>> + cleanup_snapshot_access(di);
>
> (not part of this patch, but why does our `setup_snapshot_access()` not
> call `cleanup_snapshot_access()` on failure?)
No particular reason, and I'll change it in v6. It doesn't even need to
call the function on failure, it just needs to drop the cbw filter in
the single branch where it matters.
>> + qemu_mutex_lock(&backup_state.stat.lock);
>> + backup_state.stat.reused = 0;
>> +
>> + /* clear previous backup's bitmap_list */
>> + if (backup_state.stat.bitmap_list) {
>> + GList *bl = backup_state.stat.bitmap_list;
>> + while (bl) {
>> + g_free(((PBSBitmapInfo *)bl->data)->drive);
>> + g_free(bl->data);
>> + bl = g_list_next(bl);
>> + }
>> + g_list_free(backup_state.stat.bitmap_list);
>> + backup_state.stat.bitmap_list = NULL;
>> + }
>
> ^ should be factored into a function instead of copied
Will do!
> The code below can also be factored out as AFAICT it, too, is a copy, if
> `backup_file` is a parameter (which which `NULL` may be passed, as
> `g_strdup()` explicitly maps NULL to NULL), and `uuid` as well I guess
> (here it's not touched but may as well be cleared (or the function would
> ignore it when NULL is passed)).
>
>> +
>> + /* initialize global backup_state now */
>> +
>> + if (backup_state.stat.error) {
>> + error_free(backup_state.stat.error);
>> + backup_state.stat.error = NULL;
>> + }
>> +
>> + backup_state.stat.start_time = time(NULL);
>> + backup_state.stat.end_time = 0;
>> +
>> + if (backup_state.stat.backup_file) {
>> + g_free(backup_state.stat.backup_file);
>> + }
>> + backup_state.stat.backup_file = NULL;
>> +
>> + if (backup_state.target_id) {
>> + g_free(backup_state.target_id);
>> + }
>> + backup_state.target_id = g_strdup(target_id);
>
> (^ Not sure if `qmp_backup`should include the above)
The target is set there too, hardcoded to "Proxmox". If we want to
support bitmaps for multiple PBS targets (there is some user demand for
that), we'll use different values there in the future too. But it's not
part of the stat struct, so I'll add a short dedicated helper instead.
>
>> +
>> + /*
>> + * The stats will never update, because there is no internal backup job. Initialize them anyway
>> + * for completeness.
>> + */
>> + backup_state.stat.total = total;
>> + backup_state.stat.dirty = total - backup_state.stat.reused;
>> + backup_state.stat.transferred = 0;
>> + backup_state.stat.zero_bytes = 0;
>> + backup_state.stat.finishing = false;
>> + backup_state.stat.starting = false; // there's no associated QEMU job
>
> ^ up to this point - requiring stat.lock to be held I suppose, (unless
> qmp_backup allows moving the stuff in between to above the lock()
> call...)
Will do!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel