[libvirt PATCH 13/17] util: extract virStorageFile code into storage_file
Pavel Hrdina
phrdina at redhat.com
Thu Jan 7 13:17:04 UTC 2021
On Mon, Jan 04, 2021 at 05:43:32PM +0100, Peter Krempa wrote:
> 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
Sounds good, I'll use storage_source_conf and storage_source for the
file names.
> >
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
>
> [...]
>
> > 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 ...
Basically I checked where they are used. Every function called from
src/conf was moved to src/conf/storage_source and the rest to the new
storage_file.
> 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;
Thanks for the explanation for what the functions are used but it
doesn't make it clear to me where they should be placed.
> 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.
Sounds good to match the naming of functions to the structure that it
operates with but I'm afraid that it will make more confusion in the
future if someone tries to add a new functionality into the
virStorageSource code, especially if the conf or storage_file place
should be used.
I guess we should eventually somehow split the structure into two
separate structures.
> > 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.
Sure, sounds good.
Thanks for the review.
Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210107/7fbacb8a/attachment-0001.sig>
More information about the libvir-list
mailing list