HPET clocksource is forbidden #82
Conversation
Change validate_clocksources logic to support all the available clocks except hpet. This eliminates the need of arch dependent code and simplifies the validation.
| if [[ "${available_clocksources}" == *"${clocksource}"* ]]; then | ||
| if [[ "${forbidden_clocksources}" != *"${clocksource}"* ]]; then |
There was a problem hiding this comment.
I'm not sure this is good logic here.
The first line is making sure that the current clocksource is one of the available clock sources? That is a given, right? This just seems unnecessary.
The second line allows for partial matches which might not be a good thing at some point:
list="foo bar zorg"
item="mzorg"
if [[ "${list}" != *"${item}"* ]]; then echo match; else echo "no match"; fi
emits this when run: match
There was a problem hiding this comment.
I'm not sure this is good logic here.
The first line is making sure that the current clocksource is one of the available clock sources? That is a given, right? This just seems unnecessary.
The second line allows for partial matches which might not be a good thing at some point:
list="foo bar zorg" item="mzorg" if [[ "${list}" != *"${item}"* ]]; then echo match; else echo "no match"; fiemits this when run:
match
The two lines should check:
if available && !forbidden
But yes, you are right. It is giving false positive on partial matches for the forbidden list. I'll change that logic.
The tests go well until the substring test....
# forbidden="hpet"
# available="tsc foo bar"
# clock="tsc"
# if [[ "${available}" == *"${clock}"* ]]; then echo "ok"; else echo "unavailable"; fi
ok
# if [[ "${forbidden}" != *"${clock}"* ]]; then echo "ok"; else echo "forbidden"; fi
ok
# clock="hpet"
# if [[ "${forbidden}" != *"${clock}"* ]]; then echo "ok"; else echo "forbidden"; fi
forbidden
# clock="new"
# if [[ "${available}" == *"${clock}"* ]]; then echo "ok"; else echo "unavailable"; fi
unavailable
# clock="Xhpet"
# if [[ "${forbidden}" != *"${clock}"* ]]; then echo "ok"; else echo "forbidden"; fi
ok
# if [[ "${available}" == *"${clock}"* ]]; then echo "ok"; else echo "unavailable"; fi
unavailable
The partial match problem (substring): clock=pet and forbidden=hpet
# clock="pet"
# if [[ "${forbidden}" != *"${clock}"* ]]; then echo "ok"; else echo "forbidden"; fi
forbidden
I'll fix this.
| clocksource=$(cat current_clocksource) | ||
| available_clocksources=$(cat available_clocksource) | ||
| supported_clocksources="tsc kvm-clock hyperv_clocksource_tsc_page" | ||
| clocksource="$(cat current_clocksource)" | ||
| available_clocksources="$(cat available_clocksource)" | ||
| forbidden_clocksources="hpet" |
There was a problem hiding this comment.
I'm not sure how I feel about switching from a list of explicitly supported clocksources to a list of explicitly forbidden ones. I suppose that explicity forbidden is easier to maintain, but it also means we could be running with a clocksource that we have not validated to actually be acceptable. Thoughts @atheurer ?
There was a problem hiding this comment.
what is the purpose of checking what is available ?
If we have a hard coded list for supported_clocksources, there is no point in checking available ones in the clocksource dir.
The current code allows kvm and hyperv clocksources and even doesn't check if they are available.
Change validate_clocksources logic to support all the available clocks except hpet. This eliminates the need of arch dependent code and simplifies the validation.