1. Patchew
  2. Libvirt
  3. [PATCH v2 0/8] Introduce network backed NVRAM
  4. View patch

[PATCH v2 3/8] Validate remote store NVRAM

Rohit Kumar posted 8 patches 3 years, 10 months ago
There is a newer version of this series
[PATCH v2 3/8] Validate remote store NVRAM
Posted by Rohit Kumar 3 years, 10 months ago 
Remote store NVRAM feature is being enabled only
if it supports 'blockdev' capability.

Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
Signed-off-by: Florian Schmidt <flosch@nutanix.com>
Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
---
 src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 96f5427678..2a961b1f50 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def)
 }
 
 
+static int
+qemuValidateDomainDefNvram(const virDomainDef *def,
+                           virQEMUCaps *qemuCaps)
+{
+    if (def->os.loader && def->os.loader->nvram) {
+        if (def->os.loader->nvram->type !=  VIR_STORAGE_TYPE_FILE &&
+            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("This Qemu does not support 'blockdev' capability "
+                             "for remote store NVRAM. NVRAM type other than "
+                             "'file' is not supported with this QEMU"));
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+
 /**
  * qemuValidateDefGetVcpuHotplugGranularity:
  * @def: domain definition
@@ -1185,6 +1204,9 @@ qemuValidateDomainDef(const virDomainDef *def,
     if (qemuValidateDomainDefBoot(def) < 0)
         return -1;
 
+    if (qemuValidateDomainDefNvram(def, qemuCaps) < 0)
+        return -1;
+
     if (qemuValidateDomainVCpuTopology(def, qemuCaps) < 0)
         return -1;
 
-- 
2.25.1
Re: [PATCH v2 3/8] Validate remote store NVRAM
Posted by Peter Krempa 3 years, 9 months ago 
On Fri, Apr 08, 2022 at 10:48:46 -0700, Rohit Kumar wrote:
> Remote store NVRAM feature is being enabled only
> if it supports 'blockdev' capability.
> 
> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
> Signed-off-by: Florian Schmidt <flosch@nutanix.com>
> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
> ---
>  src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 96f5427678..2a961b1f50 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def)
>  }

As noted in 2/8, this will need to be moved earlier.

>  
>  
> +static int
> +qemuValidateDomainDefNvram(const virDomainDef *def,
> +                           virQEMUCaps *qemuCaps)
> +{

Return early if there's nothing to validate to decrease indentation
level.

> +    if (def->os.loader && def->os.loader->nvram) {
> +        if (def->os.loader->nvram->type !=  VIR_STORAGE_TYPE_FILE &&
> +            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("This Qemu does not support 'blockdev' capability "
> +                             "for remote store NVRAM. NVRAM type other than "
> +                             "'file' is not supported with this QEMU"));

"Remote NVRAM is not supported by this qemu".

> +            return -1;
> +        }
> +    }

Also you need to reject all the other unsupported configs here.

> +
> +    return 0;
Re: [PATCH v2 3/8] Validate remote store NVRAM
Posted by Peter Krempa 3 years, 9 months ago 
On Thu, Apr 21, 2022 at 16:52:54 +0200, Peter Krempa wrote:
> On Fri, Apr 08, 2022 at 10:48:46 -0700, Rohit Kumar wrote:
> > Remote store NVRAM feature is being enabled only
> > if it supports 'blockdev' capability.
> > 
> > Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
> > Signed-off-by: Florian Schmidt <flosch@nutanix.com>
> > Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
> > ---
> >  src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> > index 96f5427678..2a961b1f50 100644
> > --- a/src/qemu/qemu_validate.c
> > +++ b/src/qemu/qemu_validate.c
> > @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def)
> >  }
> 
> As noted in 2/8, this will need to be moved earlier.
> 
> >  
> >  
> > +static int
> > +qemuValidateDomainDefNvram(const virDomainDef *def,
> > +                           virQEMUCaps *qemuCaps)
> > +{
> 
> Return early if there's nothing to validate to decrease indentation
> level.
> 
> > +    if (def->os.loader && def->os.loader->nvram) {
> > +        if (def->os.loader->nvram->type !=  VIR_STORAGE_TYPE_FILE &&
> > +            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("This Qemu does not support 'blockdev' capability "
> > +                             "for remote store NVRAM. NVRAM type other than "
> > +                             "'file' is not supported with this QEMU"));
> 
> "Remote NVRAM is not supported by this qemu".
> 
> > +            return -1;
> > +        }
> > +    }
> 
> Also you need to reject all the other unsupported configs here.

Additionally you'll also need to call the function that validates a
storage source definition too at this point.
Re: [PATCH v2 3/8] Validate remote store NVRAM
Posted by Rohit Kumar 3 years, 9 months ago 
On 21/04/22 8:36 pm, Peter Krempa wrote:
> On Thu, Apr 21, 2022 at 16:52:54 +0200, Peter Krempa wrote:
>> On Fri, Apr 08, 2022 at 10:48:46 -0700, Rohit Kumar wrote:
>>> Remote store NVRAM feature is being enabled only
>>> if it supports 'blockdev' capability.
>>>
>>> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
>>> Signed-off-by: Florian Schmidt <flosch@nutanix.com>
>>> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
>>> ---
>>>   src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
>>> index 96f5427678..2a961b1f50 100644
>>> --- a/src/qemu/qemu_validate.c
>>> +++ b/src/qemu/qemu_validate.c
>>> @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def)
>>>   }
>> As noted in 2/8, this will need to be moved earlier.
Ack.
>>
>>>   
>>>   
>>> +static int
>>> +qemuValidateDomainDefNvram(const virDomainDef *def,
>>> +                           virQEMUCaps *qemuCaps)
>>> +{
>> Return early if there's nothing to validate to decrease indentation
>> level.
Ack.
>>
>>> +    if (def->os.loader && def->os.loader->nvram) {
>>> +        if (def->os.loader->nvram->type !=  VIR_STORAGE_TYPE_FILE &&
>>> +            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                           _("This Qemu does not support 'blockdev' capability "
>>> +                             "for remote store NVRAM. NVRAM type other than "
>>> +                             "'file' is not supported with this QEMU"));
>> "Remote NVRAM is not supported by this qemu".
Ack. Thanks!
>>
>>> +            return -1;
>>> +        }
>>> +    }
>> Also you need to reject all the other unsupported configs here.
Sure. I will update it in the next patch. Thanks!
> Additionally you'll also need to call the function that validates a
> storage source definition too at this point.
Yes, I will add it. Thanks!
>
Re: [PATCH v2 3/8] Validate remote store NVRAM
Posted by Peter Krempa 3 years, 9 months ago 
On Tue, Apr 26, 2022 at 16:16:11 +0530, Rohit Kumar wrote:
> 
> On 21/04/22 8:36 pm, Peter Krempa wrote:
> > On Thu, Apr 21, 2022 at 16:52:54 +0200, Peter Krempa wrote:
> > > On Fri, Apr 08, 2022 at 10:48:46 -0700, Rohit Kumar wrote:
> > > > Remote store NVRAM feature is being enabled only
> > > > if it supports 'blockdev' capability.
> > > > 
> > > > Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
> > > > Signed-off-by: Florian Schmidt <flosch@nutanix.com>
> > > > Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
> > > > ---
> > > >   src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++
> > > >   1 file changed, 22 insertions(+)
> > > > 
> > > > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> > > > index 96f5427678..2a961b1f50 100644
> > > > --- a/src/qemu/qemu_validate.c
> > > > +++ b/src/qemu/qemu_validate.c
> > > > @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def)
> > > >   }
> > > As noted in 2/8, this will need to be moved earlier.
> Ack.

If you don't have any additional points to raise or a discussion to
start, there's no need to acknowledge every point I make in a review.

For anything you acknowledge, simply do what was requested in the next
version.

Here I had to spend time opening a message where simply isn't anything
for me to respond to.
Re: [PATCH v2 3/8] Validate remote store NVRAM
Posted by Ani Sinha 3 years, 10 months ago 
On Fri, Apr 8, 2022 at 11:19 PM Rohit Kumar <rohit.kumar3@nutanix.com> wrote:
>
> Remote store NVRAM feature is being enabled only
> if it supports 'blockdev' capability.
>
> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
> Signed-off-by: Florian Schmidt <flosch@nutanix.com>
> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>

Please add negative unit tests to check the validation along with this
patch. Prefix the patch with "qemu:".

> ---
>  src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 96f5427678..2a961b1f50 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def)
>  }
>
>
> +static int
> +qemuValidateDomainDefNvram(const virDomainDef *def,
> +                           virQEMUCaps *qemuCaps)
> +{
> +    if (def->os.loader && def->os.loader->nvram) {
> +        if (def->os.loader->nvram->type !=  VIR_STORAGE_TYPE_FILE &&
> +            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("This Qemu does not support 'blockdev' capability "
> +                             "for remote store NVRAM. NVRAM type other than "
> +                             "'file' is not supported with this QEMU"));
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
>  /**
>   * qemuValidateDefGetVcpuHotplugGranularity:
>   * @def: domain definition
> @@ -1185,6 +1204,9 @@ qemuValidateDomainDef(const virDomainDef *def,
>      if (qemuValidateDomainDefBoot(def) < 0)
>          return -1;
>
> +    if (qemuValidateDomainDefNvram(def, qemuCaps) < 0)
> +        return -1;
> +
>      if (qemuValidateDomainVCpuTopology(def, qemuCaps) < 0)
>          return -1;
>
> --
> 2.25.1
>
Re: [PATCH v2 3/8] Validate remote store NVRAM
Posted by Rohit Kumar 3 years, 10 months ago 
On 09/04/22 8:19 pm, Ani Sinha wrote:
> On Fri, Apr 8, 2022 at 11:19 PM Rohit Kumar <rohit.kumar3@nutanix.com> wrote:
>> Remote store NVRAM feature is being enabled only
>> if it supports 'blockdev' capability.
>>
>> Signed-off-by: Prerna Saxena <prerna.saxena@nutanix.com>
>> Signed-off-by: Florian Schmidt <flosch@nutanix.com>
>> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
> Please add negative unit tests to check the validation along with this
> patch. Prefix the patch with "qemu:".
Thanks for pointing this out, Ani. Sure, I will add it the next patch 
series.
>
>> ---
>>   src/qemu/qemu_validate.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
>> index 96f5427678..2a961b1f50 100644
>> --- a/src/qemu/qemu_validate.c
>> +++ b/src/qemu/qemu_validate.c
>> @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def)
>>   }
>>
>>
>> +static int
>> +qemuValidateDomainDefNvram(const virDomainDef *def,
>> +                           virQEMUCaps *qemuCaps)
>> +{
>> +    if (def->os.loader && def->os.loader->nvram) {
>> +        if (def->os.loader->nvram->type !=  VIR_STORAGE_TYPE_FILE &&
>> +            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                           _("This Qemu does not support 'blockdev' capability "
>> +                             "for remote store NVRAM. NVRAM type other than "
>> +                             "'file' is not supported with this QEMU"));
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>>   /**
>>    * qemuValidateDefGetVcpuHotplugGranularity:
>>    * @def: domain definition
>> @@ -1185,6 +1204,9 @@ qemuValidateDomainDef(const virDomainDef *def,
>>       if (qemuValidateDomainDefBoot(def) < 0)
>>           return -1;
>>
>> +    if (qemuValidateDomainDefNvram(def, qemuCaps) < 0)
>> +        return -1;
>> +
>>       if (qemuValidateDomainVCpuTopology(def, qemuCaps) < 0)
>>           return -1;
>>
>> --
>> 2.25.1
>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.