Re: [v2] storage: Do not use comma as seperator for lvs output

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

 



On Mon, Oct 10, 2011 at 12:28:04PM +0800, Osier Yang wrote:
> * src/storage/storage_backend_logical.c:
> 
> If a logical vol is created as striped. (e.g. --stripes 3),
> the "device" field of lvs output will have multiple fileds which are
> seperated by comma. Thus the RE we write in the codes will not
> work well anymore. E.g. (lvs output for a stripped vol, uses "#" as
> seperator here):
> 
> test_stripes##fSLSZH-zAS2-yAIb-n4mV-Al9u-HA3V-oo9K1B#\
> /dev/sdc1(10240),/dev/sdd1(0)#42949672960#4194304
> 
> The RE we use:
> 
>     const char *regexes[] = {
>         "^\\s*(\\S+),(\\S*),(\\S+),(\\S+)\\((\\S+)\\),(\\S+),([0-9]+),?\\s*$"
>     };
> 
> Also the RE doesn't match the "devices" field of striped vol properly,
> it contains multiple "device path" and "offset".
> 
> This patch mainly does:
>     1) Change the seperator into "#"
>     2) Change the RE for "devices" field from "(\\S+)\\((\\S+)\\)"
>        into "(\\S+)".
>     3) Add two new options for lvs command, (segtype, stripes)
>     4) Extend the RE to match the value for the two new fields.
>     5) Parse the "devices" field seperately in virStorageBackendLogicalMakeVol,
>        multiple "extents" info are generated if the vol is striped. The
>        number of "extents" is equal to the stripes number of the striped vol.
> 
> A incidental fix: (virStorageBackendLogicalMakeVol)
>     Free "vol" if it's new created and there is error.
> 
> Demo on striped vol with the patch applied:
> 
> % virsh vol-dumpxml /dev/test_vg/vol_striped2
> <volume>
>   <name>vol_striped2</name>
>   <key>QuWqmn-kIkZ-IATt-67rc-OWEP-1PHX-Cl2ICs</key>
>   <source>
>     <device path='/dev/sda5'>
>       <extent start='79691776' end='88080384'/>
>     </device>
>     <device path='/dev/sda6'>
>       <extent start='62914560' end='71303168'/>
>     </device>
>   </source>
>   <capacity>8388608</capacity>
>   <allocation>8388608</allocation>
>   <target>
>     <path>/dev/test_vg/vol_striped2</path>
>     <permissions>
>       <mode>0660</mode>
>       <owner>0</owner>
>       <group>6</group>
>       <label>system_u:object_r:fixed_disk_device_t:s0</label>
>     </permissions>
>   </target>
> </volume>
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=727474
> 
> v1 - v2:
>    v1 just simply changes the seperator into "#".
> 
> ---
>  src/storage/storage_backend_logical.c |  156 +++++++++++++++++++++++++--------
>  1 files changed, 121 insertions(+), 35 deletions(-)
> 
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 589f486..dda68fd 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -62,13 +62,22 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
>  }
>  
>  
> +#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped"
> +
>  static int
>  virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
>                                  char **const groups,
>                                  void *data)
>  {
>      virStorageVolDefPtr vol = NULL;
> +    bool is_new_vol = false;
>      unsigned long long offset, size, length;
> +    const char *regex_unit = "(\\S+)\\((\\S+)\\)";
> +    char *regex = NULL;
> +    regex_t *reg = NULL;
> +    regmatch_t *vars = NULL;
> +    char *p = NULL;
> +    int i, err, nextents, nvars, ret = -1;
>  
>      /* See if we're only looking for a specific volume */
>      if (data != NULL) {
> @@ -88,19 +97,18 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
>              return -1;
>          }
>  
> +        is_new_vol = true;
>          vol->type = VIR_STORAGE_VOL_BLOCK;
>  
>          if ((vol->name = strdup(groups[0])) == NULL) {
>              virReportOOMError();
> -            virStorageVolDefFree(vol);
> -            return -1;
> +            goto cleanup;
>          }
>  
>          if (VIR_REALLOC_N(pool->volumes.objs,
>                            pool->volumes.count + 1)) {
>              virReportOOMError();
> -            virStorageVolDefFree(vol);
> -            return -1;
> +            goto cleanup;
>          }
>          pool->volumes.objs[pool->volumes.count++] = vol;
>      }
> @@ -109,8 +117,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
>          if (virAsprintf(&vol->target.path, "%s/%s",
>                          pool->def->target.path, vol->name) < 0) {
>              virReportOOMError();
> -            virStorageVolDefFree(vol);
> -            return -1;
> +            goto cleanup;
>          }
>      }
>  
> @@ -118,8 +125,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
>          if (virAsprintf(&vol->backingStore.path, "%s/%s",
>                          pool->def->target.path, groups[1]) < 0) {
>              virReportOOMError();
> -            virStorageVolDefFree(vol);
> -            return -1;
> +            goto cleanup;
>          }
>  
>          vol->backingStore.format = VIR_STORAGE_POOL_LOGICAL_LVM2;
> @@ -128,47 +134,127 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
>      if (vol->key == NULL &&
>          (vol->key = strdup(groups[2])) == NULL) {
>          virReportOOMError();
> -        return -1;
> +        goto cleanup;
>      }
>  
>      if (virStorageBackendUpdateVolInfo(vol, 1) < 0)
> -        return -1;
> +        goto cleanup;
>  
> +    nextents = 1;
> +    if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) {
> +        if (virStrToLong_i(groups[5], NULL, 10, &nextents) < 0) {
> +            virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                  _("malformed volume extent stripes value"));
> +            goto cleanup;
> +        }
> +    }
>  
>      /* Finally fill in extents information */
>      if (VIR_REALLOC_N(vol->source.extents,
> -                      vol->source.nextent + 1) < 0) {
> +                      vol->source.nextent + nextents) < 0) {
>          virReportOOMError();
> -        return -1;
> +        goto cleanup;
>      }
>  
> -    if ((vol->source.extents[vol->source.nextent].path =
> -         strdup(groups[3])) == NULL) {
> +    if (virStrToLong_ull(groups[6], NULL, 10, &length) < 0) {
> +        virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> +                              "%s", _("malformed volume extent length value"));
> +        goto cleanup;
> +    }
> +    if (virStrToLong_ull(groups[7], NULL, 10, &size) < 0) {
> +        virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> +                              "%s", _("malformed volume extent size value"));
> +        goto cleanup;
> +    }
> +
> +    /* Now parse the "devices" feild seperately */
> +    regex = strdup(regex_unit);
> +
> +    for (i = 1; i < nextents; i++) {
> +        if (VIR_REALLOC_N(regex, strlen(regex) + strlen(regex_unit) + 2) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +        /* "," is the seperator of "devices" field */
> +        strcat(regex, ",");
> +        strncat(regex, regex_unit, strlen(regex_unit));
> +    }
> +
> +    if (VIR_ALLOC(reg) < 0) {
>          virReportOOMError();
> -        return -1;
> +        goto cleanup;
>      }
>  
> -    if (virStrToLong_ull(groups[4], NULL, 10, &offset) < 0) {
> -        virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> -                              "%s", _("malformed volume extent offset value"));
> -        return -1;
> +    /* Each extent has a "path:offset" pair, and vars[0] will
> +     * be the whole matched string.
> +     */
> +    nvars = (nextents * 2) + 1;
> +    if (VIR_ALLOC_N(vars, nvars) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
>      }
> -    if (virStrToLong_ull(groups[5], NULL, 10, &length) < 0) {
> +
> +    err = regcomp(reg, regex, REG_EXTENDED);
> +    if (err != 0) {
> +        char error[100];
> +        regerror(err, reg, error, sizeof(error));
>          virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> -                              "%s", _("malformed volume extent length value"));
> -        return -1;
> +                              _("Failed to compile regex %s"),
> +                              error);
> +        goto cleanup;
>      }
> -    if (virStrToLong_ull(groups[6], NULL, 10, &size) < 0) {
> -        virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> -                              "%s", _("malformed volume extent size value"));
> -        return -1;
> +
> +    if (regexec(reg, groups[3], nvars, vars, 0) != 0) {
> +        virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                              _("malformed volume extent devices value"));
> +        goto cleanup;
>      }
>  
> -    vol->source.extents[vol->source.nextent].start = offset * size;
> -    vol->source.extents[vol->source.nextent].end = (offset * size) + length;
> -    vol->source.nextent++;
> +    p = groups[3];
>  
> -    return 0;
> +    /* vars[0] is skipped */
> +    for (i = 0; i < nextents; i++) {
> +        int j, len;
> +        const char *offset_str = NULL;
> +
> +        j = (i * 2) + 1;
> +        len = vars[j].rm_eo - vars[j].rm_so;
> +        p[vars[j].rm_eo] = '\0';
> +
> +        if ((vol->source.extents[vol->source.nextent].path =
> +            strndup(p + vars[j].rm_so, len)) == NULL) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +
> +        len = vars[j + 1].rm_eo - vars[j + 1].rm_so;
> +        if (!(offset_str = strndup(p + vars[j + 1].rm_so, len))) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +
> +        if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) {
> +            virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                  _("malformed volume extent offset value"));
> +            goto cleanup;
> +        }
> +
> +        VIR_FREE(offset_str);
> +
> +        vol->source.extents[vol->source.nextent].start = offset * size;
> +        vol->source.extents[vol->source.nextent].end = (offset * size) + length;
> +        vol->source.nextent++;
> +    }
> +
> +    ret = 0;
> +
> +cleanup:
> +    VIR_FREE(regex);
> +    VIR_FREE(reg);
> +    VIR_FREE(vars);
> +    if (is_new_vol && (ret == -1))
> +        virStorageVolDefFree(vol);
> +    return ret;
>  }
>  
>  static int
> @@ -193,15 +279,15 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
>       *    not a suitable separator (rhbz 470693).
>       */
>      const char *regexes[] = {
> -        "^\\s*(\\S+),(\\S*),(\\S+),(\\S+)\\((\\S+)\\),(\\S+),([0-9]+),?\\s*$"
> +       "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#?\\s*$"
>      };
>      int vars[] = {
> -        7
> +        8
>      };
>      const char *prog[] = {
> -        LVS, "--separator", ",", "--noheadings", "--units", "b",
> +        LVS, "--separator", "#", "--noheadings", "--units", "b",
>          "--unbuffered", "--nosuffix", "--options",
> -        "lv_name,origin,uuid,devices,seg_size,vg_extent_size",
> +        "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size",
>          pool->def->source.name, NULL
>      };
>

ACK, based on testing this patch with a couple of vol groups, some with
and some without stripped volumes

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
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]