1. Patchew
  2. Libvirt
  3. View series

[PATCH] virt-host-validate: Allow longer list of CPU flags

Michal Privoznik posted 1 patch 3 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/2b6c754248f8e315529d3b401f2ae19f9b44e22f.1721731291.git.mprivozn@redhat.com
There is a newer version of this series
tools/virt-host-validate-common.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
[PATCH] virt-host-validate: Allow longer list of CPU flags
Posted by Michal Privoznik 3 months, 1 week ago 
On various occasions, virt-host-validate parses /proc/cpuinfo to
learn about CPU flags (see virHostValidateGetCPUFlags()). It does
so, by reading the file line by line until the line with CPU
flags is reached. Then the line is split into individual flags
(using space as a delimiter) and the list of flags is then
iterated over.

This works, except for cases when the line with CPU flags is too
long. Problem is - the line is capped at 1024 bytes and on newer
CPUs (and newer kernels), the line can be significantly longer.
I've seen a line that's ~1200 characters long (with 164 flags
reported).

Switch to unbounded read from the file (getline()).

Resolves: https://issues.redhat.com/browse/RHEL-39969
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/virt-host-validate-common.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
index 591143c24d..e5fa6606d2 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -106,21 +106,19 @@ virBitmap *virHostValidateGetCPUFlags(void)
 {
     FILE *fp;
     virBitmap *flags = NULL;
+    g_autofree char *line = NULL;
+    size_t linelen = 0;
 
     if (!(fp = fopen("/proc/cpuinfo", "r")))
         return NULL;
 
     flags = virBitmapNew(VIR_HOST_VALIDATE_CPU_FLAG_LAST);
 
-    do {
-        char line[1024];
+    while (getline(&line, &linelen, fp) > 0) {
         char *start;
         g_auto(GStrv) tokens = NULL;
         GStrv next;
 
-        if (!fgets(line, sizeof(line), fp))
-            break;
-
         /* The line we're interested in is marked differently depending
          * on the architecture, so check possible prefixes */
         if (!STRPREFIX(line, "flags") &&
@@ -129,12 +127,6 @@ virBitmap *virHostValidateGetCPUFlags(void)
             !STRPREFIX(line, "facilities"))
             continue;
 
-        /* fgets() includes the trailing newline in the output buffer,
-         * so we need to clean that up ourselves. We can safely access
-         * line[strlen(line) - 1] because the checks above would cause
-         * us to skip empty strings */
-        line[strlen(line) - 1] = '\0';
-
         /* Skip to the separator */
         if (!(start = strchr(line, ':')))
             continue;
@@ -153,7 +145,7 @@ virBitmap *virHostValidateGetCPUFlags(void)
             if ((value = virHostValidateCPUFlagTypeFromString(*next)) >= 0)
                 ignore_value(virBitmapSetBit(flags, value));
         }
-    } while (1);
+    }
 
     VIR_FORCE_FCLOSE(fp);
 
-- 
2.44.2
Re: [PATCH] virt-host-validate: Allow longer list of CPU flags
Posted by Jiri Denemark 3 months, 1 week ago 
On Tue, Jul 23, 2024 at 12:41:31 +0200, Michal Privoznik wrote:
> On various occasions, virt-host-validate parses /proc/cpuinfo to
> learn about CPU flags (see virHostValidateGetCPUFlags()). It does
> so, by reading the file line by line until the line with CPU
> flags is reached. Then the line is split into individual flags
> (using space as a delimiter) and the list of flags is then
> iterated over.
> 
> This works, except for cases when the line with CPU flags is too
> long. Problem is - the line is capped at 1024 bytes and on newer
> CPUs (and newer kernels), the line can be significantly longer.
> I've seen a line that's ~1200 characters long (with 164 flags
> reported).
> 
> Switch to unbounded read from the file (getline()).
> 
> Resolves: https://issues.redhat.com/browse/RHEL-39969
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  tools/virt-host-validate-common.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
> index 591143c24d..e5fa6606d2 100644
> --- a/tools/virt-host-validate-common.c
> +++ b/tools/virt-host-validate-common.c
> @@ -106,21 +106,19 @@ virBitmap *virHostValidateGetCPUFlags(void)
>  {
>      FILE *fp;
>      virBitmap *flags = NULL;
> +    g_autofree char *line = NULL;
> +    size_t linelen = 0;
>  
>      if (!(fp = fopen("/proc/cpuinfo", "r")))
>          return NULL;
>  
>      flags = virBitmapNew(VIR_HOST_VALIDATE_CPU_FLAG_LAST);
>  
> -    do {
> -        char line[1024];
> +    while (getline(&line, &linelen, fp) > 0) {
>          char *start;
>          g_auto(GStrv) tokens = NULL;
>          GStrv next;
>  
> -        if (!fgets(line, sizeof(line), fp))
> -            break;
> -
>          /* The line we're interested in is marked differently depending
>           * on the architecture, so check possible prefixes */
>          if (!STRPREFIX(line, "flags") &&
> @@ -129,12 +127,6 @@ virBitmap *virHostValidateGetCPUFlags(void)
>              !STRPREFIX(line, "facilities"))
>              continue;
>  
> -        /* fgets() includes the trailing newline in the output buffer,
> -         * so we need to clean that up ourselves. We can safely access
> -         * line[strlen(line) - 1] because the checks above would cause
> -         * us to skip empty strings */
> -        line[strlen(line) - 1] = '\0';
> -

getline() also includes the trailing newline in the buffer (as long as
there is any newline) and thus we still need to drop it. We just don't
need to call strlen to compute the length. Although I would suggest
using a little bit more robust code instead of describing the existing
code is safe...

    if (linelen > 1 && line[linelen - 1] == '\n')
        line[linelen - 1] = '\0';

>          /* Skip to the separator */
>          if (!(start = strchr(line, ':')))
>              continue;
> @@ -153,7 +145,7 @@ virBitmap *virHostValidateGetCPUFlags(void)
>              if ((value = virHostValidateCPUFlagTypeFromString(*next)) >= 0)
>                  ignore_value(virBitmapSetBit(flags, value));
>          }
> -    } while (1);
> +    }
>  
>      VIR_FORCE_FCLOSE(fp);
>  

Jirka
Re: [PATCH] virt-host-validate: Allow longer list of CPU flags
Posted by Michal Prívozník 3 months, 1 week ago 
On 7/23/24 13:08, Jiri Denemark wrote:
> On Tue, Jul 23, 2024 at 12:41:31 +0200, Michal Privoznik wrote:
>> On various occasions, virt-host-validate parses /proc/cpuinfo to
>> learn about CPU flags (see virHostValidateGetCPUFlags()). It does
>> so, by reading the file line by line until the line with CPU
>> flags is reached. Then the line is split into individual flags
>> (using space as a delimiter) and the list of flags is then
>> iterated over.
>>
>> This works, except for cases when the line with CPU flags is too
>> long. Problem is - the line is capped at 1024 bytes and on newer
>> CPUs (and newer kernels), the line can be significantly longer.
>> I've seen a line that's ~1200 characters long (with 164 flags
>> reported).
>>
>> Switch to unbounded read from the file (getline()).
>>
>> Resolves: https://issues.redhat.com/browse/RHEL-39969
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  tools/virt-host-validate-common.c | 16 ++++------------
>>  1 file changed, 4 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
>> index 591143c24d..e5fa6606d2 100644
>> --- a/tools/virt-host-validate-common.c
>> +++ b/tools/virt-host-validate-common.c
>> @@ -106,21 +106,19 @@ virBitmap *virHostValidateGetCPUFlags(void)
>>  {
>>      FILE *fp;
>>      virBitmap *flags = NULL;
>> +    g_autofree char *line = NULL;
>> +    size_t linelen = 0;
>>  
>>      if (!(fp = fopen("/proc/cpuinfo", "r")))
>>          return NULL;
>>  
>>      flags = virBitmapNew(VIR_HOST_VALIDATE_CPU_FLAG_LAST);
>>  
>> -    do {
>> -        char line[1024];
>> +    while (getline(&line, &linelen, fp) > 0) {
>>          char *start;
>>          g_auto(GStrv) tokens = NULL;
>>          GStrv next;
>>  
>> -        if (!fgets(line, sizeof(line), fp))
>> -            break;
>> -
>>          /* The line we're interested in is marked differently depending
>>           * on the architecture, so check possible prefixes */
>>          if (!STRPREFIX(line, "flags") &&
>> @@ -129,12 +127,6 @@ virBitmap *virHostValidateGetCPUFlags(void)
>>              !STRPREFIX(line, "facilities"))
>>              continue;
>>  
>> -        /* fgets() includes the trailing newline in the output buffer,
>> -         * so we need to clean that up ourselves. We can safely access
>> -         * line[strlen(line) - 1] because the checks above would cause
>> -         * us to skip empty strings */
>> -        line[strlen(line) - 1] = '\0';
>> -
> 
> getline() also includes the trailing newline in the buffer (as long as
> there is any newline) and thus we still need to drop it. We just don't
> need to call strlen to compute the length. Although I would suggest
> using a little bit more robust code instead of describing the existing
> code is safe...

good point

> 
>     if (linelen > 1 && line[linelen - 1] == '\n')
>         line[linelen - 1] = '\0';

I'm going with virStringTrimOptionalNewline().


Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.