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

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(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. > 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.