2010/4/21 Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>:
> 2010/4/20 Chris Wong <wongc-redhat@xxxxxxxx>:
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list