-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FilterX: fix invalid LogMessage variables access #333
base: main
Are you sure you want to change the base?
FilterX: fix invalid LogMessage variables access #333
Conversation
e359d69
to
8bef0ba
Compare
Signed-off-by: Szilard Parrag <[email protected]>
635b6f6
to
3dd8481
Compare
Signed-off-by: Szilard Parrag <[email protected]>
This is to better indicate the intention of synchronizing FilterX -> LogMessage. Signed-off-by: Szilard Parrag <[email protected]>
7dc6233
to
e67d698
Compare
…rection Signed-off-by: Szilard Parrag <[email protected]>
This will be used in _update_repr as that method should update variables with new values regardless of their current value's validity. Signed-off-by: Szilard Parrag <[email protected]>
This is required as _update_repr should update values regardless of their current validity. Signed-off-by: Szilard Parrag <[email protected]>
Signed-off-by: Szilard Parrag <[email protected]>
These type of variables should only be returned if they are in sync with the scope (log_msg_has_changes flag is FALSE). Signed-off-by: Szilard Parrag <[email protected]>
Signed-off-by: Szilard Parrag <[email protected]>
e67d698
to
88b4238
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍🏻
FilterXVariable *v = &g_array_index(self->variables, FilterXVariable, i); | ||
|
||
if (!filterx_variable_is_floating(v)) | ||
g_array_remove_index(self->variables, i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just play with the generation counter to invalidate them? (perf-related question)
|
||
FilterXVariable *variable = NULL; | ||
gboolean success = filterx_scope_lookup_variable_without_validation(scope, self->handle, &variable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
@@ -188,6 +188,8 @@ filterx_scope_validate_variable(FilterXScope *self, FilterXVariable *variable) | |||
if (filterx_variable_handle_is_floating(variable->handle) && | |||
!variable->declared && variable->generation != self->generation) | |||
return FALSE; | |||
if(!filterx_variable_handle_is_floating(variable->handle) && filterx_scope_has_log_msg_changes(self)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd to me.
filterx_scope_has_log_msg_changes()
can only happen if we leave the filterx block, something from the "old world" changes the value, and we reenter the filterx block.
You already invalidated the cache for such scenarios, so this additional check seems unnecessary. Why is it needed, are we sure it is needed?
This PR fixes accessing
LogMessage
type FilterX variables in case there had been changes betweenfilterx
blocks.