Re: [libvirt] [PATCH 3/6] storage: Split out virStorageSource accessors to separate file

[John Ferlan <jferlan redhat com>]

On 06/23/2017 09:33 AM, Peter Krempa wrote:
> The helper methods for actually accessing the storage objects don't
> really belong to the main storage driver implementation file. Split them
> out.
> ---
>  po/POTFILES.in                |   1 +
>  src/Makefile.am               |   1 +
>  src/qemu/qemu_domain.c        |   1 +
>  src/qemu/qemu_driver.c        |   1 +
>  src/security/virt-aa-helper.c |   2 +-
>  src/storage/storage_driver.c  | 551 +--------------------------------------
>  src/storage/storage_driver.h  |  28 --
>  src/storage/storage_source.c  | 585 ++++++++++++++++++++++++++++++++++++++++++
>  src/storage/storage_source.h  |  53 ++++
>  tests/virstoragetest.c        |   1 +
>  10 files changed, 645 insertions(+), 579 deletions(-)
>  create mode 100644 src/storage/storage_source.c
>  create mode 100644 src/storage/storage_source.h
> 

Since all of the helpers being moved are prefixed with virStorageFile
why not "storage_file.{c,h}"? I realize the helpers are all operating on
virStorageSourcePtr.  Is it perhaps because being too close to
virstoragefile.{c,h}?

It's not that important, but I suppose I'd expect virStorageSource
helper prefixes in a storage_source.c.

If you do decide to change the name, be sure to adjust the VIR_LOG_INIT
appropriately too. And you could have also adjusted the .h prototypes to
follow the more recently decided upon format that follows the .c files.

As an aside - the remaining functions at the bottom of storage_driver
probably could move to storage_util. Yeah, I know - patches are welcome.

I'd prefer to see storage_file, but again it's not that important...

Reviewed-by: John Ferlan <jferlan redhat com>

John