Re: [pve-devel] [PATCH qemu v5 03/32] PVE backup: implement backup access setup and teardown API for external providers

[Fiona Ebner <f.ebner@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