On 07/17/2018 08:42 PM, John Ferlan wrote:
>
>
> On 07/04/2018 05:23 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@xxxxxxxxxx>
>> ---
>> 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 01b5b4be68..1ddf00aa4c 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 && *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;
>>
>
> The next hunk is is going to pick up a non libvirt generated
> initiator.iqn, is that something we want? Or should we :
>
> if (!STRPREFIX(line, "libvirt-iface-"))
> continue;
I don't think we need this. Firstly, the code as is now doesn't care
about that either. Secondly, even though we currently call the function
only to learn about libvirt-iface-* it is generic enough to be called
over any interface.
>
> where "libvirt-iface-" could be made into a #define constant for usage
> by both this and virStorageBackendCreateIfaceIQN
>
>> - 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);
>
> Reading up on sscanf, the '%ms' seems to imply a GNU lib C mechanism vs.
> a C standard mechanism. So the query/concern being all our various build
> environments acceptance of the 'm' qualifier (at least coverity didn't
> choke on it ;-)).
>
Not really. From sscanf(3):
The use of the letter a for this purpose was problematic, since
a is also specified by the ISO C standard as a synonym for f
(floating-point input). POSIX.1-2008 instead specifies the m modifier
for assignment allocation (as documented in DESCRIPTION, above).
So it's mandated by POSIX which should be good enough for us to use.
> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
Thanks,
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
