Re: [libvirt] [PATCH 08/12] storage: Cleanup failures virStorageBackendCreateExecCommand
On 10/13/2015 08:50 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:07 -0400, John Ferlan wrote:
> After a successful qemu-img/qcow-create of the backing file, if we
> fail to stat the file, change it owner/group, or mode, then the
> cleanup path should delete the file.
>
> Also moved the virCommandSetUID/virCommandSetGID inside the condition
> used to actually run the command rather than randomly setting and not
> using it if the file had been created. The 'cmd' buffer is only used
> if we need to create.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/storage/storage_backend.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index a375fe0..7d0de63 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
...
> @@ -737,6 +740,8 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
> ret = 0;
>
> cleanup:
> + if (ret < 0 && filecreated)
> + unlink(vol->target.path);
This might not work if the volume was created with different uid/gid as
the process that is attempting to delete it (in case of e.g. NFS.).
Ugh... this function was really strange especially that chown/chmod
after a create on an NETFS target. The net result if the first pile
of 'ifs' passes is a creation in a NETFS pool using either 'qemu-img'
or 'qcow-create' under the uid/gid from the vol->target.{uid|gid}.
So yes, we'd have to virFileRemove that.
The other creation would able to unlink directly, although I suppose a
revector into virFileRemove would work, although it'd be passing uid,
gid == -1, -1.
I know you don't necessarily prefer inline diffs, but this one's fairly
short:
- if (ret < 0 && filecreated)
- unlink(vol->target.path);
+ if (ret < 0 && filecreated) {
+ if (netfs)
+ virFileDelete(vol->target.path, vol->target.uid, vol->target.gid);
+ else
+ unlink(vol->target.path);
+ }
where 'netfs' is a new bool set when we create in that first if
Does this look more reasonable?
John