From patchwork Wed Jun 22 17:37:58 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cole Robinson X-Patchwork-Id: 70679 Delivered-To: patch@linaro.org Received: by 10.140.28.4 with SMTP id 4csp30271qgy; Wed, 22 Jun 2016 10:41:27 -0700 (PDT) X-Received: by 10.28.207.13 with SMTP id f13mr9821465wmg.53.1466617264737; Wed, 22 Jun 2016 10:41:04 -0700 (PDT) Return-Path: Received: from mx3-phx2.redhat.com (mx3-phx2.redhat.com. [209.132.183.24]) by mx.google.com with ESMTPS id n125si2245868wmb.87.2016.06.22.10.41.04 (version=TLS1 cipher=AES128-SHA bits=128/128); Wed, 22 Jun 2016 10:41:04 -0700 (PDT) Received-SPF: pass (google.com: domain of libvir-list-bounces@redhat.com designates 209.132.183.24 as permitted sender) client-ip=209.132.183.24; Authentication-Results: mx.google.com; spf=pass (google.com: domain of libvir-list-bounces@redhat.com designates 209.132.183.24 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx3-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id u5MHcHOm032275; Wed, 22 Jun 2016 13:38:17 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id u5MHbxnQ024505 for ; Wed, 22 Jun 2016 13:37:59 -0400 Received: from [10.10.116.65] (ovpn-116-65.rdu2.redhat.com [10.10.116.65]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5MHbwWA003791; Wed, 22 Jun 2016 13:37:59 -0400 To: Jovanka Gulicoska , libvir-list@redhat.com References: <1466609048-16724-1-git-send-email-jovanka.gulicoska@gmail.com> From: Cole Robinson Message-ID: <1da71ee8-d380-a2e2-a238-fbbd421c15f9@redhat.com> Date: Wed, 22 Jun 2016 13:37:58 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <1466609048-16724-1-git-send-email-jovanka.gulicoska@gmail.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-loop: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH v3] storage: Fix several issues with transient pool cleanup X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com On 06/22/2016 11:24 AM, Jovanka Gulicoska wrote: > There are several cases where we do not handle transient pool destroy > and cleanup correctly. For example: > > https://bugzilla.redhat.com/show_bug.cgi?id=1227475 > > Move the pool cleanup logic to a new function storagePoolSetInactive and > use it consistently. > --- > src/storage/storage_driver.c | 45 +++++++++++++++++++++++++------------------- > 1 file changed, 26 insertions(+), 19 deletions(-) > > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > index e2d729f..930ae5a 100644 > --- a/src/storage/storage_driver.c > +++ b/src/storage/storage_driver.c > @@ -77,6 +77,23 @@ static void storageDriverUnlock(void) > } > > static void > +storagePoolSetInactive(virStoragePoolObjPtr *poolptr) > +{ > + virStoragePoolObjPtr pool = *poolptr; > + > + pool->active = false; > + > + if (pool->configFile == NULL) { > + virStoragePoolObjRemove(&driver->pools, pool); > + *poolptr = NULL; > + } else if (pool->newDef) { > + virStoragePoolDefFree(pool->def); > + pool->def = pool->newDef; > + pool->newDef = NULL; > + } > +} > + > +static void > storagePoolUpdateState(virStoragePoolObjPtr pool) > { > bool active; > @@ -115,16 +132,19 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) > if (backend->refreshPool(NULL, pool) < 0) { > if (backend->stopPool) > backend->stopPool(NULL, pool); > + active = false; > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Failed to restart storage pool '%s': %s"), > pool->def->name, virGetLastErrorMessage()); > goto error; > } > + active = true; > } > > - pool->active = active; > ret = 0; This bit differs from my suggested diff, and as such pool->active is never activated if active = true; so it needs this diff But actually now that I'm playing with this I'm seeing several other problems tied up in this with related to driver startup and state files. And this particular code path here has some subtle misbehavior because we can remove a storage pool while we are in the middle of iterating over the list of pools... I'm going to hold off on pushing this, but I'm going to work on a series of other cleanups in this area that will include your patch Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 930ae5a..08b2385 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -138,7 +138,7 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) pool->def->name, virGetLastErrorMessage()); goto error; } - active = true; + pool->active = true; } ret = 0;