Re: [libvirt PATCH 13/17] util: extract virStorageFile code into storage_file

On Mon, Dec 14, 2020 at 16:55:33 +0100, Pavel Hrdina wrote: > Up until now we had a runtime code and XML related code in the same > source file inside util directory. > > This patch takes the runtime part and extracts it into the new > storage_file directory. I like this idea but I'm not a fan of the direction of the rename that happens along with it. Specifically rename of virStorageSource* to virStorageFile. All the functions here basically work on a virStorageSource so preferably we keep the name in both places the code for it exists, thus src/conf/storage_source.[ch] and src/storage_source/storage_source.[ch]. I'm aware that it might be confusing at first to have two files which can take functions for a single type, unfortunately the distinction seems necessary, since we have two sources which can create configuration based on virStorageSource. Alternatively when we want to avoid files with same name we can use storage_source_conf.c and storage_source.c

> > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- [...] > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 1bbf567847..43be8dd61a 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1618,6 +1618,43 @@ virSecurityManagerVerify; > virSecurityXATTRNamespaceDefined; Having the name duality then also brings up the question how you picked the functions to move here, since ...

The following ones actually access the storage directly: > +# storage_file/storage_file.h > +virStorageFileAccess; > +virStorageFileCanonicalizePath; > +virStorageFileChown; > +virStorageFileCreate; > +virStorageFileGetBackingStoreStr; > +virStorageFileGetUniqueIdentifier; > +virStorageFileProbeFormat; > +virStorageFileRead; > +virStorageFileStat; > +virStorageFileUnlink; > +virStorageSourceUpdateBackingSizes; > +virStorageSourceUpdateCapacity; > +virStorageSourceUpdatePhysicalSize; These are helper function to the backend access: > +virStorageFileInit; > +virStorageFileInitAs; > +virStorageFileDeinit; > +virStorageFileSupportsAccess; > +virStorageFileSupportsBackingChainTraversal; > +virStorageFileSupportsCreate; > +virStorageFileSupportsSecurityDriver; These access storage indirectly: > +virStorageFileGetMetadata; > +virStorageFileGetMetadataFromBuf; > +virStorageFileGetMetadataFromFD; > +virStorageFileReportBrokenChain; This one is a qemuism and should be rather moved to src/qemu/qemu_block.c or qemu_domain.c > +virStorageSourceFindByNodeName; These just allocate a virStorageSource based on parsing a string: > +virStorageSourceNewFromBacking; > +virStorageSourceNewFromBackingAbsolute; This is almost same as the above (actually it could be replaced by virStorageSourceNewFromBackingAbsolute + a check that the result is _PROTOCOL_RBD) > +virStorageSourceParseRBDColonString; These are just helpers which look through a chain of virStorageSources or are not really accessing anything > +virStorageFileChainLookup; > +virStorageFileGetRelativeBackingPath; > +virStorageFileParseBackingStoreStr; > +virStorageFileParseChainIndex; These are actually private data XML formatter/parser > +virStorageSourcePrivateDataFormatRelPath; > +virStorageSourcePrivateDataParseRelPath;

The naming mismatch comes from the years this code was gradually modified and I'd really prefer if everything unifies on virStorageSource since everything is actually related to virStorageSource. For the backends which do the actual access of storage we could rename it to virStorageSourceBackend.

> diff --git a/src/storage_file/storage_file.c b/src/storage_file/storage_file.c > new file mode 100644 > index 0000000000..6bfeb26233 > --- /dev/null > +++ b/src/storage_file/storage_file.c > @@ -0,0 +1,3845 @@ > +/* > + * storage_file.c: file utility functions for FS storage backend > + * > + * Copyright (C) 2007-2017 Red Hat, Inc. > + * Copyright (C) 2007-2008 Daniel P. Berrange > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>;. > + */ > + > +#include <config.h> > + > +#include <fcntl.h> > +#include <unistd.h> > + > +#include "storage_file.h" > +#include "viralloc.h" > +#include "virendian.h" > +#include "virfile.h" > +#include "virhash.h" > +#include "virjson.h" > +#include "virlog.h" > +#include "virstoragefilebackend.h" > +#include "virstring.h" > +#include "viruri.h" > + > +#define VIR_FROM_THIS VIR_FROM_STORAGE > + > +VIR_LOG_INIT("storage_file"); > + > + > +enum lv_endian { > + LV_LITTLE_ENDIAN = 1, /* 1234 */ > + LV_BIG_ENDIAN /* 4321 */ > +}; > + > +enum { > + BACKING_STORE_OK, > + BACKING_STORE_INVALID, > + BACKING_STORE_ERROR, > +}; > + > +#define FILE_TYPE_VERSIONS_LAST 3 > + > +struct FileEncryptionInfo { > + int format; /* Encryption format to assign */ > + > + int magicOffset; /* Byte offset of the magic */ > + const char *magic; /* Optional string of magic */ > + > + enum lv_endian endian; /* Endianness of file format */ > + > + int versionOffset; /* Byte offset from start of file > + * where we find version number, > + * -1 to always fail the version test, > + * -2 to always pass the version test */ > + int versionSize; /* Size in bytes of version data (0, 2, or 4) */ > + int versionNumbers[FILE_TYPE_VERSIONS_LAST]; > + /* Version numbers to validate. Zeroes are ignored. */ > + > + int modeOffset; /* Byte offset of the format native encryption mode */ > + char modeValue; /* Value expected at offset */ > + > + int payloadOffset; /* start offset of the volume data (in 512 byte sectors) */ > +}; > + > +struct FileTypeInfo { > + int magicOffset; /* Byte offset of the magic */ > + const char *magic; /* Optional string of file magic > + * to check at head of file */ > + enum lv_endian endian; /* Endianness of file format */ > + > + int versionOffset; /* Byte offset from start of file > + * where we find version number, > + * -1 to always fail the version test, > + * -2 to always pass the version test */ > + int versionSize; /* Size in bytes of version data (0, 2, or 4) */ > + int versionNumbers[FILE_TYPE_VERSIONS_LAST]; > + /* Version numbers to validate. Zeroes are ignored. */ > + int sizeOffset; /* Byte offset from start of file > + * where we find capacity info, > + * -1 to use st_size as capacity */ > + int sizeBytes; /* Number of bytes for size field */ > + int sizeMultiplier; /* A scaling factor if size is not in bytes */ > + /* Store a COW base image path (possibly relative), > + * or NULL if there is no COW base image, to RES; > + * return BACKING_STORE_* */ > + const struct FileEncryptionInfo *cryptInfo; /* Encryption info */ > + int (*getBackingStore)(char **res, int *format, > + const char *buf, size_t buf_size); > + int (*getFeatures)(virBitmapPtr *features, int format, > + char *buf, ssize_t len); > +}; > + > + > +static int > +cowGetBackingStore(char **, > + int *, > + const char *, > + size_t); > + > +static int > +qcowXGetBackingStore(char **, int *, > + const char *, > + size_t); > + > +static int > +qcow2GetFeatures(virBitmapPtr *features, > + int format, > + char *buf, > + ssize_t len); > + > +static int > +vmdk4GetBackingStore(char **, > + int *, > + const char *, > + size_t); > + > +static int > +qedGetBackingStore(char **, > + int *, > + const char *, > + size_t); I'd also strongly prefer if this image detection code is stashed into a separate file so that I don't have to see it any time I deal with virStorageSource. I would not worry with a rename except for maybe the exported functions.