[libvirt] [PATCH] phyp: don't steal storage management from other drivers

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100628/53040b17/attachment-0001.sig>