Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices
On Fri, Jan 22, 2016 at 05:21:06PM -0500, John Ferlan wrote: > From: Joe Harvell > > Since our 'devices' parsing logic now will use the 'nextents' (or > lvs 'stripes' output) to decide whether or not to parse the field, > use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)" > (1 or more) when grabbing the 'groups[3]' or 'devices' field. > > Signed-off-by: John Ferlan > --- > src/storage/storage_backend_logical.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/storage/storage_backend_logical.c > b/src/storage/storage_backend_logical.c > index 3010f58..2af3e69 100644 > --- a/src/storage/storage_backend_logical.c > +++ b/src/storage/storage_backend_logical.c > @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, > *striped, so "," is not a suitable separator either (rhbz 727474). > */ > const char *regexes[] = { > - > "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" > +/* name orig uuid devs stripes segsz vgextsz sz > lvattr */ > + > "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" > }; > int vars[] = { > 9 This could be squashed into the previous commit and if you are creating a comment with labels, please use the exact names that lvs uses. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices
On 01/26/2016 09:55 AM, Pavel Hrdina wrote: > On Fri, Jan 22, 2016 at 05:21:06PM -0500, John Ferlan wrote: >> From: Joe Harvell >> >> Since our 'devices' parsing logic now will use the 'nextents' (or >> lvs 'stripes' output) to decide whether or not to parse the field, >> use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)" >> (1 or more) when grabbing the 'groups[3]' or 'devices' field. >> >> Signed-off-by: John Ferlan >> --- >> src/storage/storage_backend_logical.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/storage/storage_backend_logical.c >> b/src/storage/storage_backend_logical.c >> index 3010f58..2af3e69 100644 >> --- a/src/storage/storage_backend_logical.c >> +++ b/src/storage/storage_backend_logical.c >> @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr >> pool, >> *striped, so "," is not a suitable separator either (rhbz 727474). >> */ >> const char *regexes[] = { >> - >> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" >> +/* name orig uuid devs stripes segsz vgextsz sz >> lvattr */ >> + >> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" >> }; >> int vars[] = { >> 9 > > This could be squashed into the previous commit and if you are creating a > comment with labels, please use the exact names that lvs uses. > I'll just remove the comment - yes it's shorthand, but "vg_extent_size" is a bit long for the "([0-9]+)"... BTW: I went separately mainly to make it more obvious of the change. Patch 3 was just dealing with removing the need for the 'segtype' field as it was made obsolete by just using the stripes field to fill in the 'nextents'. Patch 4 alters the regex to allow 0 or more, rather than at least 1. Although I do understand why combining them is also perfectly reasonable. I was trying not to do two things at one time... 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 ;-) A 'vol-dumpxml' on a "found" thin lv from a thin-pool will provide an empty "" and the capacity == allocation; whereas, a 'vol-dumpxml' on a snapshot lv will provide the "... with the where the size of the extent of course matches the allocation value when seen with vol-info. The of a thin lv is a lv thin-pool and there is no extents data (e.g. nextents == 0). However, there is "pool_lv" data for a "thin" segtype which could then be used for display in the "...". There's still no way to "create" a thin pool from , but finding/displaying one is still possible... The details of which "/dev/sdX" is used is hidden by the lv mgr thin_pool code. We don't display the thin_pool because by itself since it cannot be used as a volume, but that pool contains the information about the real sizes, so it's useful. Right now there's no way to get that useful information so going with the "size" (e.g. allocation) value would have to suffice. Future patches can perhaps adjust this. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices
On Wed, Jan 27, 2016 at 10:05:36AM -0500, John Ferlan wrote: > > > On 01/26/2016 09:55 AM, Pavel Hrdina wrote: > > On Fri, Jan 22, 2016 at 05:21:06PM -0500, John Ferlan wrote: > >> From: Joe Harvell > >> > >> Since our 'devices' parsing logic now will use the 'nextents' (or > >> lvs 'stripes' output) to decide whether or not to parse the field, > >> use the regex of "(\\S*)" (e.g. zero or more) instead of "(\\S+)" > >> (1 or more) when grabbing the 'groups[3]' or 'devices' field. > >> > >> Signed-off-by: John Ferlan > >> --- > >> src/storage/storage_backend_logical.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/storage/storage_backend_logical.c > >> b/src/storage/storage_backend_logical.c > >> index 3010f58..2af3e69 100644 > >> --- a/src/storage/storage_backend_logical.c > >> +++ b/src/storage/storage_backend_logical.c > >> @@ -333,7 +333,8 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr > >> pool, > >> *striped, so "," is not a suitable separator either (rhbz > >> 727474). > >> */ > >> const char *regexes[] = { > >> - > >> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" > >> +/* name orig uuid devs stripes segsz vgextsz > >> sz lvattr */ > >> + > >> "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S*)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$" > >> }; > >> int vars[] = { > >> 9 > > > > This could be squashed into the previous commit and if you are creating a > > comment with labels, please use the exact names that lvs uses. > > > > I'll just remove the comment - yes it's shorthand, but "vg_extent_size" > is a bit long for the "([0-9]+)"... > > BTW: I went separately mainly to make it more obvious of the change. > > Patch 3 was just dealing with removing the need for the 'segtype' field > as it was made obsolete by just using the stripes field to fill in the > 'nextents'. > > Patch 4 alters the regex to allow 0 or more, rather than at least 1. > Although I do understand why combining them is also perfectly > reasonable. I was trying not to do two things at one time... > > 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'. > > A 'vol-dumpxml' on a "found" thin lv from a thin-pool will provide an > empty "" and the capacity == allocation; whereas, a > 'vol-dumpxml' on a snapshot lv will provide the "... path='/dev/sdX'> with the where the size of > the extent of course matches the allocation value when seen with vol-info. > > The of a thin lv is a lv thin-pool and there is no extents data > (e.g. nextents == 0). However, there is "pool_lv" data for a "thin" > segtype which could then be used for display in the > "...". There's still no way to "create" a thin pool from > , but finding/displaying one is still possible... The details of > which "/dev/sdX" is used is hidden by the lv mgr thin_pool code. > > We don't display the thin_pool because by itself since it cannot be used > as a volume, but that pool contains the information about the real > sizes, so it's useful. Right now there's no way to get that useful > information so going with the "size" (e.g. allocation) value would have > to suffice. Future patches can perhaps adjust this. > > John > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices
... >> >> 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): instead of something like (if using 'stripes' to get nsegments): 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 that's fixable... The interesting part about 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] logical: Adjust regex for devices
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. > > > > > > > > instead of something like (if using 'stripes' to get nsegments): > > > > > > > > > > > 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. Pavel > > > > > But that's fixable... The interesting part about 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 > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. > >> >> >> >> >> >> >> >> instead of something like (if using 'stripes' to get nsegments): >> >> >> >> >> >> >> >> >> >> >> 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 > >> >> >> >> >> But that's fixable... The interesting part about 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 >> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list