Re: [libvirt] [PATCH] phyp: don't steal storage management from other drivers
2010/6/28 Eric Blake <eblake(a)redhat.com>:
Fix regression introduced in commit a4a287242 - basically, the
phyp storage driver should only accept the same URIs that the
main phyp driver is willing to accept. Blindly accepting all
URIs meant that the phyp storage driver was being consulted for
'virsh -c qemu:///session pool-list --all', rather than the
qemu storage driver, then since the URI was not for phyp, attempts
to then use the phyp driver crahsed because it was not initialized.
* src/phyp/phyp_driver.c (phypStorageOpen): Copy phypOpen on which
connections we are willing to accept.
---
> > Looks like the phyp storage driver is being used instead of the libvirtd
> > storage driver (and then segfaulting). Looked briefly for a fix, but it
> > wasn't obvious to me.
> The phypStorageOpen() method is totally bogus. It is ignoring the conn
> object and accepting all connections.
Sorry for not realizing that during my review; this is my first
time dealing with a storage driver patch.
> It must only return VIR_DRV_OPEN_SUCCESS if conn->driver->name ==
VIR_DRV_PHYP
Agreed. I think this patch does the right thing.
The intention is correct, but the implementation is not. Why do you
duplicate the whole phypOpen code in phypStorageOpen?
Besides of being unnecessary this also overwrites the already
initialized private driver data of the virConnectPtr.
Something like I did for the ESX storage driver (and Daniel suggested)
should be sufficient here too.
static virDrvOpenStatus
phypStorageOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags)
{
virCheckFlags(0, VIR_DRV_OPEN_ERROR);
if (STRNEQ(conn->driver->name, "PHYP")) {
return VIR_DRV_OPEN_DECLINED;
}
return VIR_DRV_OPEN_SUCCESS;
}
Probably comparing the conn->driver->no would be even better.
static virDrvOpenStatus
phypStorageOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags)
{
virCheckFlags(0, VIR_DRV_OPEN_ERROR);
if (conn->driver->no != VIR_DRV_PHYP) {
return VIR_DRV_OPEN_DECLINED;
}
return VIR_DRV_OPEN_SUCCESS;
}
When libvirt tries to open the secondary drivers (like a storage
driver) we already have a main hypervisor driver successfully opened.
so we don't need to check the URI again. We just need to make sure
that the PHYP storage driver only accepts an open call iff the open
main driver is the PHYP driver.
Matthias