[libvirt] [PATCH] hypervisor driver for Jailhouse

[Daniel P. Berrange]

On Fri, Nov 13, 2015 at 10:28:02AM +0100, Christian Loehle wrote:
> On 11/11/2015 10:17 AM, Daniel P. Berrange wrote:
> >> +    virCommandSetOutputBuffer(cmd, &output);
> >> +    size_t count = -1; //  Don't count table header line
> >> +    size_t i = 0;
> >> +    if (virCommandRun(cmd, NULL) < 0)
> >> +        goto error;
> >> +    while (output[i] != '\0') {
> >> +        if (output[i] == '\n') count++;
> >> +        i++;
> >> +    }
> >> +    if (VIR_ALLOC_N(*parsedOutput, count)) {
> >> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                  _("Failed to allocate jailhouse_cell array of size %zu"), count);
> > 
> > Again no need to report error. You need to check
> > 
> >  VIR_ALLOC_N(...) < 0
> > 
> > too - this error check is inverted right now
> 
> I removed the report, but I don't quite understand why this check
> would be inverted.
> VIR_ALLOC_N returns 0 on success, in which case it won't enter the
> if block and -1 on failure(OOM) in which case the if block will be
> entered. Care to elaborate?

Ok, saying it was inverted is not accurate, but it is at risk of
incorrect behaviour because it treats positive integers as failure
and by convention in libvirt positive integers always indicate
success. It just happens that we VIR_ALLOC_N doesn't /currently/
return a positive integer, but it could in future....

> >> +    i = 0;
> >> +    size_t j;
> >> +    while (output[i++] != '\n'); //  Skip table header line
> >> +    for (j = 0; j < count; j++) {
> >> +        size_t k;
> >> +        for (k = 0; k <= IDLENGTH; k++) // char after number needs to be NUL for virStrToLong
> >> +            if (output[i+k] == ' ') {
> >> +                output[i+k] = '\0';
> >> +                break;
> >> +            }
> >> +        char c = output[i+IDLENGTH];
> >> +        output[i+IDLENGTH] = '\0'; //   in case ID is 8 chars long, so beginning of name won't get parsed
> >> +        if (virStrToLong_i(output+i, NULL, 0, &(*parsedOutput)[j].id))
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                    _("Failed to parse id to long: %s"), output+i);
> >> +        output[i+IDLENGTH] = c;
> >> +        i += IDLENGTH;
> >> +        if (virStrncpy((*parsedOutput)[j].name, output+i, NAMELENGTH, NAMELENGTH+1) == NULL)
> >> +            // should never happen
> >> +            goto error;
> >> +        (*parsedOutput)[j].name[NAMELENGTH] = '\0';
> >> +        for (k = 0; k < NAMELENGTH; k++)
> >> +            if ((*parsedOutput)[j].name[k] == ' ')
> >> +                    break;
> >> +        (*parsedOutput)[j].name[k] = '\0';
> >> +        i += NAMELENGTH;
> >> +        if (STREQLEN(output+i, STATERUNNINGSTRING, STATELENGTH)) (*parsedOutput)[j].state = STATERUNNING;
> >> +        else if (STREQLEN(output+i, STATESHUTDOWNSTRING, STATELENGTH)) (*parsedOutput)[j].state = STATESHUTDOWN;
> >> +        else if (STREQLEN(output+i, STATEFAILEDSTRING, STATELENGTH)) (*parsedOutput)[j].state = STATEFAILED;
> >> +        else if (STREQLEN(output+i, STATERUNNINGLOCKEDSTRING, STATELENGTH)) (*parsedOutput)[j].state = STATERUNNINGLOCKED;
> >> +        i += STATELENGTH;
> >> +        (*parsedOutput)[j].assignedCPUsLength = parseCPUs(output+i, &((*parsedOutput)[j].assignedCPUs));
> >> +        i += CPULENGTH;
> >> +        (*parsedOutput)[j].failedCPUsLength = parseCPUs(output+i, &((*parsedOutput)[j].failedCPUs));
> >> +        i += CPULENGTH;
> >> +        i++; // skip \n
> >> +    }
> > 
> > It would be nice to include an example of the 'cell list' output to understand
> > what this code is trying to parse. Then i migth be able to suggest a way
> > to do this more simply.
> 
> ID      Name                    State           Assigned CPUs           Failed CPUs
> 0       QEMU-VM                 running         0-3

So you could use virStringSplit once to break the output into lines
and then use either virStringSplit a second time or a regex match
to break each line into fields, splitting on space character.

An alternative approach might be to use virCommandRunRegex which
takes care of splitting output into lines, and uses a regex to
split each line into fields. It just invokes a callback that
gives you a list of the pieces for each line.

I think either approach would probably be clearer that this code
you have currently.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|