Re: [libvirt] [PATCH 1/3] storage: refresh volume before deletion

[...] >> OK - so a too long winded way to say - I don't think the following patch >> matters as anything (including libvirt) could have changed the file's >> ownership and/or protections, but not updated the volume XML. The >> refresh updates the volume XML to match the file's protection and >> furthermore only matters in the current virFileRemove if the change is >> back to root:root and only because we compare vs. gete{uid|gid}(). >> > > I don't follow this conclusion really... refreshVol syncs the XML/volume cache > with the current uid/gid/mode on disk, which is then passed down to > virFileRemove. If the cached uid is 'qemu' but the on disk uid is 'cole', the > internal cache is synced, virFileRemove will setuid(cole) and successfully > remove the disk image. Without this change, libvirt would setuid(qemu) and > fail to remove the disk. > > So I don't see how this patch is unneeded, or only works for root:root > I was considering the checks: + if (geteuid() != 0) + return false; We're running as root... + + /* uid/gid weren'd specified */ + if ((uid == (uid_t) -1) && (gid == (gid_t) -1)) + return false; We've provided qemu:qemu... + + /* already running as proper uid/gid */ + if (uid == geteuid() && gid == getegid()) + return false; + At this point geteuid() would return 0 (root) Maybe I'm wrong... I suppose it cannot hurt, but without patch 3 we'd go through the setuid path - I think. I could also be missing something really obvious.