On 02/10/14 23:09, Eric Blake wrote:
> On 02/03/2014 09:54 AM, Peter Krempa wrote:
>> Implement the APIs added by the previous patch in the default storage
>> driver used by qemu.
>> ---
>> src/check-aclrules.pl | 1 +
>> src/storage/storage_backend.c | 37 ++++++++++++++++++
>> src/storage/storage_backend.h | 43 +++++++++++++++++++++
>> src/storage/storage_driver.c | 89 +++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 170 insertions(+)
>>
>
>> +static int
>> +storageFileInit(virStorageFilePtr file)
>> +{
>> + virStorageFileBackendPrivPtr priv = NULL;
>> +
>> + if (VIR_ALLOC(priv) < 0)
>> + return -1;
>> +
>> + file->priv = priv;
>> +
>> + if (!(priv->backend = virStorageFileBackendForType(file->type,
>> + file->protocol)))
>> + goto error;
>> +
>> + if (priv->backend->backendInit) {
>> + if (priv->backend->backendInit(file) < 0)
>> + goto error;
>> + }
>> +
>> + return 0;
>
> So if there's no backendInit callback function, we just succeed?
> Shouldn't that error out? Or is backendInit just optional, and the
> important thing is wheterh priv->backend was successfully looked up.
The backendInit function is optional. It's needed for backends that need
to open connections or so. The regular local file backend doesn't do
anything special in this case so it is deliberately left to do nothing.
>
>
>> +static int
>> +storageFileUnlink(virStorageFilePtr file)
>> +{
>> + virStorageFileBackendPrivPtr priv = file->priv;
>> +
>> + if (!priv->backend->storageFileUnlink)
>> + return ENOSYS;
>
> Returning a positive errno value? Don't you want a negative here? Any
> documentation on the fact that these functions are returning errno
> values instead of -1, and with errno itself left indeterminate? Or
> maybe make this 'errno = ENOSYS; return -1;'?
Well the behavior is documented in "libvirt_private.c" in the previous
patch to return positive errnos, but I think that returning -1 and
setting errno to mirror the usual approach from libc is better so I'll
change it.
>
>> +
>> + return priv->backend->storageFileUnlink(file);
>> +}
>> +
>> +
>> +static int
>> +storageFileStat(virStorageFilePtr file,
>> + struct stat *st)
>> +{
>> + virStorageFileBackendPrivPtr priv = file->priv;
>> +
>> + if (!(priv->backend->storageFileStat))
>> + return ENOSYS;
>
> Same issue about failure returns.
>
>> +
>> + return priv->backend->storageFileStat(file, st);
>> +}
>> +
>> +
>> static virStorageDriver storageDriver = {
>> .name = "storage",
>> .storageOpen = storageOpen, /* 0.4.0 */
>> @@ -2631,6 +2711,15 @@ static virStorageDriver storageDriver = {
>>
>> .storagePoolIsActive = storagePoolIsActive, /* 0.7.3 */
>> .storagePoolIsPersistent = storagePoolIsPersistent, /* 0.7.3 */
>> +
>> + /* ----- internal APIs ----- */
>> + .storageFileInit = storageFileInit,
>> + .storageFileDeinit = storageFileDeinit,
>> +
>> + .storageFileUnlink = storageFileUnlink,
>> + .storageFileStat = storageFileStat,
>> + .storageFileCreate = storageFileCreate,
>> +
>> };
>>
>>
>
> I can see where you're going with this, but am not quite sure it is
> ready to merge without addressing my comments above.
>
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list