Re: [PATCH 03/14] virDomainDiskSourceNVMeFormat: Format only valid 'managed' values

2020-03-24 Thread Michal Prívozník 
On 24. 3. 2020 13:12, Peter Krempa wrote:
> 
> Well, the issue is when the virStorageSource is created from the backing
> store rather than parsed from XML which is added later on.
> 

Should have a source post parse callback then? Anyway,

Reviewed-by: Michal Privoznik 

for whole series.

Michal

Re: [PATCH 03/14] virDomainDiskSourceNVMeFormat: Format only valid 'managed' values

2020-03-24 Thread Peter Krempa 
On Tue, Mar 24, 2020 at 12:38:58 +0100, Michal Privoznik wrote:
> On 23. 3. 2020 19:11, Peter Krempa wrote:
> > VIR_TRISTATE_BOOL_ABSENT which maps to the 'default' string would not be
> > parsed back, so we shouldn't format it either.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >  src/conf/domain_conf.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index f1e4d33a8d..f6a225e4e6 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -24662,8 +24662,9 @@ virDomainDiskSourceNVMeFormat(virBufferPtr attrBuf,
> >const virStorageSourceNVMeDef *nvme)
> >  {
> >  virBufferAddLit(attrBuf, " type='pci'");
> > -virBufferAsprintf(attrBuf, " managed='%s'",
> > -  virTristateBoolTypeToString(nvme->managed));
> > +if (nvme->managed != VIR_TRISTATE_BOOL_ABSENT)
> > +virBufferAsprintf(attrBuf, " managed='%s'",
> > +  virTristateBoolTypeToString(nvme->managed));
> >  virBufferAsprintf(attrBuf, " namespace='%llu'", nvme->namespc);
> >  virPCIDeviceAddressFormat(childBuf, nvme->pciAddr, false);
> >  }
> > 
> 
> There is a postparse callback which deals with _ABSENT (see commit
> 8cd7196974d):
> 
> 
> static int
> virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
>   const virDomainDef *def,
>   virDomainXMLOptionPtr xmlopt)
> {
> ...
> if (disk->src->type == VIR_STORAGE_TYPE_NVME) {
> if (disk->src->nvme->managed == VIR_TRISTATE_BOOL_ABSENT)
> disk->src->nvme->managed = VIR_TRISTATE_BOOL_YES;
> }
> ...
> }
> 
> But maybe the callback is not called from tests where virStorageSource
> is parsed directly?

Well, the issue is when the virStorageSource is created from the backing
store rather than parsed from XML which is added later on.

Re: [PATCH 03/14] virDomainDiskSourceNVMeFormat: Format only valid 'managed' values

2020-03-24 Thread Michal Prívozník 
On 23. 3. 2020 19:11, Peter Krempa wrote:
> VIR_TRISTATE_BOOL_ABSENT which maps to the 'default' string would not be
> parsed back, so we shouldn't format it either.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/conf/domain_conf.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f1e4d33a8d..f6a225e4e6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -24662,8 +24662,9 @@ virDomainDiskSourceNVMeFormat(virBufferPtr attrBuf,
>const virStorageSourceNVMeDef *nvme)
>  {
>  virBufferAddLit(attrBuf, " type='pci'");
> -virBufferAsprintf(attrBuf, " managed='%s'",
> -  virTristateBoolTypeToString(nvme->managed));
> +if (nvme->managed != VIR_TRISTATE_BOOL_ABSENT)
> +virBufferAsprintf(attrBuf, " managed='%s'",
> +  virTristateBoolTypeToString(nvme->managed));
>  virBufferAsprintf(attrBuf, " namespace='%llu'", nvme->namespc);
>  virPCIDeviceAddressFormat(childBuf, nvme->pciAddr, false);
>  }
> 

There is a postparse callback which deals with _ABSENT (see commit
8cd7196974d):


static int
virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
  const virDomainDef *def,
  virDomainXMLOptionPtr xmlopt)
{
...
if (disk->src->type == VIR_STORAGE_TYPE_NVME) {
if (disk->src->nvme->managed == VIR_TRISTATE_BOOL_ABSENT)
disk->src->nvme->managed = VIR_TRISTATE_BOOL_YES;
}
...
}

But maybe the callback is not called from tests where virStorageSource
is parsed directly?

Michal

[PATCH 03/14] virDomainDiskSourceNVMeFormat: Format only valid 'managed' values

2020-03-23 Thread Peter Krempa 
VIR_TRISTATE_BOOL_ABSENT which maps to the 'default' string would not be
parsed back, so we shouldn't format it either.

Signed-off-by: Peter Krempa 
---
 src/conf/domain_conf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f1e4d33a8d..f6a225e4e6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24662,8 +24662,9 @@ virDomainDiskSourceNVMeFormat(virBufferPtr attrBuf,
   const virStorageSourceNVMeDef *nvme)
 {
 virBufferAddLit(attrBuf, " type='pci'");
-virBufferAsprintf(attrBuf, " managed='%s'",
-  virTristateBoolTypeToString(nvme->managed));
+if (nvme->managed != VIR_TRISTATE_BOOL_ABSENT)
+virBufferAsprintf(attrBuf, " managed='%s'",
+  virTristateBoolTypeToString(nvme->managed));
 virBufferAsprintf(attrBuf, " namespace='%llu'", nvme->namespc);
 virPCIDeviceAddressFormat(childBuf, nvme->pciAddr, false);
 }
-- 
2.24.1