7:34 a.m.
Reported by 'philipp' on OFTC.
Peter Krempa (3):
virDomainBackupDefAssignStore: Restructure control flow
conf: backup: Fix logic for generating default backup filenames
schemas: backup: Allow missing 'type' attribute for backup disk
src/conf/backup_conf.c | 17 +++++++++++------
src/conf/schemas/domainbackup.rng | 4 ++++
tests/domainbackupxml2xmlin/backup-push.xml | 3 +++
tests/domainbackupxml2xmlout/backup-push.xml | 9 +++++++++
4 files changed, 27 insertions(+), 6 deletions(-)
--
2.40.1
7:34 a.m.
New subject: [PATCH 1/3] virDomainBackupDefAssignStore: Restructure control flow
Return early for errors instead of using 'else' branches.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/backup_conf.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index 4a8c05dca9..013c08cd6e 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -422,17 +422,19 @@ virDomainBackupDefAssignStore(virDomainBackupDiskDef *disk,
_("disk '%1$s' has no media"),
disk->name);
return -1;
}
- } else if (!disk->store) {
- if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_FILE) {
- disk->store = virStorageSourceNew();
- disk->store->type = VIR_STORAGE_TYPE_FILE;
- disk->store->path = g_strdup_printf("%s.%s", src->path,
suffix);
- } else {
+ }
+
+ if (!disk->store) {
+ if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_FILE) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("refusing to generate file name for disk
'%1$s'"),
disk->name);
return -1;
}
+
+ disk->store = virStorageSourceNew();
+ disk->store->type = VIR_STORAGE_TYPE_FILE;
+ disk->store->path = g_strdup_printf("%s.%s", src->path,
suffix);
}
return 0;
--
2.40.1
7:34 a.m.
New subject: [PATCH 2/3] conf: backup: Fix logic for generating default backup filenames
If the 'disk->store' property is already allocated which happens e.g.
when the disk is described by the backup XML but the optional filename
is not filled in 'virDomainBackupDefAssignStore' would not fill in the
default location.
Fix the logic to do it also if a 'virStorageSource' categorizes as
empty.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/backup_conf.c | 7 +++++--
tests/domainbackupxml2xmlin/backup-push.xml | 1 +
tests/domainbackupxml2xmlout/backup-push.xml | 3 +++
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index 013c08cd6e..e151c29738 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -424,7 +424,8 @@ virDomainBackupDefAssignStore(virDomainBackupDiskDef *disk,
}
}
- if (!disk->store) {
+ if (!disk->store ||
+ virStorageSourceIsEmpty(disk->store)) {
if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_FILE) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("refusing to generate file name for disk
'%1$s'"),
@@ -432,7 +433,9 @@ virDomainBackupDefAssignStore(virDomainBackupDiskDef *disk,
return -1;
}
- disk->store = virStorageSourceNew();
+ if (!disk->store)
+ disk->store = virStorageSourceNew();
+
disk->store->type = VIR_STORAGE_TYPE_FILE;
disk->store->path = g_strdup_printf("%s.%s", src->path,
suffix);
}
diff --git a/tests/domainbackupxml2xmlin/backup-push.xml
b/tests/domainbackupxml2xmlin/backup-push.xml
index 0bfec9b270..a95833d407 100644
--- a/tests/domainbackupxml2xmlin/backup-push.xml
+++ b/tests/domainbackupxml2xmlin/backup-push.xml
@@ -5,6 +5,7 @@
<driver type='raw'/>
<target file='/path/to/file'/>
</disk>
+ <disk name='vdb' type='file' backupmode='full'/>
<disk name='hda' backup='no'/>
</disks>
</domainbackup>
diff --git a/tests/domainbackupxml2xmlout/backup-push.xml
b/tests/domainbackupxml2xmlout/backup-push.xml
index 317dcf6e47..fff7db716b 100644
--- a/tests/domainbackupxml2xmlout/backup-push.xml
+++ b/tests/domainbackupxml2xmlout/backup-push.xml
@@ -5,6 +5,9 @@
<driver type='raw'/>
<target file='/path/to/file'/>
</disk>
+ <disk name='vdb' backup='yes' type='file'
backupmode='full'>
+ <target file='/fake/vdb.qcow2.SUFFIX'/>
+ </disk>
<disk name='hda' backup='no'/>
<disk name='vdextradisk' backup='no'/>
</disks>
--
2.40.1
7:34 a.m.
New subject: [PATCH 3/3] schemas: backup: Allow missing 'type' attribute for backup disk
One of our examples in the 'formatbackup.rst' page shows following
config:
<disk name='vda' backup='yes'/>
The schema didn't allow it though. Fix the schema as the internals were
supposed to support it (except for the bug fixed in previous patches).
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/schemas/domainbackup.rng | 4 ++++
tests/domainbackupxml2xmlin/backup-push.xml | 2 ++
tests/domainbackupxml2xmlout/backup-push.xml | 6 ++++++
3 files changed, 12 insertions(+)
diff --git a/src/conf/schemas/domainbackup.rng b/src/conf/schemas/domainbackup.rng
index bfc29a6c06..80ba155aad 100644
--- a/src/conf/schemas/domainbackup.rng
+++ b/src/conf/schemas/domainbackup.rng
@@ -166,6 +166,10 @@
<value>no</value>
</attribute>
</group>
+ <!-- Allow to plainly select a disk for backup without any other config
-->
+ <group>
+ <ref name="backupAttr"/>
+ </group>
<group>
<ref name="backupAttr"/>
<attribute name="type">
diff --git a/tests/domainbackupxml2xmlin/backup-push.xml
b/tests/domainbackupxml2xmlin/backup-push.xml
index a95833d407..f1cb38fa82 100644
--- a/tests/domainbackupxml2xmlin/backup-push.xml
+++ b/tests/domainbackupxml2xmlin/backup-push.xml
@@ -6,6 +6,8 @@
<target file='/path/to/file'/>
</disk>
<disk name='vdb' type='file' backupmode='full'/>
+ <disk name='vdc'/>
+ <disk name='vdd' backup='yes'/>
<disk name='hda' backup='no'/>
</disks>
</domainbackup>
diff --git a/tests/domainbackupxml2xmlout/backup-push.xml
b/tests/domainbackupxml2xmlout/backup-push.xml
index fff7db716b..0c6fa6d4ee 100644
--- a/tests/domainbackupxml2xmlout/backup-push.xml
+++ b/tests/domainbackupxml2xmlout/backup-push.xml
@@ -8,6 +8,12 @@
<disk name='vdb' backup='yes' type='file'
backupmode='full'>
<target file='/fake/vdb.qcow2.SUFFIX'/>
</disk>
+ <disk name='vdc' backup='yes' type='file'
backupmode='incremental' incremental='1525889631'>
+ <target file='/fake/vdc.qcow2.SUFFIX'/>
+ </disk>
+ <disk name='vdd' backup='yes' type='file'
backupmode='incremental' incremental='1525889631'>
+ <target file='/fake/vdd.qcow2.SUFFIX'/>
+ </disk>
<disk name='hda' backup='no'/>
<disk name='vdextradisk' backup='no'/>
</disks>
--
2.40.1
8:11 a.m.
On a Monday in 2023, Peter Krempa wrote:
Reported by 'philipp' on OFTC.
Peter Krempa (3):
virDomainBackupDefAssignStore: Restructure control flow
conf: backup: Fix logic for generating default backup filenames
schemas: backup: Allow missing 'type' attribute for backup disk
src/conf/backup_conf.c | 17 +++++++++++------
src/conf/schemas/domainbackup.rng | 4 ++++
tests/domainbackupxml2xmlin/backup-push.xml | 3 +++
tests/domainbackupxml2xmlout/backup-push.xml | 9 +++++++++
4 files changed, 27 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
557
days inactive
557
days old
4 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa