On 2012年12月28日 23:59, Michal Privoznik wrote:

Since sanlock doesn't run under root:root, we have chown()'ed the __LIBVIRT__DISKS__ lease file to the user:group defined in the sanlock config. However, when writing the patch I've forgot about lease files for each disk (this is the /var/lib/libvirt/sanlock/<md5>) file. --- src/locking/lock_driver_sanlock.c | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 75ced84..c955003 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -679,6 +679,17 @@ static int virLockManagerSanlockCreateLease(struct sanlk_resource *res) } VIR_DEBUG("Someone else just created lockspace %s", res->disks[0].path); } else { + /* chown() the path to make sure sanlock can access it */ + if ((driver->user != -1 || driver->group != -1)&& + (fchown(fd, driver->user, driver->group)< 0)) { + virReportSystemError(errno, + _("cannot chown '%s' to (%u, %u)"), + res->disks[0].path, + (unsigned int) driver->user, + (unsigned int) driver->group); + goto error_unlink; + } + if ((rv = sanlock_align(&res->disks[0]))< 0) { if (rv<= -200) virReportError(VIR_ERR_INTERNAL_ERROR,

ACK

On 12/28/2012 08:59 AM, Michal Privoznik wrote:

Since sanlock doesn't run under root:root, we have chown()'ed the __LIBVIRT__DISKS__ lease file to the user:group defined in the sanlock config. However, when writing the patch I've forgot about lease files for each disk (this is the /var/lib/libvirt/sanlock/<md5>) file. --- src/locking/lock_driver_sanlock.c | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 75ced84..c955003 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -679,6 +679,17 @@ static int virLockManagerSanlockCreateLease(struct sanlk_resource *res) } VIR_DEBUG("Someone else just created lockspace %s", res->disks[0].path); } else { + /* chown() the path to make sure sanlock can access it */ + if ((driver->user != -1 || driver->group != -1) &&

I'm late in my review, but this needs to be: (driver->user != (uid_t) -1 || driver->group != (gid_t) -1) Why? Because POSIX makes no requirements on whether [ug]id_t are signed or unsigned, nor whether they are smaller, equal, or wider than int. Therefore, without the cast, it is possible to hit a platform where the comparison fails to do what you want, if you don't also have the cast in place (that is, it is possible to have '(uid_t)-1 != -1' evaluate to true instead of the intended false, due to integer promotion rules). Thankfully, it's only theoretical at this point (sanlock has only been ported to Linux, where uid_t is sane), but we might as well make the fix. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org