Missing check if the meter has been added then the config file removed #682
Conversation
If a meter has been added and then the config file deleted for any reason, It will be left blank in place. This commit removes these blanks.
| @@ -46,6 +46,16 @@ void Header_populateFromSettings(Header* this) { | |||
| Header_forEachColumn(this, col) { | |||
| const MeterColumnSettings* colSettings = &this->settings->columns[col]; | |||
| for (int i = 0; i < colSettings->len; i++) { | |||
| char* end, dynamic[32]; | |||
| unsigned int param = 0; | |||
| if(String_startsWith(colSettings->names[i], "Dynamic")) { | |||
There was a problem hiding this comment.
Should probably include the opening parens a the name is Dynamic for this meter and there's always a parameter in parens expected. This is actually a generic issue with these meters that take additional config. A better fix is probably to allow the meter creation function to return success or failure and skip the meter on failure …
There was a problem hiding this comment.
A better fix is probably to allow the meter creation function to return success or failure and skip the meter on failure …
I think this is not relevant at this moment ( Header_populateFromSettings function). at this moment meters are populated from htoprc, not from the meter creation function...
Another idea (I don't think it's ideal) to update htoprc by the meter creation function every ./pcp-htop startup, not only every modification
There was a problem hiding this comment.
A better fix is probably to allow the meter creation function to return success or failure and skip the meter on failure …
I think this is not relevant at this moment (
Header_populateFromSettingsfunction). at this moment meters are populated from htoprc, not from the meter creation function...
This is mostly some planning ahead right now: It might only affect the PCP dynamic meters right now, but since some other meters, like the network interface I/O, disk usage, and probably other meters in the future too, will make use of this parameter format, it's likely easier to give the meter a chance to check its parameters and signal any failures. Otherwise we'd be introducing special cases all over the place without keeping those in the context they apply to.
So my suggested solution would be extending the meter interface to signal back invalid parameters/arguments and have routines creating these meters ignore meters that signal any failures. If any failure was signaled upon loading htoprc we flag the config to be saved, thus persisting a clean(ed) copy without the offending meters.
Another idea (I don't think it's ideal) to update htoprc by the meter creation function every ./pcp-htop startup, not only every modification
See above.
| if(String_startsWith(colSettings->names[i], "Dynamic")) { | ||
| char* paren = strchr(colSettings->names[i], '('); | ||
| int ok = sscanf(paren, "(%30s)", dynamic); // DynamicMeter | ||
| if (ok && (end = strrchr(dynamic, ')'))) *end = '\0'; |
| param = ok ? DynamicMeter_search(this->pl, dynamic) : 0; | ||
| if(!param) | ||
| continue; | ||
| } | ||
| Header_addMeterByName(this, colSettings->names[i], col); |
There was a problem hiding this comment.
Good catch and good fix @smalinux - I think though that this way of tackling it ends up duplicating parsing code that's inside Header_addMeterByName() at line 100 already. It'd be simpler to change that line to return a MeterModeId that indicates the error (e.g. LAST) and then make the calling code just skip onto the next loop entry.
I tend to agree this validation needs to happen before the call to Meter_new though (which could separately acquire some validation as @BenBE suggests, but maybe not for this issue - we'll never even reach Meter_new in this case). I'll send through a revised patch.
I see there's another problematic situation, kinda the inverse to this one @smalinux - where we could have duplicate definitions of a DynamicMeter (e.g. in /etc/pcp/htop and ~/.config/htop/htoprc) - we'll need a diagnostic to warn about that I think (and pick one based on some priority ordering). At the moment it causes an error because we attempt to define the same derived metric twice. Anyway, fix incoming.
htoprc that we didn't find during start up. This just leaves blank sections of the display as @sohaib found. Related to htop-dev#682
htoprc that we didn't find during start up. This just leaves blank sections of the display as @smalinux found. Related to htop-dev#682
htoprc that we didn't find during start up. This just leaves blank sections of the display as @smalinux found. Related to htop-dev#682
htoprc that we didn't find during start up. This just leaves blank sections of the display as @smalinux found. Related to htop-dev#682
If a meter has been added and then the config file deleted for any
reason, It will be left blank in place.
This commit removes these blanks.