[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] esx: Add support for memtune/min_guarantee



2010/11/9 Daniel Veillard <veillard redhat com>:
> On Sun, Nov 07, 2010 at 01:40:42PM +0100, Matthias Bolte wrote:
>> Also add a test case for the VMX handling of it.
> [...]
>> +    if (esxVI_LookupVirtualMachineByUuidAndPrepareForTask
>> +          (priv->primary, domain->uuid, NULL, &virtualMachine,
>> +           priv->autoAnswer) < 0 ||
> [...]
>> +    if (esxVI_String_AppendValueToList
>> +          (&propertyNameList, "config.memoryAllocation.reservation") < 0 ||
>> +        esxVI_LookupVirtualMachineByUuid(priv->primary, domain->uuid,
>
>  Small nitpick to prove I read the patch :-)
> It seems your editor tend to cut lines in different way if within
> a condition than if within a normal expression, I would have put
>      (&propertyNameList,
> on the same line as the if ( ...

Well, if the parameter list cannot be split at a ',' because the
parameters would still be too long then I put the opening '(' at the
next line, two spaces indented relative to the function name. This is
independent from the context of the function call. No inconsistency
here IMHO :)

> [...]
>> +int
>> +esxVI_GetLong(esxVI_ObjectContent *objectContent, const char *propertyName,
>> +              esxVI_Long **value, esxVI_Occurrence occurence)
>> +{
>> +    esxVI_DynamicProperty *dynamicProperty;
>> +
>> +    if (value == NULL || *value != NULL) {
>> +        ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
>> +        return -1;
>> +    }
>> +
>> +    for (dynamicProperty = objectContent->propSet; dynamicProperty != NULL;
>> +         dynamicProperty = dynamicProperty->_next) {
>> +        if (STREQ(dynamicProperty->name, propertyName)) {
>> +            if (esxVI_Long_CastFromAnyType(dynamicProperty->val, value) < 0) {
>
>  Hum is esxVI_Long_CastFromAnyType raising an error if failing, because
> we don't rais an error in the caller if esxVI_GetLong() fails.

Yes, basically all functions in the ESX driver raise errors
themselves. esxVI_Long_CastFromAnyType is no exception here, it's just
no visible in the patch :)

The only exceptions are esxUtil_UnescapeHex and
esxUtil_ParseVirtualMachineIDString. esxUtil_UnescapeHex doesn't do it
because the caller can report a better error message (with more
context) and esxUtil_ParseVirtualMachineIDString has no reason to do
so... and I'll change that. Thanks for asking :)

>
> ACK,
>
> Daniel

Thanks, pushed.

Matthias


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]