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(a)redhat.com>
>> ---
>> 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