On Thu, May 18, 2023 at 05:42:03PM +0200, Laszlo Ersek wrote: > On 5/18/23 15:42, Richard W.M. Jones wrote: > > On Thu, May 18, 2023 at 03:09:42PM +0200, Laszlo Ersek wrote: > >> Libguestfs uses the > >> > >> /dev/VG/LV > >> > >> format internally, for identifying LVM logical volumes, but the user might > >> want to specify the > >> > >> /dev/mapper/VG-LV ID > >> > >> format with the "--key ID:..." options. > >> > >> Introduce unescape_device_mapper_lvm() for turning > >> > >> /dev/mapper/VG-LV > >> > >> key IDs into > >> > >> /dev/VG/LV > >> > >> ones, unescaping doubled hyphens to single hyphens in both VG and LV in > >> the process. > >> > >> Call unescape_device_mapper_lvm() from key_store_import_key(). That is, > >> translate the ID as soon as the "--key" option is processed -- let the > >> keystore only know about the usual /dev/VG/LV format, for later matching. > >> > >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2168506 > >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> > >> --- > >> > >> Notes: > >> v2: > >> > >> - rewrite without regular expressions [Rich, Laszlo] > >> > >> - restructure the commit message > >> > >> v1: > >> > >> regcomp() must definitely allocate memory dynamically, and we > >> (intentionally) never free that -- we never call regfree(). I assume > >> valgrind will catch this as a leak, so we might have to extend > >> "valgrind-suppressions" in each dependent superproject. However, I'm > >> unable to run "make check-valgrind" successfully in e.g. virt-v2v even > >> before these patches; see also > >> <https://listman.redhat.com/archives/libguestfs/2023-May/031496.html>. > >> > >> options/keys.c | 100 ++++++++++++++++++++ > >> 1 file changed, 100 insertions(+) > >> > >> diff --git a/options/keys.c b/options/keys.c > >> index bc7459749276..52b27369016a 100644 > >> --- a/options/keys.c > >> +++ b/options/keys.c > >> @@ -260,6 +260,105 @@ key_store_add_from_selector (struct key_store *ks, > >> const char *selector) > >> return key_store_import_key (ks, &key); > >> } > >> > >> +/* Turn /dev/mapper/VG-LV into /dev/VG/LV, in-place. */ > >> +static void > >> +unescape_device_mapper_lvm (char *id) > >> +{ > >> + static const char dev[] = "/dev/", dev_mapper[] = "/dev/mapper/"; > >> + const char *input_start; > >> + char *output; > >> + enum { M_SCAN, M_FILL, M_DONE } mode; > >> + > >> + if (!STRPREFIX (id, dev_mapper)) > >> + return; > >> + > >> + /* Start parsing "VG-LV" from "id" after "/dev/mapper/". */ > >> + input_start = id + (sizeof dev_mapper - 1); > >> + > >> + /* Start writing the unescaped "VG/LV" output after "/dev/". */ > >> + output = id + (sizeof dev - 1); > >> + > >> + for (mode = M_SCAN; mode < M_DONE; ++mode) { > >> + char c; > >> + const char *input = input_start; > >> + const char *hyphen_buffered = NULL; > >> + bool single_hyphen_seen = false; > >> + > >> + do { > >> + c = *input; > >> + > >> + switch (c) { > >> + case '-': > >> + if (hyphen_buffered == NULL) > >> + /* This hyphen may start an escaped hyphen, or it could be the > >> + * separator in VG-LV. > >> + */ > >> + hyphen_buffered = input; > >> + else { > >> + /* This hyphen completes an escaped hyphen; unescape it. */ > >> + if (mode == M_FILL) > >> + *output++ = '-'; > >> + hyphen_buffered = NULL; > >> + } > >> + break; > >> + > >> + case '/': > >> + /* Slash characters are forbidden in VG-LV anywhere. If there's > >> any, > >> + * we'll find it in the first (i.e., scanning) phase, before we > >> output > >> + * anything back to "id". > >> + */ > >> + assert (mode == M_SCAN); > >> + return; > >> + > >> + default: > >> + /* Encountered a non-slash, non-hyphen character -- which also > >> may be > >> + * the terminating NUL. > >> + */ > >> + if (hyphen_buffered != NULL) { > >> + /* The non-hyphen character comes after a buffered hyphen, so > >> the > >> + * buffered hyphen is supposed to be the single hyphen that > >> separates > >> + * VG from LV in VG-LV. There are three requirements for this > >> + * separator: (a) it must be unique (we must not have seen > >> another > >> + * such separator earlier), (b) it must not be at the start of > >> VG-LV > >> + * (because VG would be empty that way), (c) it must not be at > >> the end > >> + * of VG-LV (because LV would be empty that way). Should any of > >> these > >> + * be violated, we'll catch that during the first (i.e., > >> scanning) > >> + * phase, before modifying "id". > >> + */ > >> + if (single_hyphen_seen || hyphen_buffered == input_start || > >> + c == '\0') { > >> + assert (mode == M_SCAN); > >> + return; > >> + } > >> + > >> + /* Translate the separator hyphen to a slash character. */ > >> + if (mode == M_FILL) > >> + *output++ = '/'; > >> + hyphen_buffered = NULL; > >> + single_hyphen_seen = true; > >> + } > >> + > >> + /* Output the non-hyphen character (including the terminating NUL) > >> + * regardless of whether there was a buffered hyphen separator > >> (which, > >> + * by now, we'll have attempted to translate and flush). > >> + */ > >> + if (mode == M_FILL) > >> + *output++ = c; > >> + } > >> + > >> + ++input; > >> + } while (c != '\0'); > >> + > >> + /* We must have seen the VG-LV separator. If that's not the case, > >> we'll > >> + * catch it before modifying "id". > >> + */ > >> + if (!single_hyphen_seen) { > >> + assert (mode == M_SCAN); > >> + return; > >> + } > >> + } > >> +} > > > > So this code can never return an error? > > It surely can; the one error mode is exactly the same as it was in v1 > (the regex version). Namely, the error mode is that the input pathname > in "id" does not match the expected form (i.e., /dev/mapper/VG-LV), and > then we don't modify "id" at all, we let "id" be added to the keystore > exactly as the user specified it. > > The error mode is activated whenever we reach any of the explicit return > statements in the function: > > - when the first STRPREFIX call falls > > - or when any of the other three "return" statements are reached (which > can only be reached during the scanning phase, M_SCAN; that is, *before* > we write anything at all into "id"). > > The point of separating M_SCAN from M_FILL is to verify the full format > match before we start modifying "id" in-place. The traversal (parsing) > through "VG-LV" is exactly the same in both cases, but in the first > pass, we just check. If that completes fine, we repeat the same > traversal, but then we also modify "id" in-place. > > The translation only ever contracts VG-LV (it produces at most as many > bytes as it consumes), and the write point is always to the left of the > read point, so the in-place modification, including the final > NUL-termination, is safe. > > > eg if the input was "A-B-C", > > I think it would translate the string to "A/B-C" which is an output > > but the input seems like it was wrong. > > If "id" contains the string "/dev/mapper/A-B-C", then it *mismatches* > the expected form "/dev/mapper/VG-LV" just as much as an ID containing > "/dev/sda2" would mismatch the expected form. > > In either of those cases, "id" does not get modified, and gets added to > the keystore precisely as the user specified it on the command line. > > We don't say "this ID is wrong, error!". We don't know what the ID is, > we only know what it is *not*. We only say "this is not a > /dev/mapper/VG-LV specification, we just don't know what it is supposed > to be, so we won't touch it, we'll add it as it is, to the keystore". > > That's exactly the behavior (= blind addition) that we have right now, > i.e., pre-patch.
OK got it, in that case: Reviewed-by: Richard W.M. Jones <rjo...@redhat.com> > unescape_device_mapper_lvm() either modifies "id" in place (if "id" on > input has the expected form), or leaves "id" alone. > > The logic following the unescape_device_mapper_lvm() *call site* doesn't > know which way the function went, but it does not *need* to know. > > > Or is it that only the VG is > > escaped? (I don't imagine there is some specification for /dev/mapper > > device names.) > > "/dev/mapper/A-B-C" simply cannot be interpreted according to the > "/dev/mapper/VG-LV" scheme. For such an interpretation to be possible, > we'd have to have one of the following strings in "id": > > - "/dev/mapper/A--B-C" --> "/dev/A-B/C" > - "/dev/mapper/A-B--C" --> "/dev/A/B-C" > > If the user specifies (for example) "/dev/mapper/A-B-C", we reject that, > so the ID remains "/dev/mapper/A-B-C", and that is what gets added to > the keystore -- exactly as it happens pre-patch. > > If the user specifies (for example) "/dev/mapper/A--B--C", we also > reject that (there is no separator to tell us what is the VG and what is > the LV), so the ID remains "/dev/mapper/A--B--C", and that is what gets > added to the keystore -- exactly as it happens pre-patch. > > FWIW, this aspect is identical between version 1 and version 2 of this > patch: in v1, if the regex didn't match, then "id" would simply not be > rewritten, and would get added to the keystore as specified by the user. > The v1->v2 update is an implementation detal, internal to > unescape_device_mapper_lvm() -- it only affects how the expected form is > recognized (regex versus manual parsing), and how a matching input is > translated (the output being constructed from matched regex > subexpressions, versus byte-wise buffering and flushing integrated with > scanning). Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
