Re: [libvirt] [PATCH 08/12] storage: Cleanup failures virStorageBackendCreateExecCommand
On 10/14/2015 08:21 AM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 15:11:01 -0400, John Ferlan wrote:
> On 10/13/2015 08:50 AM, Peter Krempa wrote:
>> On Fri, Oct 09, 2015 at 09:34:07 -0400, John Ferlan wrote:
...
>>
>> 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.
It might be a better option to refactor it prior to tweaking the cleanup
path.
I think for the uid/gid && chown paths - yes, I can see that. The way I
read the code, we'll get the uid/gid of the created file from the NFS
pool and it'll be the same as "vol->target.perms->{uid|gid}", thus we
won't make the chown call because the uid/gid will be set to -1. So I
think it could be refactored inside the "if (!filecreated)".
However, it's the chmod path that's got me thinking a bit harder. It
seems that code should check "st.st_mode != mode" before calling -
although chmod could consider that a noop before it "checks" whether the
calling process has the ability to perform the change. Regardless,
since the NETFS path doesn't include setting mode bits (e.g. generating
a "mask" to be used in the umask at the start of the command
processing), thus it seems that path would attempt the chmod path.
Although perhaps we've been "fortunate" to not use this path for NETFS
since it seems logically to me it would have failed in the funky root
squash environment. I'll have to give this path a try since I have root
squash pool configured.
John
>
> 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?
Perhaps even without the netfs check if you think it makes sense so that
the logic isn't overcomplicated.
Peter