Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices
On 01/28/2016 06:30 AM, Pavel Hrdina wrote:
On Wed, Jan 27, 2016 at 03:26:12PM -0500, John Ferlan wrote:
> ...
>>>
>>> However, after some more "investigation" of the environment - I
think
>>> perhaps there's still a couple of loose ends. Keeping the
'segtype'
>>> field may be necessary/useful... Details follow if interested ;-)
>>
>> Yes, you're right and I did a bad job during review. The segtype is
required
>> and we should always consider it, there is like 20 segtypes right now and some
>> of them could also use 'stripes'.
>>
>
> The 'segtype' is currently only required because commit id
'82c1740a'
> added 'segtype' and 'stripes' to resolve a bz (or more than one
> depending on how far you chase) by using 'segtype' to determine whether
> more than one existed. It also did quite a few things in one step which
> by today's review standards is a bad thing ;-).
>
> However, that only worked for "striped" lv's. If there was a
"mirror"
> lv, then while the mirror could be found, the vol-dumpxml output is
> wrong because it only parsed 1 extent (incorrectly at that):
That leads me to conclusion, that we should parse only the segtypes that we know
how to parse. The rest should be ignored.
>
> <source>
> <device path='lv_test_mirror_rimage_0(0),lv_test_mirror_rimage_1'>
> <extent start='0' end='33554432'/>
> </device>
> </source>
>
> instead of something like (if using 'stripes' to get nsegments):
>
> <source>
> <device path='lv_test_mirror_rimage_0'>
> <extent start='0' end='33554432'/>
> </device>
> <device path='lv_test_mirror_rimage_1'>
> <extent start='0' end='33554432'/>
> </device>
> </source>
>
> Linking 'nsegments' to 'striped' lv's is a
"limitation".
>
> Anyway, dropping 'segtype' wasn't necessarily bad unless you needed
> "more information", such as perhaps the lv thin pool source when
> nsegments == 0. This becomes obvious once the change to use "(\\S*)"
> instead of "(\\S+)" hits. Now we can "see" thin lv's in a
volume group.
> Not sure what else becomes visible, though. The problem is you then get:
But we need more information and we propagate those information to user via our
API and that's why I think that we should parse only those volume which we know
how to parse to pass correct values to user.
I understand your point, but what is that list? What happens if/when we
"choose" something that works today?
The issues with the code now:
1. It doesn't recognize a thin lv
2. It doesn't parse a mirror or raid segtype extent correctly
Both are based around using the 'segtype' to make a decision to use the
'stripes' field in order to get the number of segments found. While it
does accurately give the number of devices for striped, mirror, raid,
thin, thin-pool, and linear lv's - it's not documented that way.
So by using 'segtype' to limit we've already caused an issue, so I'm not
sure using that to limit what we parse.
Using it to make a decision about how to parse a specific segtype I
think would be OK.
Secondarily, using 'stripes' as the count of extents is probably wrong
too. The count of segments should probably be made by actively parsing
the devices field and doing the VIR_APPEND_ELEMENT as you originally
suggested each time we parse something that has "string(#)", where
string and (#) are the parsed elements.
Let's see where this takes me ;-)
John
Pavel
>
> <source>
> </source>
>
> But that's fixable... The interesting part about <source> is that it's
> an output only (virStorageVolDefParseXML doesn't read it). So, by adding
> a new parse field 'pool_lv', we can then check 'segtype' to be
"thin"
> and if so, store the new field in a new vol->source.thin_pool field.
>
> Still cannot create a thin lv pool/volume without using the lvcreate
> command. Nor can one create a mirror or stripe lv using libvirt's
> vol-create command. One just cannot "find" a thin lv right now...
>
> John
>