On Wed, Dec 17, 2008 at 09:00:11PM +0100, Chris Lalancette wrote:
> Cole Robinson wrote:
> > There's a null dereference in the storage driver when defining a pool.
> > Attached patch fixes it for me.
> >
> > diff --git a/src/storage_driver.c b/src/storage_driver.c
> > index 2432a9a..ac5e443 100644
> > --- a/src/storage_driver.c
> > +++ b/src/storage_driver.c
> > @@ -546,7 +546,7 @@ storagePoolDefine(virConnectPtr conn,
> > goto cleanup;
> > def = NULL;
> >
> > - if (virStoragePoolObjSaveDef(conn, driver, pool, def) < 0) {
> > + if (virStoragePoolObjSaveDef(conn, driver, pool, pool->def) < 0) {
> > virStoragePoolObjRemove(&driver->pools, pool);
> > goto cleanup;
> > }
> >
> >
>
> Hm, I definitely see what you are getting at, and why the crash is there, but
> I'm not sure this is totally correct. virStoragePoolObjAssignDef() only assigns
> this def to pool->def iff the storage pool is not running. That means that if
> the pool is running, and we make this change, for running pools you would always
> get the old def, not the updated one. I think we need to move the "def = NULL"
> further down, but that of course will require other changes so we still have the
> unified cleanup target.
Opps, yes you are correct.
IMHO, the API contract here is the problem
Instead of being given a 'virStoragePoolDefPtr' instance, it should be
given the parent 'virStoragePoolObjPtr', along with a flag indicating
whether to save the live or inactive config.
That way the lookup of the correct 'def' is in one place, not in all
callers, and it makes the API contract clear
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list