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.
+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;'?
+ + 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org