Re: [libvirt] [PATCHv4 3/7] storage: Implement file storage APIs in the default storage driver
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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)