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

Re: [libvirt] Bug with creating snapshots in ESX in 0.8.0 (patch included)



2010/4/21 Matthias Bolte <matthias bolte googlemail com>:
> 2010/4/20 Chris Wong <wongc-redhat hoku net>:
>> I was testing out the new snapshot on functionality with GSX (VMWare Server
>> 2.0.2) and noticed that it would fail to create a snapshot on a VM with no
>> snapshots. I tracked it down to the esxDomainSnapshotCreateXML call, which
>> would prematurely fail if the Root Snapshot Tree was empty -- which it would
>> be.
>
> Seems like I didn't test this with a VM without snapshots and did
> notice this problem. Sorry for that.
>
>> Verified that both ESXi 4.0 and GSX return an empty snapshot tree. I don't
>> feel this should be an error, and it prevents snapshots from being created.
>
> Yes, an empty root snapshot list is a valid response.
>
>> I don't think I saw a fix for this in the master git branch. I'm not too
>> sure if this is the proper path to go down, but at the very least it fixes
>> my specific problem. If there is another fix or if I completely missed this
>> point, let me know.
>
> The problem has not been fixed yet, because I wasn't aware of it until now.
>
>> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
>> index fea887a..70bfc2c 100644
>> --- a/src/esx/esx_driver.c
>> +++ b/src/esx/esx_driver.c
>> @@ -3330,12 +3330,16 @@ esxDomainSnapshotCreateXML(virDomainPtr domain,
>> const char *xmlDesc,
>>            (priv->host, domain->uuid, NULL, &virtualMachine,
>>             priv->autoAnswer) < 0 ||
>>          esxVI_LookupRootSnapshotTreeList(priv->host, domain->uuid,
>> -                                         &rootSnapshotList) < 0 ||
>> -        esxVI_GetSnapshotTreeByName(rootSnapshotList, def->name,
>> -                                    &snapshotTree, &snapshotTreeParent,
>> -                                    esxVI_Occurrence_OptionalItem) < 0) {
>> +                                         &rootSnapshotList) < 0) {
>>          goto failure;
>>      }
>> +    if (rootSnapshotList) {
>> +        if (esxVI_GetSnapshotTreeByName(rootSnapshotList, def->name,
>> +                                        &snapshotTree, &snapshotTreeParent,
>> +                                        esxVI_Occurrence_OptionalItem) < 0)
>> {
>> +            goto failure;
>> +        }
>> +    }
>>      if (snapshotTree != NULL) {
>>          ESX_ERROR(VIR_ERR_OPERATION_INVALID,

This change is not necessary. It's valid to call
esxVI_GetSnapshotTreeByName with rootSnapshotList == NULL. NULL
represents an empty list and esxVI_GetSnapshotTreeByName is called
with esxVI_Occurrence_OptionalItem, so its okay if the requested
snapshot doesn't exist. One could argue that calling
esxVI_GetSnapshotTreeByName is pointless when we know that the list is
empty, but the current code is less complex and trying to avoid a
function call is pointless compared to all the SOAP communication
that's going on.

>> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
>> index 89ef2dd..93ef36b 100644
>> --- a/src/esx/esx_vi.c
>> +++ b/src/esx/esx_vi.c
>> @@ -2561,12 +2561,6 @@ esxVI_LookupRootSnapshotTreeList
>>          }
>>      }
>> -    if (*rootSnapshotTreeList == NULL) {
>> -        ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                     _("Could not lookup root snapshot list"));
>> -        goto failure;
>> -    }
>> -

This is the important change, because NULL is a perfectly valid empty
list. This check should have never been there.

>>    cleanup:
>>      esxVI_String_Free(&propertyNameList);
>>      esxVI_ObjectContent_Free(&virtualMachine);
>>
>
> Your patch looks good, but I'll have to validate that removing the
> NULL check from esxVI_LookupRootSnapshotTreeList doesn't break any
> other caller of this function.

I check it and removing the check for NULL doesn't break anything else.

Okay, I applied and pushed the relevant part of your patch.

Matthias


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