From patchwork Mon Jun 20 18:16:17 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cole Robinson X-Patchwork-Id: 70473 Delivered-To: patch@linaro.org Received: by 10.140.28.4 with SMTP id 4csp1638464qgy; Mon, 20 Jun 2016 11:19:07 -0700 (PDT) X-Received: by 10.31.86.194 with SMTP id k185mr5540052vkb.88.1466446747707; Mon, 20 Jun 2016 11:19:07 -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 e67si20982704vkb.90.2016.06.20.11.19.06 (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 20 Jun 2016 11:19:07 -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 u5KIGJ3T015138; Mon, 20 Jun 2016 14:16:19 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id u5KIGIdb010448 for ; Mon, 20 Jun 2016 14:16:18 -0400 Received: from [10.10.116.65] (ovpn-116-65.rdu2.redhat.com [10.10.116.65]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5KIGIoN006370; Mon, 20 Jun 2016 14:16:18 -0400 To: Jovanka Gulicoska , libvir-list@redhat.com References: <1466439539-2947-1-git-send-email-jovanka.gulicoska@gmail.com> From: Cole Robinson Message-ID: <0bde0948-4da2-5ef6-857b-c1c88cfa4f98@redhat.com> Date: Mon, 20 Jun 2016 14:16:17 -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: <1466439539-2947-1-git-send-email-jovanka.gulicoska@gmail.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-loop: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] 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/20/2016 12:18 PM, 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 | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > index e2d729f..74af35d 100644 > --- a/src/storage/storage_driver.c > +++ b/src/storage/storage_driver.c > @@ -77,6 +77,21 @@ static void storageDriverUnlock(void) > } > > static void > +storagePoolSetInactive(virStoragePoolObjPtr pool) > +{ > + pool->active = false; > + > + if (pool->configFile == NULL) { > + virStoragePoolObjRemove(&driver->pools, pool); > + pool = NULL; > + } else if (pool->newDef) { > + virStoragePoolDefFree(pool->def); > + pool->def = pool->newDef; > + pool->newDef = NULL; > + } > +} > + Hmm. I'm testing this, and I see one problem here. The pool = NULL bit is not propagated to the caller in any way, which is required to not crash in cleanup paths if the pool disappears. I think this function needs to take virStoragePoolObjPtr *pool, and callers pass in &pool, so the *pool = NULL; bit affects the caller. > +static void > storagePoolUpdateState(virStoragePoolObjPtr pool) > { > bool active; > @@ -143,6 +158,7 @@ storagePoolUpdateAllState(void) > virStoragePoolObjPtr pool = driver->pools.objs[i]; > > virStoragePoolObjLock(pool); > + storagePoolSetInactive(pool); > storagePoolUpdateState(pool); > virStoragePoolObjUnlock(pool); I see this bit here is required to fix the reported bug, but I think it should be pushed into storagePoolUpdateState(pool), since there are cases there where the pool may be marked as 'inactive' as well. This is the diff I came up with: > } > @@ -198,6 +214,7 @@ storageDriverAutostart(void) > unlink(stateFile); > if (backend->stopPool) > backend->stopPool(conn, pool); > + storagePoolSetInactive(pool); > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Failed to autostart storage pool '%s': %s"), > pool->def->name, virGetLastErrorMessage()); > @@ -737,7 +754,7 @@ storagePoolCreateXML(virConnectPtr conn, > unlink(stateFile); > if (backend->stopPool) > backend->stopPool(conn, pool); > - virStoragePoolObjRemove(&driver->pools, pool); > + storagePoolSetInactive(pool); > pool = NULL; This pool = NULL; line can be dropped if the above first suggestion is implemented. Otherwise this looks correct to me 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 74af35d..ae965ee 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -130,16 +130,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; } + pool->active = true; } - pool->active = active; ret = 0; error: + if (!active) + storagePoolSetInactive(pool); if (ret < 0) { if (stateFile) unlink(stateFile); @@ -158,7 +161,6 @@ storagePoolUpdateAllState(void) virStoragePoolObjPtr pool = driver->pools.objs[i]; virStoragePoolObjLock(pool); - storagePoolSetInactive(pool); storagePoolUpdateState(pool); virStoragePoolObjUnlock(pool); }