Couple of miscellaneous cleanups

[PATCH 5/7] vmx: Rework virVMXConfigScanResultsCollector slightly

Posted by Michal Privoznik 3 years, 2 months ago

The idea here is that virVMXConfigScanResultsCollector() sets the
networks_max_index to the highest ethernet index seen. Well, the
struct member is signed int, we parse just seen index into uint
and then typecast to compare the two. This is not necessary,
because the maximum number of NICs a vSphere domain can have is
(<drumrolll/>): ten [1]. This will fit into signed int easily
anywhere.

1: https://configmax.esp.vmware.com/guest?vmwareproduct=vSphere&release=vSphere%208.0&categories=1-0

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/vmx/vmx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index d3e452e3ef..287a877b4a 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -1324,10 +1324,10 @@ virVMXConfigScanResultsCollector(const char* name,
     const char *suffix = NULL;
 
     if ((suffix = STRCASESKIP(name, "ethernet"))) {
-        unsigned int idx;
+        int idx;
         char *p;
 
-        if (virStrToLong_uip(suffix, &p, 10, &idx) < 0 ||
+        if (virStrToLong_i(suffix, &p, 10, &idx) < 0 ||
             *p != '.') {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("failed to parse the index of the VMX key '%s'"),
@@ -1335,8 +1335,8 @@ virVMXConfigScanResultsCollector(const char* name,
             return -1;
         }
 
-        if ((int)idx > results->networks_max_index)
-            results->networks_max_index = (int)idx;
+        if (idx > results->networks_max_index)
+            results->networks_max_index = idx;
     }
 
     return 0;
-- 
2.37.4

Re: [PATCH 5/7] vmx: Rework virVMXConfigScanResultsCollector slightly

Posted by Peter Krempa 3 years, 2 months ago

On Wed, Nov 16, 2022 at 10:11:43 +0100, Michal Privoznik wrote:
> The idea here is that virVMXConfigScanResultsCollector() sets the
> networks_max_index to the highest ethernet index seen. Well, the
> struct member is signed int, we parse just seen index into uint
> and then typecast to compare the two. This is not necessary,
> because the maximum number of NICs a vSphere domain can have is
> (<drumrolll/>): ten [1]. This will fit into signed int easily
> anywhere.
> 
> 1: https://configmax.esp.vmware.com/guest?vmwareproduct=vSphere&release=vSphere%208.0&categories=1-0
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/vmx/vmx.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index d3e452e3ef..287a877b4a 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -1324,10 +1324,10 @@ virVMXConfigScanResultsCollector(const char* name,
>      const char *suffix = NULL;
>  
>      if ((suffix = STRCASESKIP(name, "ethernet"))) {
> -        unsigned int idx;
> +        int idx;
>          char *p;
>  
> -        if (virStrToLong_uip(suffix, &p, 10, &idx) < 0 ||
> +        if (virStrToLong_i(suffix, &p, 10, &idx) < 0 ||
>              *p != '.') {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("failed to parse the index of the VMX key '%s'"),

Just a note, because it isn't obvious neiter from the context, nor from
the commit message.

'virStrToLong_uip' validates that the parsed number is not negative.

Switching to virStrToLong_i allows negative numbers to be considered,
but in this case it won't matter too much. We'd just ignore the network
device if the index for some reason was -1, and additionally we'd trust
any massive positive number anyways.

Re: [PATCH 5/7] vmx: Rework virVMXConfigScanResultsCollector slightly

Posted by Michal Prívozník 3 years, 2 months ago

On 11/16/22 12:35, Peter Krempa wrote:
> On Wed, Nov 16, 2022 at 10:11:43 +0100, Michal Privoznik wrote:
>> The idea here is that virVMXConfigScanResultsCollector() sets the
>> networks_max_index to the highest ethernet index seen. Well, the
>> struct member is signed int, we parse just seen index into uint
>> and then typecast to compare the two. This is not necessary,
>> because the maximum number of NICs a vSphere domain can have is
>> (<drumrolll/>): ten [1]. This will fit into signed int easily
>> anywhere.
>>
>> 1: https://configmax.esp.vmware.com/guest?vmwareproduct=vSphere&release=vSphere%208.0&categories=1-0
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/vmx/vmx.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
>> index d3e452e3ef..287a877b4a 100644
>> --- a/src/vmx/vmx.c
>> +++ b/src/vmx/vmx.c
>> @@ -1324,10 +1324,10 @@ virVMXConfigScanResultsCollector(const char* name,
>>      const char *suffix = NULL;
>>  
>>      if ((suffix = STRCASESKIP(name, "ethernet"))) {
>> -        unsigned int idx;
>> +        int idx;
>>          char *p;
>>  
>> -        if (virStrToLong_uip(suffix, &p, 10, &idx) < 0 ||
>> +        if (virStrToLong_i(suffix, &p, 10, &idx) < 0 ||
>>              *p != '.') {
>>              virReportError(VIR_ERR_INTERNAL_ERROR,
>>                             _("failed to parse the index of the VMX key '%s'"),
> 
> Just a note, because it isn't obvious neiter from the context, nor from
> the commit message.
> 
> 'virStrToLong_uip' validates that the parsed number is not negative.
> 
> Switching to virStrToLong_i allows negative numbers to be considered,
> but in this case it won't matter too much. We'd just ignore the network
> device if the index for some reason was -1, and additionally we'd trust
> any massive positive number anyways.
> 

Yeah, do you want me to put idx < 0 check in or mention this in a commit
message?

Michal

Re: [PATCH 5/7] vmx: Rework virVMXConfigScanResultsCollector slightly

Posted by Peter Krempa 3 years, 2 months ago

On Wed, Nov 16, 2022 at 12:37:30 +0100, Michal Prívozník wrote:
> On 11/16/22 12:35, Peter Krempa wrote:
> > On Wed, Nov 16, 2022 at 10:11:43 +0100, Michal Privoznik wrote:
> >> The idea here is that virVMXConfigScanResultsCollector() sets the
> >> networks_max_index to the highest ethernet index seen. Well, the
> >> struct member is signed int, we parse just seen index into uint
> >> and then typecast to compare the two. This is not necessary,
> >> because the maximum number of NICs a vSphere domain can have is
> >> (<drumrolll/>): ten [1]. This will fit into signed int easily
> >> anywhere.
> >>
> >> 1: https://configmax.esp.vmware.com/guest?vmwareproduct=vSphere&release=vSphere%208.0&categories=1-0
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/vmx/vmx.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> >> index d3e452e3ef..287a877b4a 100644
> >> --- a/src/vmx/vmx.c
> >> +++ b/src/vmx/vmx.c
> >> @@ -1324,10 +1324,10 @@ virVMXConfigScanResultsCollector(const char* name,
> >>      const char *suffix = NULL;
> >>  
> >>      if ((suffix = STRCASESKIP(name, "ethernet"))) {
> >> -        unsigned int idx;
> >> +        int idx;
> >>          char *p;
> >>  
> >> -        if (virStrToLong_uip(suffix, &p, 10, &idx) < 0 ||
> >> +        if (virStrToLong_i(suffix, &p, 10, &idx) < 0 ||
> >>              *p != '.') {
> >>              virReportError(VIR_ERR_INTERNAL_ERROR,
> >>                             _("failed to parse the index of the VMX key '%s'"),
> > 
> > Just a note, because it isn't obvious neiter from the context, nor from
> > the commit message.
> > 
> > 'virStrToLong_uip' validates that the parsed number is not negative.
> > 
> > Switching to virStrToLong_i allows negative numbers to be considered,
> > but in this case it won't matter too much. We'd just ignore the network
> > device if the index for some reason was -1, and additionally we'd trust
> > any massive positive number anyways.
> > 
> 
> Yeah, do you want me to put idx < 0 check in or mention this in a commit
> message?

I don't think it will be a problem even without any change, but you can
add the idx < 0 check to the existing *p != '.' to preserve the
semantics completely.

[PATCH 1/7] coding-style: Follow our own recommendation wrt spacing around commas
[PATCH 2/7] docs: Fill missing docs on STRCASEPREFIX() and STRSKIP()
[PATCH 3/7] internal: Introduce STRCASESKIP()
[PATCH 4/7] vmx: Convert virVMXConfigScanResultsCollector() to use STRCASESKIP()
[PATCH 5/7] vmx: Rework virVMXConfigScanResultsCollector slightly
[PATCH 6/7] conf: Introduce virDomainMemoryDefNew()
[PATCH 7/7] conf: Declare and use autoptr for virDomainMemoryDef