Re: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing

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(a)redhat.com&gt; >> --- >> 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.