[libvirt] [PATCH] qemu_conf: Properly mark driver->config as self-locking APIs
Since a9e97e0c we don't require driver->config, or virQEMUDriverGetConfig() to be called with locked QEMU driver. In fact, calling it with the driver locked would cause deadlock. Hence, the annotation to the struct field is not right. --- src/qemu/qemu_conf.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 8229cfc..fd5cc57 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -160,8 +160,7 @@ struct _virQEMUDriverConfig { struct _virQEMUDriver { virMutex lock; - /* Require lock to get reference on 'config', - * then lockless thereafter */ + /* Immutable pointer, self-locking APIs */ virQEMUDriverConfigPtr config; /* Immutable pointer, self-locking APIs */ -- 1.8.1.5
On 07/16/2013 12:25 PM, Michal Privoznik wrote:
Since a9e97e0c we don't require driver->config, or virQEMUDriverGetConfig() to be called with locked QEMU driver. In fact, calling it with the driver locked would cause deadlock. Hence, the annotation to the struct field is not right. --- src/qemu/qemu_conf.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
ACK, although you may want danpb to also chime in.
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 8229cfc..fd5cc57 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -160,8 +160,7 @@ struct _virQEMUDriverConfig { struct _virQEMUDriver { virMutex lock;
- /* Require lock to get reference on 'config', - * then lockless thereafter */ + /* Immutable pointer, self-locking APIs */ virQEMUDriverConfigPtr config;
/* Immutable pointer, self-locking APIs */
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jul 16, 2013 at 08:25:25PM +0200, Michal Privoznik wrote:
Since a9e97e0c we don't require driver->config, or virQEMUDriverGetConfig() to be called with locked QEMU driver. In fact, calling it with the driver locked would cause deadlock. Hence, the annotation to the struct field is not right. --- src/qemu/qemu_conf.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 8229cfc..fd5cc57 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -160,8 +160,7 @@ struct _virQEMUDriverConfig { struct _virQEMUDriver { virMutex lock;
- /* Require lock to get reference on 'config', - * then lockless thereafter */ + /* Immutable pointer, self-locking APIs */ virQEMUDriverConfigPtr config;
Actually, it is 'immutable pointer, immutable+lockless data', since we don't require any locks to be held in order to access fields in the config object. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 17.07.2013 11:46, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 08:25:25PM +0200, Michal Privoznik wrote:
Since a9e97e0c we don't require driver->config, or virQEMUDriverGetConfig() to be called with locked QEMU driver. In fact, calling it with the driver locked would cause deadlock. Hence, the annotation to the struct field is not right. --- src/qemu/qemu_conf.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 8229cfc..fd5cc57 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -160,8 +160,7 @@ struct _virQEMUDriverConfig { struct _virQEMUDriver { virMutex lock;
- /* Require lock to get reference on 'config', - * then lockless thereafter */ + /* Immutable pointer, self-locking APIs */ virQEMUDriverConfigPtr config;
Actually, it is 'immutable pointer, immutable+lockless data', since we don't require any locks to be held in order to access fields in the config object.
Regards, Daniel
Consider this squashed in: diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 37d2ba6..ca8a39e 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -357,7 +357,7 @@ static int lxcContainerSetID(virDomainDefPtr def) VIR_DEBUG("Set UID/GID to 0/0"); if (def->idmap.nuidmap && - virSetUIDGID(0, 0, groups, ngroups) < 0) { + virSetUIDGID(0, 0, NULL, 0) < 0) { virReportSystemError(errno, "%s", _("setuid or setgid failed")); return -1;
On Wed, Jul 17, 2013 at 12:32:57PM +0200, Michal Privoznik wrote:
On 17.07.2013 11:46, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 08:25:25PM +0200, Michal Privoznik wrote:
Since a9e97e0c we don't require driver->config, or virQEMUDriverGetConfig() to be called with locked QEMU driver. In fact, calling it with the driver locked would cause deadlock. Hence, the annotation to the struct field is not right. --- src/qemu/qemu_conf.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 8229cfc..fd5cc57 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -160,8 +160,7 @@ struct _virQEMUDriverConfig { struct _virQEMUDriver { virMutex lock;
- /* Require lock to get reference on 'config', - * then lockless thereafter */ + /* Immutable pointer, self-locking APIs */ virQEMUDriverConfigPtr config;
Actually, it is 'immutable pointer, immutable+lockless data', since we don't require any locks to be held in order to access fields in the config object.
Regards, Daniel
Consider this squashed in:
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 37d2ba6..ca8a39e 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -357,7 +357,7 @@ static int lxcContainerSetID(virDomainDefPtr def)
VIR_DEBUG("Set UID/GID to 0/0"); if (def->idmap.nuidmap && - virSetUIDGID(0, 0, groups, ngroups) < 0) { + virSetUIDGID(0, 0, NULL, 0) < 0) { virReportSystemError(errno, "%s", _("setuid or setgid failed")); return -1;
You replied to the wrong message :-) But ACK to the patch since I know what you meant to reply to. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik