Re: [libvirt] [PATCH 12/12] storage: On 'buildVol' failure don't delete the volume

On 10/13/2015 08:39 AM, Peter Krempa wrote: > On Fri, Oct 09, 2015 at 09:34:11 -0400, John Ferlan wrote:

> This comment should document the required behavior at the point where we > define the callback structure. Otherwise somebody might miss it as it'd > be stashed in a random function. > OK, I left: /* buildVol handles deleting volume on failure */ Since a few lines later the refreshVol calls the DeleteVol - I just wanted to make it clear that the same delete call is not necessary.

>> + storageVolRemoveFromPool(pool, voldef); >> voldef = NULL; >> goto cleanup; > > ACK if you move the comment to a reasonable place. I hope src/storage/storage_backend.h is your idea of the reasonable place - I added the following text there right above the typedef: /* A 'buildVol' backend must remove any volume created on error since * the storage driver does not distinguish whether the failure is due * to failure to create the volume, to reserve any space necessary for * the volume, to get data about the volume, to change it's accessibility, * etc. This avoids issues arising from a creation failure due to some * external action which created a volume of the same name that libvirt * was not aware of between checking the pool and the create attempt. It * also avoids extra round trips to just delete a file. */ typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, If you had a different location in mind, then let me know.