util: Fix virStorageBackendIQNFound() to work on FreeBSD

[libvirt] [PATCH] util: Fix virStorageBackendIQNFound() to work on FreeBSD

Posted by Andrea Bolognani 5 years, 8 months ago

Despite being standardized in POSIX.1-2008, the 'm'
sscanf() modifier is currently not available on FreeBSD.

Reimplement parsing without sscanf() to work around the
issue.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/util/viriscsi.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index e2c80acaaa..96934df948 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -146,8 +146,10 @@ virStorageBackendIQNFound(const char *initiatoriqn,
 
     line = outbuf;
     while (line && *line) {
+        char *current = line;
         char *newline;
-        int num;
+        char *next;
+        int i;
 
         if (!(newline = strchr(line, '\n')))
             break;
@@ -156,15 +158,29 @@ virStorageBackendIQNFound(const char *initiatoriqn,
 
         VIR_FREE(iface);
         VIR_FREE(iqn);
-        num = sscanf(line, "%ms %*[^,],%*[^,],%*[^,],%*[^,],%ms", &iface, &iqn);
 
-        if (num != 2) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("malformed output of %s: %s"),
-                           ISCSIADM, line);
+        /* Find the first space, copy everything up to that point into
+         * iface and move past it to continue processing */
+        if (!(next = strchr(current, ' ')))
+            goto error;
+
+        if (VIR_STRNDUP(iface, current, (next - current)) < 0)
             goto cleanup;
+
+        current = next + 1;
+
+        /* There are five comma separated fields after iface and we only
+         * care about the last one, so we need to skip four commas and
+         * copy whatever's left into iqn */
+        for (i = 0; i < 4; i++) {
+            if (!(next = strchr(current, ',')))
+                goto error;
+            current = next + 1;
         }
 
+        if (VIR_STRDUP(iqn, current) < 0)
+            goto cleanup;
+
         if (STREQ(iqn, initiatoriqn)) {
             VIR_STEAL_PTR(*ifacename, iface);
 
@@ -186,6 +202,12 @@ virStorageBackendIQNFound(const char *initiatoriqn,
     VIR_FREE(outbuf);
     virCommandFree(cmd);
     return ret;
+
+ error:
+    virReportError(VIR_ERR_INTERNAL_ERROR,
+                   _("malformed output of %s: %s"),
+                   ISCSIADM, line);
+    goto cleanup;
 }
 
 
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix virStorageBackendIQNFound() to work on FreeBSD

Posted by Michal Privoznik 5 years, 8 months ago

On 07/25/2018 11:30 AM, Andrea Bolognani wrote:
> Despite being standardized in POSIX.1-2008, the 'm'
> sscanf() modifier is currently not available on FreeBSD.
> 
> Reimplement parsing without sscanf() to work around the
> issue.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/util/viriscsi.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)

Le sigh. So apparently we can't take 10 years old standard for granted
and have to use this ugly (as in not obvious) style of parsing. One
possibility is to go to BSD guys and ask them to comply with POSIX, but
that is very likely to fail. So as much as I hate to write this,

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix virStorageBackendIQNFound() to work on FreeBSD

Posted by John Ferlan 5 years, 8 months ago


On 07/25/2018 07:17 AM, Michal Privoznik wrote:
> On 07/25/2018 11:30 AM, Andrea Bolognani wrote:
>> Despite being standardized in POSIX.1-2008, the 'm'
>> sscanf() modifier is currently not available on FreeBSD.
>>
>> Reimplement parsing without sscanf() to work around the
>> issue.
>>
>> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>> ---
>>  src/util/viriscsi.c | 34 ++++++++++++++++++++++++++++------
>>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> Le sigh. So apparently we can't take 10 years old standard for granted
> and have to use this ugly (as in not obvious) style of parsing. One
> possibility is to go to BSD guys and ask them to comply with POSIX, but
> that is very likely to fail. So as much as I hate to write this,
> 
> ACK
> 
> Michal

Should have gone with the overly intrusive virStringSplit[Count] option
like I suggested during review ;-) [which is at least partially open
coded here, but I digress].

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix virStorageBackendIQNFound() to work on FreeBSD

Posted by Andrea Bolognani 5 years, 8 months ago

On Wed, 2018-07-25 at 07:57 -0400, John Ferlan wrote:
> Should have gone with the overly intrusive virStringSplit[Count] option
> like I suggested during review ;-) [which is at least partially open
> coded here, but I digress].

I didn't follow the review, I just jumped on it when I spotted
the build failures on CentOS CI...

If you can come up with a better version that still works on
FreeBSD I'm pretty sure nobody will mind :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list