From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 8638C1FF0C2 for ; Thu, 22 Oct 2026 15:07:20 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 710BA21612; Thu, 22 Oct 2026 15:07:18 +0200 (CEST) Date: Thu, 22 Oct 2026 15:07:11 +0200 From: Wolfgang Bumiller To: Shannon Sterz Subject: Re: [PATCH cluster 03/21] pmxcfs: add live backup capability Message-ID: References: <20260828133030.351140-1-s.sterz@proxmox.com> <20260828133030.351140-4-s.sterz@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260828133030.351140-4-s.sterz@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792674431170 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.413 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 52MIAJUOFFJZMACPHBHCYMGAK33FE2MP X-Message-ID-Hash: 52MIAJUOFFJZMACPHBHCYMGAK33FE2MP X-MailFrom: w.bumiller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri, Aug 28, 2026 at 03:30:12PM +0200, Shannon Sterz wrote: > Signed-off-by: Shannon Sterz > --- > src/PVE/Cluster.pm | 49 ++++++++++++++- > src/pmxcfs/cfs-ipc-ops.h | 2 + > src/pmxcfs/cfs-utils.h | 1 + > src/pmxcfs/database.c | 128 +++++++++++++++++++++++++++++++++++++++ > src/pmxcfs/memdb.c | 30 +++++++++ > src/pmxcfs/memdb.h | 94 ++++++++++++++++++++++++++++ > src/pmxcfs/server.c | 94 ++++++++++++++++++++++++++++ > 7 files changed, 395 insertions(+), 3 deletions(-) > > diff --git a/src/PVE/Cluster.pm b/src/PVE/Cluster.pm > index 034b78c..31c06e8 100644 > --- a/src/PVE/Cluster.pm > +++ b/src/PVE/Cluster.pm > @@ -42,6 +42,9 @@ my $lockdir = "/etc/pve/priv/lock"; > my $dbfile = "/var/lib/pve-cluster/config.db"; > my $dbbackupdir = "/var/lib/pve-cluster/backup"; > > +# maximum number of files to keep for each db backup type (live, cluster join) > +my $db_backup_maxfiles = 10; > + > # this is just a readonly copy, the relevant one is in status.c from pmxcfs > # observed files are the one we can get directly through IPCC, they are cached > # using a computed version and only those can be used by the cfs_*_file methods > @@ -236,6 +239,14 @@ my $ipcc_verify_token = sub { > die "$!\n"; > }; > > +my $ipcc_init_backup = sub { > + my ($filename) = @_; > + > + my $bindata = pack "Z*", $filename; > + PVE::IPCC::ipcc_send_rec(CFS_IPC_INIT_BACKUP, $bindata); > + warn $! if $! != 0; > +}; > + > my $ccache = {}; > > sub cfs_update { > @@ -906,11 +917,10 @@ sub cfs_backup_database { > my $cmd = [['sqlite3', $dbfile, '.dump'], ['gzip', '-', \ ">${backup_fn}"]]; > run_command($cmd, 'errmsg' => "cannot backup old database\n"); > > - my $maxfiles = 10; # purge older backup > my $backups = [sort { $b cmp $a } <$dbbackupdir/config-*.sql.gz>]; > > - if ((my $count = scalar(@$backups)) > $maxfiles) { > - foreach my $f (@$backups[$maxfiles .. $count - 1]) { > + if ((my $count = scalar(@$backups)) > $db_backup_maxfiles) { > + foreach my $f (@$backups[$db_backup_maxfiles .. $count - 1]) { > next if $f !~ m/^(\S+)$/; # untaint > print "delete old backup '$1'\n"; > unlink $1; > @@ -929,4 +939,37 @@ sub cfs_rename_db_unsafe { > rename $dbfile, "$dbfile.$ctime.bak" or warn "failed to rename old config database - $!\n"; > } > > +=head3 cfs_live_backup_database() > + > +Creates a new live backup of the database backing the cluster file system. > + > +Returns a file path to the created backup. > + > +=cut > + > +sub cfs_live_backup_database { > + mkdir $dbbackupdir or $!{EEXIST} or die "failed to create backup dir - $!\n"; ^ While we do need to keep the `chmod` call below (due to the old code existing), we should just pass `0700` to the `mkdir` invocation directly as well (and also add it in `cfs_backup_database()`), to not create unnecessary races. > + chmod 0700, $dbbackupdir or die "failed to change mode for backup dir - $!\n"; > + >>From here… > + my $backups = [sort { $b cmp $a } glob("$dbbackupdir/config-backup-*.db")]; > + > + if ((my $count = scalar(@$backups)) > $db_backup_maxfiles) { > + foreach my $f (@$backups[$db_backup_maxfiles .. $count - 1]) { > + next if $f !~ m/^(\S+)$/; # untaint > + print "cleaning up old backup '$1'\n"; > + unlink $1; > + } > + } …to here, this exact code already exists in the sub above it - factor it out. Although there is a small difference: the sub above it cleans up *after* creating the new file. If we only keep 1 file, we now have a time period where the last backup was deleted and the new one doesn't exist yet. Perhaps this one should pass `$db_backup_maxfiles + 1` to the to-be-added cleanup helper. > + > + my $ctime = time(); > + my $backup_file = "config-backup-$ctime.db"; > + print "starting backup of current database to \"$dbbackupdir/$backup_file\"\n"; > + > + eval { &$ipcc_init_backup($backup_file); }; > + > + warn $@ if $@; > + > + return $backup_file; > +} > + > 1; > diff --git a/src/pmxcfs/cfs-ipc-ops.h b/src/pmxcfs/cfs-ipc-ops.h > index 249308d..054ebb3 100644 > --- a/src/pmxcfs/cfs-ipc-ops.h > +++ b/src/pmxcfs/cfs-ipc-ops.h > @@ -45,4 +45,6 @@ > > #define CFS_IPC_GET_GUEST_CONFIG_PROPERTIES 13 > > +#define CFS_IPC_INIT_BACKUP 14 > + > #endif > diff --git a/src/pmxcfs/cfs-utils.h b/src/pmxcfs/cfs-utils.h > index ed67264..19e3aab 100644 > --- a/src/pmxcfs/cfs-utils.h > +++ b/src/pmxcfs/cfs-utils.h > @@ -32,6 +32,7 @@ > #define HOST_CLUSTER_CONF_FN "/etc/corosync/corosync.conf" > #define CFS_PID_FN "/var/run/pve-cluster.pid" > #define VARLIBDIR "/var/lib/pve-cluster" > +#define BACKUPDIR VARLIBDIR "/backup" > #define RUNDIR "/run/pve-cluster" > > #define CFS_MAX(a, b) (((a) > (b)) ? (a) : (b)) > diff --git a/src/pmxcfs/database.c b/src/pmxcfs/database.c > index 4ee2389..07b3664 100644 > --- a/src/pmxcfs/database.c > +++ b/src/pmxcfs/database.c > @@ -49,6 +49,8 @@ struct db_backend { > sqlite3_stmt *stmt_commit; > sqlite3_stmt *stmt_rollback; > sqlite3_stmt *stmt_load_all; > + sqlite3_backup *current_backup; > + sqlite3 *backup_db; > }; > > #define VERSIONFILENAME "__version__" > @@ -650,6 +652,10 @@ void bdb_backend_close(db_backend_t *bdb) { > sqlite3_finalize(bdb->stmt_rollback); > sqlite3_finalize(bdb->stmt_load_all); > > + // calling bdb_finish_or_abort_backup() has no effect if no backup is happening, so calling it > + // unconditionally here is ok > + (void)bdb_finish_or_abort_backup(bdb, TRUE); > + > int rc; > if ((rc = sqlite3_close(bdb->db)) != SQLITE_OK) { > cfs_critical("sqlite3_close failed: %d\n", rc); > @@ -738,3 +744,125 @@ fail: > > return NULL; > } > + > +int bdb_init_backup(db_backend_t *bdb, const char *backup_filename) { > + g_return_val_if_fail(bdb != NULL, BACKUP_RETURN_EPAR); > + g_return_val_if_fail(bdb->db != NULL, BACKUP_RETURN_EPAR); > + g_return_val_if_fail(backup_filename != NULL, BACKUP_RETURN_EPAR); > + > + if (bdb->current_backup != NULL || bdb->backup_db != NULL) { > + cfs_debug("trying to start a new backup while one is already running, aborting..."); > + return BACKUP_RETURN_EGEN; > + } > + > + int rc; > + g_autofree gchar *canonical_filename = g_canonicalize_filename(backup_filename, BACKUPDIR); > + cfs_message("starting live backup '%s'...", canonical_filename); > + > + int flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE; > + > + if ((rc = sqlite3_open_v2(canonical_filename, &bdb->backup_db, flags, NULL)) != SQLITE_OK) { > + cfs_critical("sqlite error while opening backup file (%d): %s", rc, sqlite3_errstr(rc)); > + (void)sqlite3_close_v2(bdb->backup_db); > + bdb->backup_db = NULL; > + return BACKUP_RETURN_EGEN; > + } > + > + if (chmod(canonical_filename, 0600) == -1) { (*sigh*, why don't they take a mode in the sqlite3 api - we don't want to change the umask in threaded code -_-. Then again, the directory is already meant to be 0700 anyway, so 🤷) > + cfs_critical("chmod failed for backup db: %s", strerror(errno)); > + (void)sqlite3_close_v2(bdb->backup_db); > + bdb->backup_db = NULL; > + return BACKUP_RETURN_EGEN; > + } > + > + bdb->current_backup = sqlite3_backup_init(bdb->backup_db, "main", bdb->db, "main"); > + > + if (bdb->current_backup == NULL) { > + rc = sqlite3_errcode(bdb->backup_db); > + cfs_critical("sqlite error while initializing backup (%d): %s", rc, sqlite3_errstr(rc)); > + (void)sqlite3_close_v2(bdb->backup_db); > + bdb->backup_db = NULL; > + return BACKUP_RETURN_EGEN; > + } > + > + return BACKUP_RETURN_CONT; > +} > + > +int bdb_finish_or_abort_backup(db_backend_t *bdb, gboolean abort) { > + g_return_val_if_fail(bdb != NULL, BACKUP_RETURN_EPAR); > + > + int rc; > + int to_return_code = BACKUP_RETURN_DONE; > + GString *db_filename = NULL; Use g_autoptr(GString) db_filename = NULL; and drop the `g_string_free` call at the end. > + > + if (bdb->backup_db != NULL) { Does it not make sense to just if (!bdb->backup_db) return; ? Can `current_backup` be set without `backup_db`? > + sqlite3_filename fn; > + if ((fn = sqlite3_db_filename(bdb->backup_db, "main")) != NULL) { IMO that's bad form - do the assignment in the line above and just `if (fn != NULL)` > + // the filename is managed by the sqlite3 object, so copy it into its own string to > + // preserve it across the `sqlite3_close_v2()` call. > + db_filename = g_string_new(fn); > + }; > + } > + > + if ((rc = sqlite3_backup_finish(bdb->current_backup)) != SQLITE_OK) { > + cfs_critical("sqlite error when ending backup (%d): %s", rc, sqlite3_errstr(rc)); > + to_return_code = BACKUP_RETURN_EGEN; > + } > + > + bdb->current_backup = NULL; > + > + if ((rc = sqlite3_close_v2(bdb->backup_db)) != SQLITE_OK) { > + cfs_critical("sqlite error when ending backup (%d): %s", rc, sqlite3_errstr(rc)); > + to_return_code = BACKUP_RETURN_EGEN; > + } > + > + bdb->backup_db = NULL; > + > + // if abort is true or an error was encountered, we need to clean up the database file here > + if ((abort || to_return_code != BACKUP_RETURN_DONE) && db_filename != NULL) { > + (void)unlink(db_filename->str); nit: should we warn if this fails? > + } > + > + if (db_filename != NULL) { > + g_string_free(db_filename, TRUE); > + } > + > + return to_return_code; > +} > + > +int bdb_handle_backup(db_backend_t *bdb) { > + g_return_val_if_fail(bdb != NULL, BACKUP_RETURN_EPAR); > + g_return_val_if_fail(bdb->db != NULL, BACKUP_RETURN_EPAR); > + > + if (bdb->backup_db == NULL || bdb->current_backup == NULL) { > + cfs_debug("no backup in progress, please initialize one first..."); > + return BACKUP_RETURN_EGEN; > + } > + > + // `backup_db` and `current_backup` are initialized successfully, do a back up step > + int rc = sqlite3_backup_step(bdb->current_backup, 10); How is the number 10 chosen? Does it make sense to have this tuneable? > + > + if (rc == SQLITE_OK || rc == SQLITE_BUSY || rc == SQLITE_LOCKED) { > + // backup step succeeded or encountered a non-permanent error. backup is not done. > + // -> signal that backup step is done and another one should be scheduled > + return BACKUP_RETURN_CONT; > + } > + > + int to_return_code = BACKUP_RETURN_DONE; > + > + if (rc != SQLITE_DONE) { > + cfs_critical("sqlite error during backup step (%d): %s", rc, sqlite3_errstr(rc)); > + to_return_code = BACKUP_RETURN_EGEN; > + } else { > + cfs_debug("backup completed, cleaning up..."); > + } > + > + // abort the backup if we encountered an error > + rc = bdb_finish_or_abort_backup(bdb, to_return_code != BACKUP_RETURN_DONE); > + > + if (rc != BACKUP_RETURN_DONE) { > + to_return_code = BACKUP_RETURN_EGEN; > + } > + > + return to_return_code; > +} > diff --git a/src/pmxcfs/memdb.c b/src/pmxcfs/memdb.c > index 77d7652..b7372f9 100644 > --- a/src/pmxcfs/memdb.c > +++ b/src/pmxcfs/memdb.c > @@ -1520,3 +1520,33 @@ memdb_index_t *memdb_encode_index(GHashTable *index, memdb_tree_entry_t *root) { > > return idx; > } > + > +int memdb_init_backup(memdb_t *memdb, const char *backup_filename) { > + g_return_val_if_fail(memdb != NULL, BACKUP_RETURN_EPAR); > + > + g_mutex_lock(&memdb->mutex); > + int to_return = bdb_init_backup(memdb->bdb, backup_filename); > + g_mutex_unlock(&memdb->mutex); > + > + return to_return; > +} > + > +int memdb_finish_or_abort_backup(memdb_t *memdb, gboolean abort) { > + g_return_val_if_fail(memdb != NULL, BACKUP_RETURN_EPAR); > + > + g_mutex_lock(&memdb->mutex); > + int to_return = bdb_finish_or_abort_backup(memdb->bdb, abort); > + g_mutex_unlock(&memdb->mutex); > + > + return to_return; > +} > + > +int memdb_handle_backup(memdb_t *memdb) { > + g_return_val_if_fail(memdb != NULL, BACKUP_RETURN_EPAR); > + > + g_mutex_lock(&memdb->mutex); > + int to_return = bdb_handle_backup(memdb->bdb); > + g_mutex_unlock(&memdb->mutex); > + > + return to_return; > +} > diff --git a/src/pmxcfs/memdb.h b/src/pmxcfs/memdb.h > index bca8967..2c27959 100644 > --- a/src/pmxcfs/memdb.h > +++ b/src/pmxcfs/memdb.h > @@ -34,6 +34,11 @@ > #define MEMDB_BLOCKSIZE 4096 > #define MEMDB_BLOCKS ((MEMDB_MAX_FSSIZE + MEMDB_BLOCKSIZE - 1) / MEMDB_BLOCKSIZE) > > +#define BACKUP_RETURN_CONT 0 // backup step completed, schedule next one > +#define BACKUP_RETURN_DONE 1 // backup done > +#define BACKUP_RETURN_EGEN 2 // encountered error while trying to back up > +#define BACKUP_RETURN_EPAR 3 // a parameter passed to the backup helper functions was invalid Do we have a precedent for such a numeric return code? Otherwise, if we do such a thing, IMO the error cases should be negative. I'm also not sure a "generic" vs "parameter" error is a useful distinction, except for debugging, where in-place error logging with just returning -1/EGEN would be more useful. The exact value is actually only checked for `memdb_handle_backup` where the `EPAR` case is AFAICT impossible anyway. And eg. `bdb_init_backup`/`memdb_init_backup` are just checked for `CONT`, so a boolean might be a better return type for that one. No use in having a lot of dead cases around. > + > typedef struct memdb_tree_entry memdb_tree_entry_t; > struct memdb_tree_entry { > guint64 parent; > @@ -151,6 +156,50 @@ memdb_index_t *memdb_index_copy(memdb_index_t *idx); > > gboolean memdb_tree_entry_csum(memdb_tree_entry_t *te, guchar csum[32]); > > +/** > + * memdb_init_backup: > + * @memdb: a memdb object that has an initialized db to back up. > + * @backup_filename: the name for the backup file. > + * > + * Initializes a backup of the configuration database. > + * > + * Return: an integer representing the status of the backup. Either `BACKUP_RETURN_CONT` if the > + * backup was initialized correctly or an error such as `BACKUP_RETURN_EGEN` or > + * `BACKUP_RETURN_EPAR`. > + */ > +int memdb_init_backup(memdb_t *memdb, const char *backup_filename); > + > +/** > + * memdb_finish_or_abort_backup: > + * @memdb: a memdb object that is currently being backed up. > + * @abort: if true, this will be considered an abort operation and the backup database file will > + * be removed. > + * > + * Will finish a backup no matter whether all backup steps have completed successfully. If they > + * haven't, the backup is invalid and all write operations should be rolled back by sqlite. If an > + * error occurs while finishing the backup or if abort is set, the database file will be removed. > + * > + * Calling this function while no backup is in progress is harmless (similar to how NULL can > + * be passed to sqlite3_backup_finish()). > + * > + * Return: an integer representing the result of the operation. Either `BACKUP_RETURN_DONE` if the > + * close operation was successful or `BACKUP_RETURN_EGEN` if finishing the backup encountered an > + * error. If no backup is in progress, `BACKUP_RETURN_EPAR` will be returned. > + */ > +int memdb_finish_or_abort_backup(memdb_t *memdb, gboolean abort); > + > +/** > + * memdb_handle_backup: > + * @memdb: a memdb object that has an initialized db to back up. > + * > + * Handles a single backup step and finalizes the backup if it is done. > + * > + * Return: an integer representing the status of the backup. Either `BACKUP_RETURN_CONT` if the > + * backup was initialized correctly, `BACKUP_RETURN_DONE` if the backup is complete or an error such > + * as `BACKUP_RETURN_EGEN` or `BACKUP_RETURN_EPAR`. > + */ > +int memdb_handle_backup(memdb_t *memdb); > + > db_backend_t *bdb_backend_open(const char *filename, memdb_tree_entry_t *root, GHashTable *index); > > void bdb_backend_close(db_backend_t *bdb); > @@ -173,4 +222,49 @@ gboolean bdb_backend_commit_update( > memdb_t *memdb, memdb_index_t *master, memdb_index_t *slave, GList *inodes > ); > > +/** > + * bdb_init_backup: > + * @bdb: a database backend that has an initialized db to back up. > + * @backup_filename: the name for the backup file. > + * > + * Initializes a backup of the configuration database. > + * > + * Return: an integer representing the status of the backup. Either `BACKUP_RETURN_CONT` if the > + * backup was initialized correctly or an error such as `BACKUP_RETURN_EGEN` or > + * `BACKUP_RETURN_EPAR`. > + */ > +int bdb_init_backup(db_backend_t *bdb, const char *backup_filename); > + > +/** > + * bdb_finish_or_abort_backup: > + * @bdb: a database backend that is currently being backed up. > + * @abort: if true, this will be considered an abort operation and the backup database file will > + * be removed. > + * > + * Will finish a backup no matter whether all backup steps have completed successfully. If they > + * haven't, the backup is invalid and all write operations should be rolled back by sqlite. If an > + * error occurs while finishing the backup or if abort is set, the database file will be removed. > + * > + * Calling this function while no backup is in progress is harmless (similar to how NULL can > + * be passed to sqlite3_backup_finish()). > + > + * > + * Return: an integer representing the result of the operation. Either `BACKUP_RETURN_DONE` if the > + * close operation was successful or `BACKUP_RETURN_EGEN` if finishing the backup encountered an > + * error. If no backup is in progress, `BACKUP_RETURN_EPAR` will be returned. > + */ > +int bdb_finish_or_abort_backup(db_backend_t *bdb, gboolean abort); > + > +/** > + * bdb_handle_backup: > + * @bdb: a database backend that has an initialized db to back up. > + * > + * Handles a single backup step and finalizes the backup if it is done. > + * > + * Return: an integer representing the status of the backup. Either `BACKUP_RETURN_CONT` if the > + * backup was initialized correctly, `BACKUP_RETURN_DONE` if the backup is complete or an error such > + * as `BACKUP_RETURN_EGEN` or `BACKUP_RETURN_EPAR`. > + */ > +int bdb_handle_backup(db_backend_t *bdb); > + > #endif /* _PVE_MEMDB_H_ */ > diff --git a/src/pmxcfs/server.c b/src/pmxcfs/server.c > index 278e2c9..8149fd3 100644 > --- a/src/pmxcfs/server.c > +++ b/src/pmxcfs/server.c > @@ -48,6 +48,7 @@ static qb_loop_t *loop; > static qb_ipcs_service_t *s1; > static GString *outbuf; > static memdb_t *memdb; > +static GRegex *filename_regex; > > static int server_started = 0; /* protect with server_started_mutex */ > static int terminate_server = 0; /* protect with server_started_mutex */ > @@ -100,6 +101,11 @@ typedef struct { > char token[]; > } cfs_verify_token_request_header_t; > > +typedef struct { > + struct qb_ipc_request_header req_header; > + char filename[]; > +} cfs_init_backup_request_header_t; > + > struct s1_context { > int32_t client_pid; > uid_t uid; > @@ -107,6 +113,37 @@ struct s1_context { > gboolean read_only; > }; > > +static void backup_job(void *data) { > + cfs_debug("handling backup step"); > + > + switch (memdb_handle_backup(memdb)) { > + case BACKUP_RETURN_CONT: > + cfs_debug("backup iteration done, scheduling next one"); > + > + int rc = qb_loop_job_add(loop, QB_LOOP_LOW, NULL, backup_job); > + > + if (rc != 0) { > + // Abort the backup as we can't finish it without scheduling a job. > + cfs_critical("could not schedule next backup job (error %d), aborting backup...", rc); > + (void)memdb_finish_or_abort_backup(memdb, TRUE); > + } > + > + break; > + case BACKUP_RETURN_DONE: > + cfs_message("live backup complete."); > + break; > + case BACKUP_RETURN_EGEN: > + cfs_debug("encountered an error while backing up."); > + break; > + case BACKUP_RETURN_EPAR: > + cfs_debug("a parameter passed to one of the backup functions was invalid."); > + break; > + default: > + cfs_debug("encountered an unknown return code while backing up."); > + break; > + } > +} > + > static int32_t s1_connection_accept_fn(qb_ipcs_connection_t *c, uid_t uid, gid_t gid) { > if ((uid == 0 && gid == 0) || (gid == cfs.gid)) { > cfs_debug("authenticated connection %d/%d", uid, gid); > @@ -446,6 +483,45 @@ static int32_t s1_msg_process_fn(qb_ipcs_connection_t *c, void *data, size_t siz > result = -ENOENT; > } > } > + } else if (request_id == CFS_IPC_INIT_BACKUP) { > + cfs_init_backup_request_header_t *rh = (cfs_init_backup_request_header_t *)data; > + int filename_len = > + request_size - G_STRUCT_OFFSET(cfs_init_backup_request_header_t, filename) - 1; > + > + if (ctx->read_only) { > + result = -EPERM; > + } else if (filename_len <= 0) { > + cfs_debug("backup filename_len <= 0, %d", filename_len); > + result = -EINVAL; > + } else if (rh->filename[filename_len] != '\0') { > + cfs_debug("backup file not NULL-terminated"); > + result = -EINVAL; > + } else if (strnlen(rh->filename, filename_len) != filename_len) { > + cfs_debug("backup file contains NULL-byte"); > + result = -EINVAL; > + } else { > + if (filename_regex != NULL && > + g_regex_match(filename_regex, rh->filename, G_REGEX_MATCH_DEFAULT, NULL)) { > + > + if (memdb_init_backup(memdb, rh->filename) != BACKUP_RETURN_CONT) { > + cfs_debug("could not initialize a backup"); > + result = -EIO; > + } else { > + result = qb_loop_job_add(loop, QB_LOOP_LOW, NULL, backup_job); > + if (result != 0) { > + cfs_critical("couldn't add first backup step (error %d), abort...", result); > + (void)memdb_finish_or_abort_backup(memdb, TRUE); > + } > + } > + > + if (result != 0) { > + cfs_debug("could not start backup"); > + } > + } else { > + cfs_debug("could not check backup file name or it contains forbidden characters"); > + result = -EINVAL; > + } > + } > } > > cfs_debug("process result %d", result); > @@ -577,6 +653,18 @@ gboolean server_start(memdb_t *db) { > > outbuf = g_string_sized_new(8192 * 8); > > + // set up filename regex on start > + filename_regex = g_regex_new( > + "^[A-Za-z0-9\\-_][A-Za-z0-9\\-_\\.]*$", G_REGEX_DEFAULT, G_REGEX_MATCH_DEFAULT, NULL > + ); > + > + if (filename_regex == NULL) { > + // Shouldn't happen since we compile a static pattern. If it does, don't abort. pmxcfs > + // will not allow live backups in this case, but should function fine otherwise. This is > + // preferable over not allowing users to run pmxcfs at all in that scenario. > + cfs_message("could not compile filename regex. live backups are not supported."); > + } > + > if (!(loop = qb_loop_create())) { > cfs_critical("cant create event loop"); > return FALSE; > @@ -625,6 +713,12 @@ void server_stop(void) { > loop = NULL; > } > > + // tear down regex on server stop > + if (filename_regex != NULL) { > + g_regex_unref(filename_regex); > + filename_regex = NULL; > + } > + > if (outbuf) { > g_string_free(outbuf, TRUE); > outbuf = NULL; > -- > 2.47.3 > > > > > --