[libvirt] [PATCH 2/4] logical: Create helper virStorageBackendLogicalParseVolDevice

[Pavel Hrdina]

On Wed, Jan 27, 2016 at 09:04:45AM -0500, John Ferlan wrote:
> 
> 
> On 01/26/2016 09:46 AM, Pavel Hrdina wrote:
> > On Fri, Jan 22, 2016 at 05:21:04PM -0500, John Ferlan wrote:
> >> Create a helper routine in order to parse the 'device' string contained
> >> within the generated 'lvs' output string.
> >>
> >> A future patch would then be able to avoid the code more cleanly
> >>
> >> Signed-off-by: John Ferlan <jferlan at redhat.com>
> >> ---
> >>  src/storage/storage_backend_logical.c | 186 +++++++++++++++++++---------------
> >>  1 file changed, 104 insertions(+), 82 deletions(-)
> >>
> >> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> >> index 76ea00a..bf67faf 100644
> >> --- a/src/storage/storage_backend_logical.c
> >> +++ b/src/storage/storage_backend_logical.c
> >> @@ -72,21 +72,115 @@ struct virStorageBackendLogicalPoolVolData {
> >>  };
> >>  
> >>  static int
> >> -virStorageBackendLogicalMakeVol(char **const groups,
> >> -                                void *opaque)
> >> +virStorageBackendLogicalParseVolDevice(virStorageVolDefPtr vol,
> >> +                                       char **const groups,
> >> +                                       int nextents,
> >> +                                       unsigned long long size,
> >> +                                       unsigned long long length)
> >>  {
> > 
> > You've called the new helper *ParseVolDevice, but it actually does more than
> > that.  I would rather see it like ParseExtents and also move all the extents
> > related code into this function (parsing length and size)
> 
> OK - my goal was to just parse the devices/extents element so that the
> next patch could use the 'nextents' to decide whether to call it...  The
> 'nextents' is just the count... Having more than 1 is seen in a
> 'striped' and 'mirror' lv. A 'thin' lv has 0... A 'thin-pool' and
> 'linear' each has 1... Names are found in the 'segtype' field, but it's
> otherwise useless. Perhaps originally the thought when adding a parse of
> that field was that the 'stripes' value only was valid for stripes and
> mirrors, but it seems it's set depending on the count of devices - so it
> seems safe to use it for the extents parsing.
> 
> FWIW: The "extents" look like:
> 
> /dev/hda2(0)     <--- linear
> 
> or
> 
> /dev/sdc1(10240),/dev/sdd1(0)     <--- striped
> 
> or
> 
> thinpool_lv_test_thin_tdata(0)    <--- thin-pool
> 
> 
> The "extents" gets further 'regcomp' and 'regexec''d to grab that "(#)"
> which is part of the black magic that I cannot explain ;-)

Yes, I understand why the next patch updates the way, that we don't assume there
is always 1 extent.  It just seemed to me that parsing length and size would be
nice to move to that function as it's used only in that function.

> 
> > 
> > [...]
> > 
> >> +    /* vars[0] is skipped */
> >> +    for (i = 0; i < nextents; i++) {
> >> +        size_t j;
> >> +        int len;
> >> +        char *offset_str = NULL;
> >> +
> >> +        j = (i * 2) + 1;
> >> +        len = vars[j].rm_eo - vars[j].rm_so;
> >> +        p[vars[j].rm_eo] = '\0';
> >> +
> >> +        if (VIR_STRNDUP(vol->source.extents[vol->source.nextent].path,
> >> +                        p + vars[j].rm_so, len) < 0)
> >> +            goto cleanup;
> >> +
> >> +        len = vars[j + 1].rm_eo - vars[j + 1].rm_so;
> >> +        if (VIR_STRNDUP(offset_str, p + vars[j + 1].rm_so, len) < 0)
> >> +            goto cleanup;
> >> +
> >> +        if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) {
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> +                           _("malformed volume extent offset value"));
> >> +            VIR_FREE(offset_str);
> >> +            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++;
> > 
> > This would be much nicer to be done using VIR_APPEND_ELEMENT().  I know that
> > this commit is just a code movement so this should be done in separate commit.
> > 
> 
> Yes I pretty much blindly moved "just" the devices parsing code, but I
> will note that we're not appending here - it's filling it in. Back in
> the caller "vol->source.extents" is allocated using
> "vol->source.nextents + nextents".  A "new" vol would have
> "source.nextents == 0".  All this loop does is fill in each
> source.extents and bump the source.nextents.

Yes, I know that we are not appending but filling existing array that was
allocated earlier, I just didn't mentioned it.  But we don't need to
pre-allocate the array but just simply use append_element.  The main point was
that the allocation is again outside of the new function and that's not a good
practice.

> In any case, I will move the allocation of source.extents, fetch of
> length and size into the helper (and rename it).