Re: [PATCH RFC v3 02/15] storage pools: functions refactoring

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxx>
> ---
>  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__ */
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux