[libvirt] Bug with creating snapshots in ESX in 0.8.0 (patch included)
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. 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. 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. 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, 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; - } - cleanup: esxVI_String_Free(&propertyNameList); esxVI_ObjectContent_Free(&virtualMachine);
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, 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; - } - 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. Thanks for reporting this issue. Matthias
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
participants (2)
-
Chris Wong -
Matthias Bolte