Re: [libvirt] Bug with creating snapshots in ESX in 0.8.0 (patch included)
2010/4/21 Matthias Bolte <matthias.bolte(a)googlemail.com>:
2010/4/20 Chris Wong <wongc-redhat(a)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