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.