On Mon, Oct 05, 2015 at 07:27:34PM -0400, John Ferlan wrote: > > > On 10/05/2015 07:28 AM, Michal Privoznik wrote: > > On 02.10.2015 15:41, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1233003 > >> > >> Although perhaps bordering on a don't do that type scenario, if > >> someone creates a volume in a pool outside of libvirt, then uses that > >> same name to create a volume in the pool via libvirt, then the creation > >> will fail and in some cases cause the same name volume to be deleted. > >> > >> This patch will refresh the pool just prior to checking whether the > >> named volume exists prior to creating the volume in the pool. While > >> it's still possible to have a timing window to create a file after the > >> check - at least we tried. At that point, someone is being malicious. > >> > >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> > >> --- > >> src/storage/storage_driver.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > >> index 7aaa060..ddf4405 100644 > >> --- a/src/storage/storage_driver.c > >> +++ b/src/storage/storage_driver.c > >> @@ -1796,6 +1796,15 @@ storageVolCreateXML(virStoragePoolPtr obj, > >> if (virStorageVolCreateXMLEnsureACL(obj->conn, pool->def, voldef) < 0) > >> goto cleanup; > >> > >> + /* While not perfect, refresh the list of volumes in the pool and > >> + * then check that the incoming name isn't already in the pool. > >> + */ > >> + if (backend->refreshPool) { > >> + virStoragePoolObjClearVols(pool); > >> + if (backend->refreshPool(obj->conn, pool) < 0) > >> + goto cleanup; > >> + } > >> + > >> if (virStorageVolDefFindByName(pool, voldef->name)) { > >> virReportError(VIR_ERR_STORAGE_VOL_EXIST, > >> _("'%s'"), voldef->name); > >> > > This is a workaround for the linked bug, not a fix. Refreshing the pool seems excessive to me. > > Okay, this makes sense. Not only from the POV you are presenting, but I'd expect my pool to be refreshed after I create new volume in it. > > Other volume APIs don't refresh the pool, I think we should be consistent. > > And I think we have a way to eliminate even the little window - just track if we successfully built the volume or not. Something among these lines: > > After properly propagating the error value if the volume exists, we don't need this patch refreshing the pool to work around the original bug. Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list