Firstly, we can utilize virCommandSetOutputBuffer() API which
will collect the command output for us. Secondly, sscanf()-ing
through each line is easier to understand (and more robust) than
jumping over a string with strchr().

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
 1 file changed, 34 insertions(+), 51 deletions(-)

diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 2e55b3c10b..44788056fd 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
 
 
 
-#define LINE_SIZE 4096
 #define IQN_FOUND 1
 #define IQN_MISSING 0
 #define IQN_ERROR -1
@@ -117,71 +116,56 @@ static int
 virStorageBackendIQNFound(const char *initiatoriqn,
                           char **ifacename)
 {
-    int ret = IQN_ERROR, fd = -1;
-    char ebuf[64];
-    FILE *fp = NULL;
-    char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
+    int ret = IQN_ERROR;
+    char *outbuf = NULL;
+    char *line = NULL;
+    char *iface = NULL;
+    char *iqn = NULL;
     virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
                                              "--mode", "iface", NULL);
 
     *ifacename = NULL;
 
-    if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Could not allocate memory for output of '%s'"),
-                       ISCSIADM);
+    virCommandSetOutputBuffer(cmd, &outbuf);
+    if (virCommandRun(cmd, NULL) < 0)
         goto cleanup;
-    }
 
-    memset(line, 0, LINE_SIZE);
+    /* Example of data we are dealing with:
+     * default tcp,<empty>,<empty>,<empty>,<empty>
+     * iser iser,<empty>,<empty>,<empty>,<empty>
+     * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
+     */
 
-    virCommandSetOutputFD(cmd, &fd);
-    if (virCommandRunAsync(cmd, NULL) < 0)
-        goto cleanup;
+    line = outbuf;
+    while (line) {
+        char *newline;
+        int num;
 
-    if ((fp = VIR_FDOPEN(fd, "r")) == NULL) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Failed to open stream for file descriptor "
-                         "when reading output from '%s': '%s'"),
-                       ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf)));
-        goto cleanup;
-    }
+        if (!(newline = strchr(line, '\n')))
+            break;
 
-    while (fgets(line, LINE_SIZE, fp) != NULL) {
-        newline = strrchr(line, '\n');
-        if (newline == NULL) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("Unexpected line > %d characters "
-                             "when parsing output of '%s'"),
-                           LINE_SIZE, ISCSIADM);
-            goto cleanup;
-        }
         *newline = '\0';
 
-        iqn = strrchr(line, ',');
-        if (iqn == NULL)
-            continue;
-        iqn++;
+        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);
+            goto cleanup;
+        }
 
         if (STREQ(iqn, initiatoriqn)) {
-            token = strchr(line, ' ');
-            if (!token) {
-                virReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("Missing space when parsing output "
-                                 "of '%s'"), ISCSIADM);
-                goto cleanup;
-            }
-
-            if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
-                goto cleanup;
+            VIR_STEAL_PTR(*ifacename, iface);
 
             VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn);
             break;
         }
-    }
 
-    if (virCommandWait(cmd, NULL) < 0)
-        goto cleanup;
+        line = newline + 1;
+    }
 
     ret = *ifacename ? IQN_FOUND : IQN_MISSING;
 
@@ -189,11 +173,10 @@ virStorageBackendIQNFound(const char *initiatoriqn,
     if (ret != IQN_FOUND)
         VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
 
-    VIR_FREE(line);
-    VIR_FORCE_FCLOSE(fp);
-    VIR_FORCE_CLOSE(fd);
+    VIR_FREE(iqn);
+    VIR_FREE(iface);
+    VIR_FREE(outbuf);
     virCommandFree(cmd);
-
     return ret;
 }
 
-- 
2.16.4

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

On Fri, Jun 29, 2018 at 17:01:49 +0200, Michal Privoznik wrote:
> Firstly, we can utilize virCommandSetOutputBuffer() API which
> will collect the command output for us. Secondly, sscanf()-ing
> through each line is easier to understand (and more robust) than
> jumping over a string with strchr().
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
>  1 file changed, 34 insertions(+), 51 deletions(-)
> 
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 2e55b3c10b..44788056fd 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c

[...]

> @@ -117,71 +116,56 @@ static int
>  virStorageBackendIQNFound(const char *initiatoriqn,
>                            char **ifacename)
>  {
> -    int ret = IQN_ERROR, fd = -1;
> -    char ebuf[64];
> -    FILE *fp = NULL;
> -    char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
> +    int ret = IQN_ERROR;
> +    char *outbuf = NULL;
> +    char *line = NULL;
> +    char *iface = NULL;
> +    char *iqn = NULL;
>      virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>                                               "--mode", "iface", NULL);
>  
>      *ifacename = NULL;
>  
> -    if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Could not allocate memory for output of '%s'"),
> -                       ISCSIADM);
> +    virCommandSetOutputBuffer(cmd, &outbuf);
> +    if (virCommandRun(cmd, NULL) < 0)
>          goto cleanup;
> -    }
>  
> -    memset(line, 0, LINE_SIZE);
> +    /* Example of data we are dealing with:
> +     * default tcp,<empty>,<empty>,<empty>,<empty>
> +     * iser iser,<empty>,<empty>,<empty>,<empty>
> +     * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
> +     */
>  
> -    virCommandSetOutputFD(cmd, &fd);
> -    if (virCommandRunAsync(cmd, NULL) < 0)
> -        goto cleanup;
> +    line = outbuf;
> +    while (line) {

This is severely misleading and makes this loop technically infinite. Or
just checks that output buffer was not null. Both operations should be
made explicit. Or you meant *line or line[0]

> +        char *newline;
> +        int num;
>  
> -    if ((fp = VIR_FDOPEN(fd, "r")) == NULL) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Failed to open stream for file descriptor "
> -                         "when reading output from '%s': '%s'"),
> -                       ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf)));
> -        goto cleanup;
> -    }
> +        if (!(newline = strchr(line, '\n')))
> +            break;

This is ther reason why it's working at this point.

>  
> -    while (fgets(line, LINE_SIZE, fp) != NULL) {
> -        newline = strrchr(line, '\n');
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 07/02/2018 09:39 AM, Peter Krempa wrote:
> On Fri, Jun 29, 2018 at 17:01:49 +0200, Michal Privoznik wrote:
>> Firstly, we can utilize virCommandSetOutputBuffer() API which
>> will collect the command output for us. Secondly, sscanf()-ing
>> through each line is easier to understand (and more robust) than
>> jumping over a string with strchr().
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
>>  1 file changed, 34 insertions(+), 51 deletions(-)
>>
>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>> index 2e55b3c10b..44788056fd 100644
>> --- a/src/util/viriscsi.c
>> +++ b/src/util/viriscsi.c
> 
> [...]
> 
>> @@ -117,71 +116,56 @@ static int
>>  virStorageBackendIQNFound(const char *initiatoriqn,
>>                            char **ifacename)
>>  {
>> -    int ret = IQN_ERROR, fd = -1;
>> -    char ebuf[64];
>> -    FILE *fp = NULL;
>> -    char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>> +    int ret = IQN_ERROR;
>> +    char *outbuf = NULL;
>> +    char *line = NULL;
>> +    char *iface = NULL;
>> +    char *iqn = NULL;
>>      virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>>                                               "--mode", "iface", NULL);
>>  
>>      *ifacename = NULL;
>>  
>> -    if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("Could not allocate memory for output of '%s'"),
>> -                       ISCSIADM);
>> +    virCommandSetOutputBuffer(cmd, &outbuf);
>> +    if (virCommandRun(cmd, NULL) < 0)
>>          goto cleanup;
>> -    }
>>  
>> -    memset(line, 0, LINE_SIZE);
>> +    /* Example of data we are dealing with:
>> +     * default tcp,<empty>,<empty>,<empty>,<empty>
>> +     * iser iser,<empty>,<empty>,<empty>,<empty>
>> +     * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
>> +     */
>>  
>> -    virCommandSetOutputFD(cmd, &fd);
>> -    if (virCommandRunAsync(cmd, NULL) < 0)
>> -        goto cleanup;
>> +    line = outbuf;
>> +    while (line) {
> 
> This is severely misleading and makes this loop technically infinite. Or
> just checks that output buffer was not null. Both operations should be
> made explicit. Or you meant *line or line[0]

Okay, how about:

while (line && *line) {

Michal

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

On 06/29/2018 11:01 AM, Michal Privoznik wrote:
> Firstly, we can utilize virCommandSetOutputBuffer() API which
> will collect the command output for us. Secondly, sscanf()-ing
> through each line is easier to understand (and more robust) than
> jumping over a string with strchr().
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
>  1 file changed, 34 insertions(+), 51 deletions(-)
> 

I assume virCommandSetOutputBuffer didn't exist when this code was created.

Also, why do I have this recollection related to portability of sscanf?
I know we use it elsewhere, but some oddball issue that someone like
Eric may recollect or know about.

> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 2e55b3c10b..44788056fd 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>  
>  
>  
> -#define LINE_SIZE 4096
>  #define IQN_FOUND 1
>  #define IQN_MISSING 0
>  #define IQN_ERROR -1
> @@ -117,71 +116,56 @@ static int
>  virStorageBackendIQNFound(const char *initiatoriqn,
>                            char **ifacename)
>  {
> -    int ret = IQN_ERROR, fd = -1;
> -    char ebuf[64];
> -    FILE *fp = NULL;
> -    char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
> +    int ret = IQN_ERROR;
> +    char *outbuf = NULL;
> +    char *line = NULL;
> +    char *iface = NULL;
> +    char *iqn = NULL;
>      virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>                                               "--mode", "iface", NULL);
>  
>      *ifacename = NULL;
>  
> -    if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Could not allocate memory for output of '%s'"),
> -                       ISCSIADM);
> +    virCommandSetOutputBuffer(cmd, &outbuf);
> +    if (virCommandRun(cmd, NULL) < 0)
>          goto cleanup;
> -    }
>  
> -    memset(line, 0, LINE_SIZE);
> +    /* Example of data we are dealing with:
> +     * default tcp,<empty>,<empty>,<empty>,<empty>
> +     * iser iser,<empty>,<empty>,<empty>,<empty>
> +     * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
> +     */

Nice to have an example especially since this is a bit of a edge case...

Another option - virStringSplitCount on the "\n" to get the number of
lines and then on each line again using "," to tear apart the pieces.
This I assume works as I don't have a initiatoriqn set up to test.

Any thoughts/recollections about sscanf? I assume it'll be felt that
virStringSplit might be overkill, but at least it's for other purposes
and perhaps less susceptible to the line && *line adjustment.

IDC - let's see if sscanf causes any other recollections before R-by'ing.

John

>  
> -    virCommandSetOutputFD(cmd, &fd);
> -    if (virCommandRunAsync(cmd, NULL) < 0)
> -        goto cleanup;
> +    line = outbuf;
> +    while (line) {
> +        char *newline;
> +        int num;
>  
> -    if ((fp = VIR_FDOPEN(fd, "r")) == NULL) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Failed to open stream for file descriptor "
> -                         "when reading output from '%s': '%s'"),
> -                       ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf)));
> -        goto cleanup;
> -    }
> +        if (!(newline = strchr(line, '\n')))
> +            break;
>  
> -    while (fgets(line, LINE_SIZE, fp) != NULL) {
> -        newline = strrchr(line, '\n');
> -        if (newline == NULL) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Unexpected line > %d characters "
> -                             "when parsing output of '%s'"),
> -                           LINE_SIZE, ISCSIADM);
> -            goto cleanup;
> -        }
>          *newline = '\0';
>  
> -        iqn = strrchr(line, ',');
> -        if (iqn == NULL)
> -            continue;
> -        iqn++;
> +        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);
> +            goto cleanup;
> +        }
>  
>          if (STREQ(iqn, initiatoriqn)) {
> -            token = strchr(line, ' ');
> -            if (!token) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("Missing space when parsing output "
> -                                 "of '%s'"), ISCSIADM);
> -                goto cleanup;
> -            }
> -
> -            if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
> -                goto cleanup;
> +            VIR_STEAL_PTR(*ifacename, iface);
>  
>              VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn);
>              break;
>          }
> -    }
>  
> -    if (virCommandWait(cmd, NULL) < 0)
> -        goto cleanup;
> +        line = newline + 1;
> +    }
>  
>      ret = *ifacename ? IQN_FOUND : IQN_MISSING;
>  
> @@ -189,11 +173,10 @@ virStorageBackendIQNFound(const char *initiatoriqn,
>      if (ret != IQN_FOUND)
>          VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
>  
> -    VIR_FREE(line);
> -    VIR_FORCE_FCLOSE(fp);
> -    VIR_FORCE_CLOSE(fd);
> +    VIR_FREE(iqn);
> +    VIR_FREE(iface);
> +    VIR_FREE(outbuf);
>      virCommandFree(cmd);
> -
>      return ret;
>  }
>  
> 

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

On 07/03/2018 01:18 AM, John Ferlan wrote:
> 
> 
> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>> Firstly, we can utilize virCommandSetOutputBuffer() API which
>> will collect the command output for us. Secondly, sscanf()-ing
>> through each line is easier to understand (and more robust) than
>> jumping over a string with strchr().
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
>>  1 file changed, 34 insertions(+), 51 deletions(-)
>>
> 
> I assume virCommandSetOutputBuffer didn't exist when this code was created.

Perhaps. Let me check git log. .. .. And yes, you are right.
virCommandSetOutputBuffer was introduced among with other basic
virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
function was introduced by Dave in Jan 2010 so he couldn't have used it.

> 
> Also, why do I have this recollection related to portability of sscanf?
> I know we use it elsewhere, but some oddball issue that someone like
> Eric may recollect or know about.

I don't know about anything. But since we use it in our code pretty
freely I assumed there is no problem with it.

> 
>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>> index 2e55b3c10b..44788056fd 100644
>> --- a/src/util/viriscsi.c
>> +++ b/src/util/viriscsi.c
>> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>>  
>>  
>>  
>> -#define LINE_SIZE 4096
>>  #define IQN_FOUND 1
>>  #define IQN_MISSING 0
>>  #define IQN_ERROR -1
>> @@ -117,71 +116,56 @@ static int
>>  virStorageBackendIQNFound(const char *initiatoriqn,
>>                            char **ifacename)
>>  {
>> -    int ret = IQN_ERROR, fd = -1;
>> -    char ebuf[64];
>> -    FILE *fp = NULL;
>> -    char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>> +    int ret = IQN_ERROR;
>> +    char *outbuf = NULL;
>> +    char *line = NULL;
>> +    char *iface = NULL;
>> +    char *iqn = NULL;
>>      virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>>                                               "--mode", "iface", NULL);
>>  
>>      *ifacename = NULL;
>>  
>> -    if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("Could not allocate memory for output of '%s'"),
>> -                       ISCSIADM);
>> +    virCommandSetOutputBuffer(cmd, &outbuf);
>> +    if (virCommandRun(cmd, NULL) < 0)
>>          goto cleanup;
>> -    }
>>  
>> -    memset(line, 0, LINE_SIZE);
>> +    /* Example of data we are dealing with:
>> +     * default tcp,<empty>,<empty>,<empty>,<empty>
>> +     * iser iser,<empty>,<empty>,<empty>,<empty>
>> +     * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
>> +     */
> 
> Nice to have an example especially since this is a bit of a edge case...
> 
> Another option - virStringSplitCount on the "\n" to get the number of
> lines and then on each line again using "," to tear apart the pieces.
> This I assume works as I don't have a initiatoriqn set up to test.
> 
> Any thoughts/recollections about sscanf? I assume it'll be felt that
> virStringSplit might be overkill, 

Indeed.

> but at least it's for other purposes
> and perhaps less susceptible to the line && *line adjustment.

I like sscanf() more because it's fewer lines of code, the variables I
need are assigned value immediately and also memory footprint is smaller
(I guess) since there's no need to store multiple arrays.

Michal

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

On 07/03/2018 01:08 AM, Michal Prívozník wrote:
> On 07/03/2018 01:18 AM, John Ferlan wrote:
>>
>>
>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>>> Firstly, we can utilize virCommandSetOutputBuffer() API which
>>> will collect the command output for us. Secondly, sscanf()-ing
>>> through each line is easier to understand (and more robust) than
>>> jumping over a string with strchr().
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
>>>  1 file changed, 34 insertions(+), 51 deletions(-)
>>>
>>
>> I assume virCommandSetOutputBuffer didn't exist when this code was created.
> 
> Perhaps. Let me check git log. .. .. And yes, you are right.
> virCommandSetOutputBuffer was introduced among with other basic
> virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
> function was introduced by Dave in Jan 2010 so he couldn't have used it.
> 
>>
>> Also, why do I have this recollection related to portability of sscanf?
>> I know we use it elsewhere, but some oddball issue that someone like
>> Eric may recollect or know about.
> 
> I don't know about anything. But since we use it in our code pretty
> freely I assumed there is no problem with it.
> 
>>
>>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>>> index 2e55b3c10b..44788056fd 100644
>>> --- a/src/util/viriscsi.c
>>> +++ b/src/util/viriscsi.c
>>> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>>>  
>>>  
>>>  
>>> -#define LINE_SIZE 4096
>>>  #define IQN_FOUND 1
>>>  #define IQN_MISSING 0
>>>  #define IQN_ERROR -1
>>> @@ -117,71 +116,56 @@ static int
>>>  virStorageBackendIQNFound(const char *initiatoriqn,
>>>                            char **ifacename)
>>>  {
>>> -    int ret = IQN_ERROR, fd = -1;
>>> -    char ebuf[64];
>>> -    FILE *fp = NULL;
>>> -    char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>>> +    int ret = IQN_ERROR;
>>> +    char *outbuf = NULL;
>>> +    char *line = NULL;
>>> +    char *iface = NULL;
>>> +    char *iqn = NULL;
>>>      virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>>>                                               "--mode", "iface", NULL);
>>>  
>>>      *ifacename = NULL;
>>>  
>>> -    if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
>>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -                       _("Could not allocate memory for output of '%s'"),
>>> -                       ISCSIADM);
>>> +    virCommandSetOutputBuffer(cmd, &outbuf);
>>> +    if (virCommandRun(cmd, NULL) < 0)
>>>          goto cleanup;
>>> -    }
>>>  
>>> -    memset(line, 0, LINE_SIZE);
>>> +    /* Example of data we are dealing with:
>>> +     * default tcp,<empty>,<empty>,<empty>,<empty>
>>> +     * iser iser,<empty>,<empty>,<empty>,<empty>
>>> +     * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
>>> +     */
>>
>> Nice to have an example especially since this is a bit of a edge case...
>>
>> Another option - virStringSplitCount on the "\n" to get the number of
>> lines and then on each line again using "," to tear apart the pieces.
>> This I assume works as I don't have a initiatoriqn set up to test.
>>
>> Any thoughts/recollections about sscanf? I assume it'll be felt that
>> virStringSplit might be overkill, 
> 
> Indeed.
> 
>> but at least it's for other purposes
>> and perhaps less susceptible to the line && *line adjustment.
> 
> I like sscanf() more because it's fewer lines of code, the variables I
> need are assigned value immediately and also memory footprint is smaller
> (I guess) since there's no need to store multiple arrays.
> 

Understood - it's probably something really early on when I first
started w/ libvirt and using sscanf was discouraged for something that
has stuck in my head. Another part for me is readability - similar to
the various [i]scsi code that uses regex's in order to parse output.
That format is just a mish-mash of search patterns that causes my eyes
to complain.

Perhaps it's best to see this along with the combined 4&5 "again".  Also
add a non initiatoriqn to the mix (even the comment above) just so show
the various formats.

Hopefully using libiscsi makes all the personally displeasing regex and
sscanf searching code disappear.

John

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

On 07/03/2018 02:51 PM, John Ferlan wrote:
> 
> 
> On 07/03/2018 01:08 AM, Michal Prívozník wrote:
>> On 07/03/2018 01:18 AM, John Ferlan wrote:
>>>
>>>
>>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>>>> Firstly, we can utilize virCommandSetOutputBuffer() API which
>>>> will collect the command output for us. Secondly, sscanf()-ing
>>>> through each line is easier to understand (and more robust) than
>>>> jumping over a string with strchr().
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>  src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
>>>>  1 file changed, 34 insertions(+), 51 deletions(-)
>>>>
>>>
>>> I assume virCommandSetOutputBuffer didn't exist when this code was created.
>>
>> Perhaps. Let me check git log. .. .. And yes, you are right.
>> virCommandSetOutputBuffer was introduced among with other basic
>> virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
>> function was introduced by Dave in Jan 2010 so he couldn't have used it.
>>
>>>
>>> Also, why do I have this recollection related to portability of sscanf?
>>> I know we use it elsewhere, but some oddball issue that someone like
>>> Eric may recollect or know about.
>>
>> I don't know about anything. But since we use it in our code pretty
>> freely I assumed there is no problem with it.
>>
>>>
>>>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>>>> index 2e55b3c10b..44788056fd 100644
>>>> --- a/src/util/viriscsi.c
>>>> +++ b/src/util/viriscsi.c
>>>> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>>>>  
>>>>  
>>>>  
>>>> -#define LINE_SIZE 4096
>>>>  #define IQN_FOUND 1
>>>>  #define IQN_MISSING 0
>>>>  #define IQN_ERROR -1
>>>> @@ -117,71 +116,56 @@ static int
>>>>  virStorageBackendIQNFound(const char *initiatoriqn,
>>>>                            char **ifacename)
>>>>  {
>>>> -    int ret = IQN_ERROR, fd = -1;
>>>> -    char ebuf[64];
>>>> -    FILE *fp = NULL;
>>>> -    char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>>>> +    int ret = IQN_ERROR;
>>>> +    char *outbuf = NULL;
>>>> +    char *line = NULL;
>>>> +    char *iface = NULL;
>>>> +    char *iqn = NULL;
>>>>      virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>>>>                                               "--mode", "iface", NULL);
>>>>  
>>>>      *ifacename = NULL;
>>>>  
>>>> -    if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
>>>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> -                       _("Could not allocate memory for output of '%s'"),
>>>> -                       ISCSIADM);
>>>> +    virCommandSetOutputBuffer(cmd, &outbuf);
>>>> +    if (virCommandRun(cmd, NULL) < 0)
>>>>          goto cleanup;
>>>> -    }
>>>>  
>>>> -    memset(line, 0, LINE_SIZE);
>>>> +    /* Example of data we are dealing with:
>>>> +     * default tcp,<empty>,<empty>,<empty>,<empty>
>>>> +     * iser iser,<empty>,<empty>,<empty>,<empty>
>>>> +     * libvirt-iface-253db048 tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
>>>> +     */
>>>
>>> Nice to have an example especially since this is a bit of a edge case...
>>>
>>> Another option - virStringSplitCount on the "\n" to get the number of
>>> lines and then on each line again using "," to tear apart the pieces.
>>> This I assume works as I don't have a initiatoriqn set up to test.
>>>
>>> Any thoughts/recollections about sscanf? I assume it'll be felt that
>>> virStringSplit might be overkill, 
>>
>> Indeed.
>>
>>> but at least it's for other purposes
>>> and perhaps less susceptible to the line && *line adjustment.
>>
>> I like sscanf() more because it's fewer lines of code, the variables I
>> need are assigned value immediately and also memory footprint is smaller
>> (I guess) since there's no need to store multiple arrays.
>>
> 
> Understood - it's probably something really early on when I first
> started w/ libvirt and using sscanf was discouraged for something that
> has stuck in my head. Another part for me is readability - similar to
> the various [i]scsi code that uses regex's in order to parse output.
> That format is just a mish-mash of search patterns that causes my eyes
> to complain.

Yup.

> 
> Perhaps it's best to see this along with the combined 4&5 "again".  Also
> add a non initiatoriqn to the mix (even the comment above) just so show
> the various formats.
> 
> Hopefully using libiscsi makes all the personally displeasing regex and
> sscanf searching code disappear.

Unfortunately it will not. The libiscsi GSoC project aims on creating
new type of pool "iscsi-direct". It is going to be completely new pool
type (storage driver backend) which will share some similarities with
iscsi pool we have but in some aspects it will be more like rbd pool,
because it is not going to have any <target/> (=no host representation
of LUNs).

Michal

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