[libvirt] [PATCH] hypervisor driver for Jailhouse

[Christian Loehle]

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?

>> +    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