RetroPie-Setup icon indicating copy to clipboard operation
RetroPie-Setup copied to clipboard

inifuncs.sh: using basic RE to allow meta-chars in keys , eg. +()

Open Gemba opened this issue 4 years ago • 3 comments

As tabled here: https://retropie.org.uk/forum/topic/30714/inifuncs-sh-use-basic-regular-expression-match-in-iniset

Gemba avatar Jun 06 '21 13:06 Gemba

Thanks. I will check if the use of egrep was intentional for some reason or if it was just a leftover from previous code. I'm cautious with this code and have in mind to include some ini tests so we can make sure changes work as intended. Also why https://github.com/RetroPie/RetroPie-Setup/pull/2960 isn't included yet.

Will get back to you on this.

joolswills avatar Jun 08 '21 00:06 joolswills

The use of egrep is a leftover from very early code. I wasn't sure about the details when you said this fails Vs using grep. Can you give an example? I think I understand the issue but want to be sure. Btw are there are actual cases where this fails in actual use with these characters in keys?

We quote some characters for sed and perhaps we should do the same for grep anyway.

The change seems sensible but I'd be keen to have some tests in place before incorporating it.

joolswills avatar Jun 10 '21 03:06 joolswills

I came across this by chance and by setting deliberatly jzintzv+ecs (see #3352) as emulator key (would be handy to visualize it has an extra switch set, I thought), however I learned that dash is the standard atm. NP with that.

It is not broken as it is now upstream, unless some scriptmodule contributors choose to put + and or () in the scriptmodule as emulator key. :wink:

Gemba avatar Jun 10 '21 20:06 Gemba

Revisiting this old ticket as I was doing some ini file tests. I was unable to reproduce the issue (keys with +() in worked with both the egrep and the grep). I can't see an issue with this PR but I would like to reproduce the issue before including the change. Please can you provide an example ini file with a key which iniGet/iniSet are working incorrectly on it without this change.

joolswills avatar Mar 23 '23 17:03 joolswills

Thanks for picking this up. Sorry to be too brief on my initial report. First I thought it was okay too, but then I remembered the backstory.

It can be reproduced like this:

  1. Add deliberately an + as emulator key in a scriptmodule's config_xyz() function. E.g. change the - to a + for the emulator key in this line
  2. Run the sudo ./retropie_packages.sh jzintv configure command once
  3. Notice the jzintv+ecs is added correctly in the /opt/retropie/configs/intellivision/emulators.cfg
  4. Run the sudo ./retropie_packages.sh jzintv configure a second time
  5. Notice that the jzintv+ecs entry in emulators.cfg is added a second time (=unexpected outcome)

Expected outcome: jzintv+ecs is only added once even after multiple ... configure calls.

I see these options as solution:

  1. Use grep instead of egrep as initially suggested in the forum post, or
  2. Do programmatic validation and yell at the scriptmodule designer that using +()| chars is not allowed and stop further processing, or
  3. Least preferred: Add a note in the scriptmodule design tips page that only - is allowed as separator in emulator keywords. Least preferred because there is no programmatic check about whatever is allowed/forbidden as emulator key(word).

Gemba avatar Mar 23 '23 22:03 Gemba

This seems to behave differently on my Desktop PC (With Ubuntu 22.10) vs on Raspbian Buster/Bullseye on the RPI. The issue doesn't occur on my Desktop system which is what I was testing on but I can reproduce it on the Raspberry Pi. Thanks.

joolswills avatar Mar 24 '23 09:03 joolswills