[libvirt] [PATCH RFC v3 02/15] storage pools: functions refactoring
John Ferlan
jferlan at redhat.com
Wed Dec 7 23:44:10 UTC 2016
On 12/02/2016 10:38 AM, Olga Krishtal wrote:
> After reusage of all possible storage pool structures
> we will able to use some storage pool functions.
>
> Signed-off-by: Olga Krishtal <okrishtal at virtuozzo.com>
> ---
> src/Makefile.am | 2 +-
> src/conf/storage_conf.c | 162 ----------------------------------------------
> src/conf/storage_conf.h | 13 +---
> src/libvirt_private.syms | 11 ++--
> src/util/virpoolcommon.c | 125 +++++++++++++++++++++++++++++++++++
> src/util/virpoolcommon.h | 16 +++++
> src/util/virstoragefile.c | 73 +++++++++++++++++++++
> src/util/virstoragefile.h | 3 +
> 8 files changed, 225 insertions(+), 180 deletions(-)
> create mode 100644 src/util/virpoolcommon.c
>
I couldn't get this one to apply via "git am -3" - the src/Makefile.am
failed... Even removing that specific change, the volpoolcommon.h
changed - so there's something amiss with the patch.... I'll just make
some comments along the way...
I also suggest adding the virpool.c to the previous patch, then using
subsequent patches to remove code from storage_conf. The end result is
the same it's just "easier" to review...
> diff --git a/src/Makefile.am b/src/Makefile.am
> index f8d4a5b..13a4976 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -185,7 +185,7 @@ UTIL_SOURCES = \
> util/viruuid.c util/viruuid.h \
> util/virxdrdefs.h \
> util/virxml.c util/virxml.h \
> - util/virpoolcommon.h \
> + util/virpoolcommon.h util/virpoolcommon.c \
I think the file should be util/virpool.c
> $(NULL)
>
> EXTRA_DIST += $(srcdir)/util/keymaps.csv $(srcdir)/util/virkeycode-mapgen.py
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 7e7bb72..f452fba 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -96,10 +96,6 @@ VIR_ENUM_IMPL(virStoragePartedFs,
> "ext2", "ext2",
> "extended")
>
> -VIR_ENUM_IMPL(virStoragePoolSourceAdapter,
> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST,
> - "default", "scsi_host", "fc_host")
> -
The above stays here - I doubt it'll be used by fspool! It's very specific.
> typedef const char *(*virStorageVolFormatToString)(int format);
> typedef int (*virStorageVolFormatFromString)(const char *format);
>
> @@ -328,73 +324,6 @@ virStorageVolDefFree(virStorageVolDefPtr def)
> VIR_FREE(def);
> }
>
> -static void
> -virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapterPtr adapter)
> -{
> - if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
> - VIR_FREE(adapter->data.fchost.wwnn);
> - VIR_FREE(adapter->data.fchost.wwpn);
> - VIR_FREE(adapter->data.fchost.parent);
> - } else if (adapter->type ==
> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
> - VIR_FREE(adapter->data.scsi_host.name);
> - }
> -}
> -
The above stays
> -void
> -virStoragePoolSourceDeviceClear(virStoragePoolSourceDevicePtr dev)
> -{
> - VIR_FREE(dev->freeExtents);
> - VIR_FREE(dev->path);
> -}
> -
Same
> -void
> -virStoragePoolSourceClear(virStoragePoolSourcePtr source)
> -{
> - size_t i;
> -
> - if (!source)
> - return;
> -
> - for (i = 0; i < source->nhost; i++)
> - VIR_FREE(source->hosts[i].name);
> - VIR_FREE(source->hosts);
> -
> - for (i = 0; i < source->ndevice; i++)
> - virStoragePoolSourceDeviceClear(&source->devices[i]);
> - VIR_FREE(source->devices);
> - VIR_FREE(source->dir);
> - VIR_FREE(source->name);
> - virStoragePoolSourceAdapterClear(&source->adapter);
> - VIR_FREE(source->initiator.iqn);
> - virStorageAuthDefFree(source->auth);
> - VIR_FREE(source->vendor);
> - VIR_FREE(source->product);
> -}
> -
> -void
> -virStoragePoolSourceFree(virStoragePoolSourcePtr source)
> -{
> - virStoragePoolSourceClear(source);
> - VIR_FREE(source);
> -}
> -
> -void
> -virStoragePoolDefFree(virStoragePoolDefPtr def)
> -{
> - if (!def)
> - return;
> -
> - VIR_FREE(def->name);
> -
> - virStoragePoolSourceClear(&def->source);
> -
> - VIR_FREE(def->target.path);
> - VIR_FREE(def->target.perms.label);
> - VIR_FREE(def);
> -}
> -
> -
The above has some common and some specific stuff - it'll need to be
more closely looked at. The whole question of need for a virPoolObj I
raised in 1/15 comes into play...
> void
> virStoragePoolObjFree(virStoragePoolObjPtr obj)
> {
> @@ -730,83 +659,6 @@ virStoragePoolDefParseSourceString(const char *srcSpec,
> return ret;
> }
>
> -static int
> -virStorageDefParsePerms(xmlXPathContextPtr ctxt,
> - virStoragePermsPtr perms,
> - const char *permxpath)
I think this moves to virpool.c and is renamed to be more generic - that
is anything that says "Storage" needs to change...
> -{
> - char *mode;
> - long long val;
> - int ret = -1;
> - xmlNodePtr relnode;
> - xmlNodePtr node;
> -
> - node = virXPathNode(permxpath, ctxt);
> - if (node == NULL) {
> - /* Set default values if there is not <permissions> element */
> - perms->mode = (mode_t) -1;
> - perms->uid = (uid_t) -1;
> - perms->gid = (gid_t) -1;
> - perms->label = NULL;
> - return 0;
> - }
> -
> - relnode = ctxt->node;
> - ctxt->node = node;
> -
> - if ((mode = virXPathString("string(./mode)", ctxt))) {
> - int tmp;
> -
> - if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) {
> - VIR_FREE(mode);
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("malformed octal mode"));
> - goto error;
> - }
> - perms->mode = tmp;
> - VIR_FREE(mode);
> - } else {
> - perms->mode = (mode_t) -1;
> - }
> -
> - if (virXPathNode("./owner", ctxt) == NULL) {
> - perms->uid = (uid_t) -1;
> - } else {
> - /* We previously could output -1, so continue to parse it */
> - if (virXPathLongLong("number(./owner)", ctxt, &val) < 0 ||
> - ((uid_t)val != val &&
> - val != -1)) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("malformed owner element"));
> - goto error;
> - }
> -
> - perms->uid = val;
> - }
> -
> - if (virXPathNode("./group", ctxt) == NULL) {
> - perms->gid = (gid_t) -1;
> - } else {
> - /* We previously could output -1, so continue to parse it */
> - if (virXPathLongLong("number(./group)", ctxt, &val) < 0 ||
> - ((gid_t) val != val &&
> - val != -1)) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("malformed group element"));
> - goto error;
> - }
> - perms->gid = val;
> - }
> -
> - /* NB, we're ignoring missing labels here - they'll simply inherit */
> - perms->label = virXPathString("string(./label)", ctxt);
> -
> - ret = 0;
> - error:
> - ctxt->node = relnode;
> - return ret;
> -}
> -
> static virStoragePoolDefPtr
> virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
> {
> @@ -2125,20 +1977,6 @@ virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool)
> return 0;
> }
>
> -virStoragePoolSourcePtr
> -virStoragePoolSourceListNewSource(virStoragePoolSourceListPtr list)
> -{
> - virStoragePoolSourcePtr source;
> -
> - if (VIR_REALLOC_N(list->sources, list->nsources + 1) < 0)
> - return NULL;
> -
> - source = &list->sources[list->nsources++];
> - memset(source, 0, sizeof(*source));
> -
> - return source;
> -}
> -
I see this moving to virpool
> char *
> virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def)
> {
> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
> index 8a9a789..ad7de86 100644
> --- a/src/conf/storage_conf.h
> +++ b/src/conf/storage_conf.h
> @@ -198,13 +198,8 @@ struct _virStorageDriverState {
> virObjectEventStatePtr storageEventState;
> };
>
> -typedef struct _virStoragePoolSourceList virStoragePoolSourceList;
> +typedef virPoolSourceList virStoragePoolSourceList;
> typedef virStoragePoolSourceList *virStoragePoolSourceListPtr;
> -struct _virStoragePoolSourceList {
> - int type;
> - unsigned int nsources;
> - virPoolSourcePtr sources;
> -};
>
> typedef bool (*virStoragePoolObjListFilter)(virConnectPtr conn,
> virStoragePoolDefPtr def);
> @@ -290,10 +285,6 @@ int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver,
> int virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool);
>
> void virStorageVolDefFree(virStorageVolDefPtr def);
> -void virStoragePoolSourceClear(virStoragePoolSourcePtr source);
> -void virStoragePoolSourceDeviceClear(virStoragePoolSourceDevicePtr dev);
> -void virStoragePoolSourceFree(virStoragePoolSourcePtr source);
> -void virStoragePoolDefFree(virStoragePoolDefPtr def);
These are TBD.
> void virStoragePoolObjFree(virStoragePoolObjPtr pool);
> void virStoragePoolObjListFree(virStoragePoolObjListPtr pools);
> void virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
> @@ -302,8 +293,6 @@ void virStoragePoolObjRemove(virStoragePoolObjListPtr pools,
> virStoragePoolSourcePtr
> virStoragePoolDefParseSourceString(const char *srcSpec,
> int pool_type);
> -virStoragePoolSourcePtr
> -virStoragePoolSourceListNewSource(virStoragePoolSourceListPtr list);
> char *virStoragePoolSourceListFormat(virStoragePoolSourceListPtr def);
>
> int virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 74dd527..a061799 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -867,7 +867,6 @@ virDomainSnapshotUpdateRelations;
> # conf/storage_conf.h
> virStoragePartedFsTypeToString;
> virStoragePoolDefFormat;
> -virStoragePoolDefFree;
> virStoragePoolDefParseFile;
> virStoragePoolDefParseNode;
> virStoragePoolDefParseSourceString;
> @@ -895,13 +894,9 @@ virStoragePoolSaveConfig;
> virStoragePoolSaveState;
> virStoragePoolSourceAdapterTypeFromString;
> virStoragePoolSourceAdapterTypeToString;
> -virStoragePoolSourceClear;
> -virStoragePoolSourceDeviceClear;
> virStoragePoolSourceFindDuplicate;
> virStoragePoolSourceFindDuplicateDevices;
> -virStoragePoolSourceFree;
> virStoragePoolSourceListFormat;
> -virStoragePoolSourceListNewSource;
> virStoragePoolTypeFromString;
> virStoragePoolTypeToString;
> virStorageVolDefFindByKey;
> @@ -2708,6 +2703,12 @@ virXPathULong;
> virXPathULongHex;
> virXPathULongLong;
>
> +# util/virpoolcommon.h
virpool.h
> +virStoragePoolDefFree;
> +virStoragePoolSourceFree;
> +virStoragePoolSourceDeviceClear;
> +virStoragePoolSourceClear;
> +virStoragePoolSourceListNewSource;
Your new names should not be "virStoragePool" - rather just "virPool"
obvious affect in code as well...
>
> # Let emacs know we want case-insensitive sorting
> # Local Variables:
> diff --git a/src/util/virpoolcommon.c b/src/util/virpoolcommon.c
> new file mode 100644
> index 0000000..3ee6251
> --- /dev/null
> +++ b/src/util/virpoolcommon.c
virpool.c
> @@ -0,0 +1,125 @@
> +/*
> + * virpoolcommon.c: utility to operate common parts in storage pools and
> + * filesystem pools
Similar virpool.c - common pool ...
Based on earlier review - some of these API's don't move here.
> + *
> + * 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 "virpoolcommon.h"
> +
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include "viralloc.h"
> +#include "virxml.h"
> +#include "viruuid.h"
> +#include "virerror.h"
> +#include "virlog.h"
> +#include "virfile.h"
> +#include "virendian.h"
> +#include "virstring.h"
> +#include "virutil.h"
> +#include "dirname.h"
> +#include "virbuffer.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_STORAGE
> +
> +VIR_ENUM_IMPL(virStoragePoolSourceAdapter,
I wouldn't expect a "common" function to have "Storage" in the name...
> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST,
> + "default", "scsi_host", "fc_host")
> +
> +static void
> +virStoragePoolSourceAdapterClear(virPoolSourceAdapterPtr adapter)
> +{
> + if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
> + VIR_FREE(adapter->data.fchost.wwnn);
> + VIR_FREE(adapter->data.fchost.wwpn);
> + VIR_FREE(adapter->data.fchost.parent);
> + } else if (adapter->type ==
> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
> + VIR_FREE(adapter->data.scsi_host.name);
> + }
> +}
Above is specific to SCSI pool...
> +
> +void
> +virStoragePoolSourceDeviceClear(virPoolSourceDevicePtr dev)
> +{
> + VIR_FREE(dev->freeExtents);
> + VIR_FREE(dev->path);
> +}
> +
Above is specific to DISK pool
> +void
> +virStoragePoolSourceClear(virPoolSourcePtr source)
> +{
> + size_t i;
> +
> + if (!source)
> + return;
> +
> + for (i = 0; i < source->nhost; i++)
> + VIR_FREE(source->hosts[i].name);
> + VIR_FREE(source->hosts);
> +
> + for (i = 0; i < source->ndevice; i++)
> + virStoragePoolSourceDeviceClear(&source->devices[i]);
> + VIR_FREE(source->devices);
> + VIR_FREE(source->dir);
> + VIR_FREE(source->name);
> + virStoragePoolSourceAdapterClear(&source->adapter);
> + VIR_FREE(source->initiator.iqn);
> + virStorageAuthDefFree(source->auth);
> + VIR_FREE(source->vendor);
> + VIR_FREE(source->product);
> +}
Above would just Clear "virPoolSource" things... leaving the
virStoragePoolSource things for the storage pool code.
> +
> +void
> +virStoragePoolSourceFree(virPoolSourcePtr source)
> +{
> + virStoragePoolSourceClear(source);
> + VIR_FREE(source);
> +}
I think this stays in storage pool
> +
> +void
> +virStoragePoolDefFree(virPoolDefPtr def)
> +{
I don't think we even allocate a virPoolDef - it's just a convenience
structure within a virStoragePool type. Or inconvenience of typing a
few extra characters for structure dereference.
> + if (!def)
> + return;
> +
> + VIR_FREE(def->name);
> +
> + virStoragePoolSourceClear(&def->source);
> +
> + VIR_FREE(def->target.path);
> + VIR_FREE(def->target.perms.label);
> + VIR_FREE(def);
> +}
> +
> +
> +virPoolSourcePtr
> +virStoragePoolSourceListNewSource(virPoolSourceListPtr list)
> +{
> + virPoolSourcePtr source;
> +
> + if (VIR_REALLOC_N(list->sources, list->nsources + 1) < 0)
> + return NULL;
> +
> + source = &list->sources[list->nsources++];
> + memset(source, 0, sizeof(*source));
> +
> + return source;
> +}
Beyond the "virStoragePool" -> "virPool" name change - I suspect the
above changes a bit based on my comments from the first patch.
> diff --git a/src/util/virpoolcommon.h b/src/util/virpoolcommon.h
> index d54de36..c1c607f 100644
> --- a/src/util/virpoolcommon.h
> +++ b/src/util/virpoolcommon.h
> @@ -142,6 +142,15 @@ struct _virPoolSource {
> int format;
> };
>
> +typedef struct _virPoolSourceList virPoolSourceList;
> +typedef virPoolSourceList *virPoolSourceListPtr;
> +struct _virPoolSourceList {
> + int type;
> + unsigned int nsources;
> + virPoolSourcePtr sources;
> +};
> +
> +
The above should have been a part of the original creation of virpool.h
> typedef struct _virPoolTarget virPoolTarget;
> typedef virPoolTarget *virPoolTargetPtr;
> struct _virPoolTarget {
> @@ -163,4 +172,11 @@ struct _virPoolDef {
> virPoolSource source;
> virPoolTarget target;
> };
> +
> +void virStoragePoolSourceClear(virPoolSourcePtr source);
> +void virStoragePoolSourceDeviceClear(virPoolSourceDevicePtr dev);
> +void virStoragePoolSourceFree(virPoolSourcePtr source);
> +void virStoragePoolDefFree(virPoolDefPtr def);
> +virPoolSourcePtr
> +virStoragePoolSourceListNewSource(virPoolSourceListPtr list);
> # endif /* __VIR_POOL_COMMON_H__ */
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 272db67..318b33d 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -3537,3 +3537,76 @@ virStorageFileCheckCompat(const char *compat)
> virStringFreeList(version);
> return ret;
> }
> +
> +int
> +virStorageDefParsePerms(xmlXPathContextPtr ctxt,
> + virStoragePermsPtr perms,
> + const char *permxpath)
I think this should have moved to virpool
> +{
> + char *mode;
> + long long val;
> + int ret = -1;
> + xmlNodePtr relnode;
> + xmlNodePtr node;
> +
> + node = virXPathNode(permxpath, ctxt);
> + if (node == NULL) {
> + perms->mode = (mode_t) -1;
> + perms->uid = (uid_t) -1;
> + perms->gid = (gid_t) -1;
> + perms->label = NULL;
> + return 0;
> + }
> +
> + relnode = ctxt->node;
> + ctxt->node = node;
> +
> + if ((mode = virXPathString("string(./mode)", ctxt))) {
> + int tmp;
> +
> + if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) {
> + VIR_FREE(mode);
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("malformed octal mode"));
> + goto error;
> + }
> + perms->mode = tmp;
> + VIR_FREE(mode);
> + } else {
> + perms->mode = (mode_t) -1;
> + }
> +
> + if (virXPathNode("./owner", ctxt) == NULL) {
> + perms->uid = (uid_t) -1;
> + } else {
> + if (virXPathLongLong("number(./owner)", ctxt, &val) < 0 ||
> + ((uid_t)val != val &&
> + val != -1)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("malformed owner element"));
> + goto error;
> + }
> +
> + perms->uid = val;
> + }
> +
> + if (virXPathNode("./group", ctxt) == NULL) {
> + perms->gid = (gid_t) -1;
> + } else {
> + if (virXPathLongLong("number(./group)", ctxt, &val) < 0 ||
> + ((gid_t) val != val &&
> + val != -1)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("malformed group element"));
> + goto error;
> + }
> + perms->gid = val;
> + }
> +
> + perms->label = virXPathString("string(./label)", ctxt);
> +
> + ret = 0;
> + error:
> + ctxt->node = relnode;
> + return ret;
> +}
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 3d09468..8021d42 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -383,4 +383,7 @@ int virStorageFileCheckCompat(const char *compat);
>
> virStorageSourcePtr virStorageSourceNewFromBackingAbsolute(const char *path);
>
> +int virStorageDefParsePerms(xmlXPathContextPtr ctxt,
> + virStoragePermsPtr perms,
> + const char *permxpath);
Obviously affecting this.
John
> #endif /* __VIR_STORAGE_FILE_H__ */
>
More information about the libvir-list
mailing list