On 06/28/2010 12:06 PM, Matthias Bolte wrote:
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.
Like I said, this is my first time dealing with a storage driver ;)
Something like I did for the ESX storage driver (and Daniel suggested) should be sufficient here too.
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) {
Are we guaranteed that conn and conn->driver will be non-null by all callers? Then again, I finally looked at esx_storage_driver.c for inspiration, and see that you assumed they are valid.
return VIR_DRV_OPEN_DECLINED; }
return VIR_DRV_OPEN_SUCCESS; }
I'll resubmit v2 accordingly. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org