[libvirt] [PATCH 3/7] conf: Add/modify storage formatting functions

John Ferlan jferlan at redhat.com
Wed Mar 25 16:21:29 UTC 2015



On 03/24/2015 06:06 AM, Erik Skultety wrote:
> This patch introduces virStoragePoolSaveStatus to properly format the
> status XML. It also adds virStoragePoolDefFormatBuf function, which
> alike in the network driver, formats the whole storage pool definition into
> a buffer that might have been previously modified by
> virStoragePoolSaveStatus to insert <poolstatus> element. The original
> virStoragePoolDefFormat function had to be modified accordingly to use
> virBuffer.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
> ---
>  src/conf/storage_conf.c  | 139 ++++++++++++++++++++++++++++++++++-------------
>  src/conf/storage_conf.h  |   6 +-
>  src/libvirt_private.syms |   1 +
>  3 files changed, 107 insertions(+), 39 deletions(-)
> 

In a bit of bikeshedding - this patch does a couple of things and could
be split into a couple of patches...  Ironically you bundled things
together here, but separated them for the stateDir changes (patches 2,
4, & 7).

first one just creates DefFormatBuf and has DefFormat call it

second one creates the virStoragePoolSaveXML, has the config code use it

third one creates virStoragePoolSaveStatus which use the new API's

> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index b070448..5d984f3 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1150,76 +1150,90 @@ virStoragePoolSourceFormat(virBufferPtr buf,
>  }
>  
>  
> -char *
> -virStoragePoolDefFormat(virStoragePoolDefPtr def)
> +int

static int - only this function cares; otherwise we have to ensure the
'buf' and 'def' arguments aren't NULL in the .h file... This way we
control having non NULL values being presented.

> +virStoragePoolDefFormatBuf(virBufferPtr buf,
> +                           virStoragePoolDefPtr def)
>  {
>      virStoragePoolOptionsPtr options;
> -    virBuffer buf = VIR_BUFFER_INITIALIZER;
> -    const char *type;
>      char uuid[VIR_UUID_STRING_BUFLEN];
> +    const char *type;
>  
>      options = virStoragePoolOptionsForPoolType(def->type);
>      if (options == NULL)
> -        return NULL;
> +        goto error;
>  
>      type = virStoragePoolTypeToString(def->type);
>      if (!type) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("unexpected pool type"));
> -        goto cleanup;
> +        goto error;
>      }
> -    virBufferAsprintf(&buf, "<pool type='%s'>\n", type);
> -    virBufferAdjustIndent(&buf, 2);
> -    virBufferEscapeString(&buf, "<name>%s</name>\n", def->name);
> +    virBufferAsprintf(buf, "<pool type='%s'>\n", type);
> +    virBufferAdjustIndent(buf, 2);
> +    virBufferEscapeString(buf, "<name>%s</name>\n", def->name);
>  
>      virUUIDFormat(def->uuid, uuid);
> -    virBufferAsprintf(&buf, "<uuid>%s</uuid>\n", uuid);
> +    virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuid);
>  
> -    virBufferAsprintf(&buf, "<capacity unit='bytes'>%llu</capacity>\n",
> +    virBufferAsprintf(buf, "<capacity unit='bytes'>%llu</capacity>\n",
>                        def->capacity);
> -    virBufferAsprintf(&buf, "<allocation unit='bytes'>%llu</allocation>\n",
> +    virBufferAsprintf(buf, "<allocation unit='bytes'>%llu</allocation>\n",
>                        def->allocation);
> -    virBufferAsprintf(&buf, "<available unit='bytes'>%llu</available>\n",
> +    virBufferAsprintf(buf, "<available unit='bytes'>%llu</available>\n",
>                        def->available);
>  
> -    if (virStoragePoolSourceFormat(&buf, options, &def->source) < 0)
> -        goto cleanup;
> +    if (virStoragePoolSourceFormat(buf, options, &def->source) < 0)
> +        goto error;
>  
>      /* RBD, Sheepdog, and Gluster devices are not local block devs nor
>       * files, so they don't have a target */
>      if (def->type != VIR_STORAGE_POOL_RBD &&
>          def->type != VIR_STORAGE_POOL_SHEEPDOG &&
>          def->type != VIR_STORAGE_POOL_GLUSTER) {
> -        virBufferAddLit(&buf, "<target>\n");
> -        virBufferAdjustIndent(&buf, 2);
> +        virBufferAddLit(buf, "<target>\n");
> +        virBufferAdjustIndent(buf, 2);
>  
> -        virBufferEscapeString(&buf, "<path>%s</path>\n", def->target.path);
> +        virBufferEscapeString(buf, "<path>%s</path>\n", def->target.path);
>  
> -        virBufferAddLit(&buf, "<permissions>\n");
> -        virBufferAdjustIndent(&buf, 2);
> -        virBufferAsprintf(&buf, "<mode>0%o</mode>\n",
> +        virBufferAddLit(buf, "<permissions>\n");
> +        virBufferAdjustIndent(buf, 2);
> +        virBufferAsprintf(buf, "<mode>0%o</mode>\n",
>                            def->target.perms.mode);
> -        virBufferAsprintf(&buf, "<owner>%d</owner>\n",
> +        virBufferAsprintf(buf, "<owner>%d</owner>\n",
>                            (int) def->target.perms.uid);
> -        virBufferAsprintf(&buf, "<group>%d</group>\n",
> +        virBufferAsprintf(buf, "<group>%d</group>\n",
>                            (int) def->target.perms.gid);
> -        virBufferEscapeString(&buf, "<label>%s</label>\n",
> +        virBufferEscapeString(buf, "<label>%s</label>\n",
>                                def->target.perms.label);
>  
> -        virBufferAdjustIndent(&buf, -2);
> -        virBufferAddLit(&buf, "</permissions>\n");
> -        virBufferAdjustIndent(&buf, -2);
> -        virBufferAddLit(&buf, "</target>\n");
> +        virBufferAdjustIndent(buf, -2);
> +        virBufferAddLit(buf, "</permissions>\n");
> +        virBufferAdjustIndent(buf, -2);
> +        virBufferAddLit(buf, "</target>\n");
>      }
> -    virBufferAdjustIndent(&buf, -2);
> -    virBufferAddLit(&buf, "</pool>\n");
> +    virBufferAdjustIndent(buf, -2);
> +    virBufferAddLit(buf, "</pool>\n");
> +
> +    return 0;
> +
> + error:
> +    return -1;
> +}
> +
> +char *
> +virStoragePoolDefFormat(virStoragePoolDefPtr def)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    if (virStoragePoolDefFormatBuf(&buf, def) < 0)
> +        goto error;
>  
>      if (virBufferCheckError(&buf) < 0)
> -        goto cleanup;
> +        goto error;
>  
>      return virBufferContentAndReset(&buf);
>  
> - cleanup:
> + error:
>      virBufferFreeAndReset(&buf);
>      return NULL;
>  }
> @@ -1899,11 +1913,60 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools,
>      return ret;
>  }
>  
> +
> +static int virStoragePoolSaveXML(const char *configFile,
> +                                 virStoragePoolDefPtr def,
> +                                 const char *xml)

static int
virStoragePoolSaveXML(const char *path,

(since it won't be the "configFile" only...)

> +{
> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +    int ret = -1;
> +
> +    virUUIDFormat(def->uuid, uuidstr);
> +    ret = virXMLSaveFile(configFile,

s/configFile/path

> +                         virXMLPickShellSafeComment(def->name, uuidstr),
> +                         "pool-edit", xml);
> +
> +    return ret;
> +}
> +
> +
> +int virStoragePoolSaveStatus(const char *stateFile,
> +                             virStoragePoolDefPtr def)

int
virStoragePoolSaveStatus(...

> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    int ret = -1;
> +    char *xml;
> +
> +    virBufferAddLit(&buf, "<poolstatus>\n");

poolstate ?

> +    virBufferAdjustIndent(&buf, 2);
> +
> +    if (virStoragePoolDefFormatBuf(&buf, def) < 0)
> +        goto error;
> +
> +    virBufferAdjustIndent(&buf, -2);
> +    virBufferAddLit(&buf, "</poolstatus>\n");

poolstate ?

> +
> +    if (virBufferCheckError(&buf) < 0)
> +        goto error;
> +
> +    if (!(xml = virBufferContentAndReset(&buf)))
> +        goto error;
> +
> +    if (virStoragePoolSaveXML(stateFile, def, xml))
> +        goto error;
> +
> +    ret = 0;
> +
> + error:
> +    VIR_FREE(xml);
> +    return ret;
> +}
> +
> +
>  int
>  virStoragePoolSaveConfig(const char *configFile,
>                           virStoragePoolDefPtr def)
>  {
> -    char uuidstr[VIR_UUID_STRING_BUFLEN];
>      char *xml;
>      int ret = -1;
>  
> @@ -1913,12 +1976,12 @@ virStoragePoolSaveConfig(const char *configFile,
>          return -1;
>      }
>  
> -    virUUIDFormat(def->uuid, uuidstr);
> -    ret = virXMLSaveFile(configFile,
> -                         virXMLPickShellSafeComment(def->name, uuidstr),
> -                         "pool-edit", xml);
> -    VIR_FREE(xml);
> +    if (virStoragePoolSaveXML(configFile, def, xml))
> +        goto cleanup;
>  
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(xml);
>      return ret;
>  }
>  
> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
> index 8ccc947..99b2f4a 100644
> --- a/src/conf/storage_conf.h
> +++ b/src/conf/storage_conf.h
> @@ -345,6 +345,8 @@ virStoragePoolDefPtr virStoragePoolDefParseFile(const char *filename);
>  virStoragePoolDefPtr virStoragePoolDefParseNode(xmlDocPtr xml,
>                                                  xmlNodePtr root);
>  char *virStoragePoolDefFormat(virStoragePoolDefPtr def);
> +int virStoragePoolDefFormatBuf(virBufferPtr buf,
> +                               virStoragePoolDefPtr def);

Unnecessary if static int

Obvious if this gets split into 3 patches, then there's a few changes to
the following stuff changes too

John
>  
>  typedef enum {
>      /* do not require volume capacity at all */
> @@ -372,7 +374,9 @@ virStoragePoolObjPtr
>  virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
>                             virStoragePoolDefPtr def);
>  
> -int virStoragePoolSaveConfig(const char *configDir,
> +int virStoragePoolSaveStatus(const char *stateFile,
> +                             virStoragePoolDefPtr def);
> +int virStoragePoolSaveConfig(const char *configFile,
>                               virStoragePoolDefPtr def);
>  int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver,
>                               virStoragePoolObjPtr pool,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 33222f0..689a08f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -811,6 +811,7 @@ virStoragePoolObjRemove;
>  virStoragePoolObjSaveDef;
>  virStoragePoolObjUnlock;
>  virStoragePoolSaveConfig;
> +virStoragePoolSaveStatus;
>  virStoragePoolSourceAdapterTypeFromString;
>  virStoragePoolSourceAdapterTypeToString;
>  virStoragePoolSourceClear;
> 




More information about the libvir-list mailing list