Re: [PATCH v2 2/2] qemuValidateDomainDeviceDefDiskTransient: Add checking set-action capability

[Peter Krempa]

On Mon, Aug 30, 2021 at 00:30:43 -0400, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma 
> 
>  option requires set-action capability
> if destroy action is set to all of the events configuration.
> 
> Signed-off-by: Masayoshi Mizuma 
> ---
>  src/qemu/qemu_validate.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 8906aa52d9..0d49512996 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c

[...]

> @@ -2991,6 +2992,7 @@ qemuValidateDomainDeviceDefDiskBlkdeviotune(const 
> virDomainDiskDef *disk,
>  
>  static int
>  qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk,
> + const virDomainDef *def,
>   virQEMUCaps *qemuCaps)
>  
>  {
> @@ -3033,6 +3035,15 @@ qemuValidateDomainDeviceDefDiskTransient(const 
> virDomainDiskDef *disk,
>  }
>  
>  if (disk->transientShareBacking == VIR_TRISTATE_BOOL_YES) {
> +if (!qemuProcessRebootAllowed(def) &&
> +!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SET_ACTION)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("transient disk backing image sharing with 
> destroy action of lifecycle "
> + "isn't supported by this QEMU binary (%s)"),
> + disk->dst);
> +return -1;

I didn't really consider this corner case worth worrying about.

I'll exchange the two terms in the condition since I prefer when
capability checks go first in such cases and put the error message on a
single line for easier greppability and also add a better explanation of
the specific scenario this is handling into the commit message.

Series:

Reviewed-by: Peter Krempa 

[PATCH v2 2/2] qemuValidateDomainDeviceDefDiskTransient: Add checking set-action capability

[Masayoshi Mizuma]

From: Masayoshi Mizuma 

 option requires set-action capability
if destroy action is set to all of the events configuration.

Signed-off-by: Masayoshi Mizuma 
---
 src/qemu/qemu_validate.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 8906aa52d9..0d49512996 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -24,6 +24,7 @@
 #include "qemu_block.h"
 #include "qemu_command.h"
 #include "qemu_domain.h"
+#include "qemu_process.h"
 #include "domain_conf.h"
 #include "virlog.h"
 #include "virutil.h"
@@ -2991,6 +2992,7 @@ qemuValidateDomainDeviceDefDiskBlkdeviotune(const 
virDomainDiskDef *disk,
 
 static int
 qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk,
+ const virDomainDef *def,
  virQEMUCaps *qemuCaps)
 
 {
@@ -3033,6 +3035,15 @@ qemuValidateDomainDeviceDefDiskTransient(const 
virDomainDiskDef *disk,
 }
 
 if (disk->transientShareBacking == VIR_TRISTATE_BOOL_YES) {
+if (!qemuProcessRebootAllowed(def) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SET_ACTION)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("transient disk backing image sharing with 
destroy action of lifecycle "
+ "isn't supported by this QEMU binary (%s)"),
+ disk->dst);
+return -1;
+}
+
 /* sharing the backing file requires hotplug of the disk in the qemu 
driver */
 switch (disk->bus) {
 case VIR_DOMAIN_DISK_BUS_USB:
@@ -3077,7 +3088,7 @@ qemuValidateDomainDeviceDefDisk(const virDomainDiskDef 
*disk,
 if (qemuValidateDomainDeviceDefDiskBlkdeviotune(disk, def) < 0)
 return -1;
 
-if (qemuValidateDomainDeviceDefDiskTransient(disk, qemuCaps) < 0)
+if (qemuValidateDomainDeviceDefDiskTransient(disk, def, qemuCaps) < 0)
 return -1;
 
 if (disk->src->shared && !disk->src->readonly &&
-- 
2.27.0