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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list