-
Notifications
You must be signed in to change notification settings - Fork 695
ocp: Fix Parsing of Statistics Snapshot FIFO Event #2777
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
Conversation
545d109 to
44a3d74
Compare
| } else | ||
| } else if ((pevent_descriptor != NULL) && | ||
| (pevent_descriptor->debug_event_class_type == | ||
| STATISTIC_SNAPSHOT_CLASS_TYPE)) { |
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.
You might want to check for event_data_size here, as it's checked above.
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.
There is no event_data_size field in a statistic snapshot event class; it's reserved in that event class. Therefore, it can't be checked in this case.
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 could check stat_data_size instead, which should be the statistic snapshot equivalent. A size of 0 is allowed by the spec, so it'd be good to handle that gracefully here.
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.
Ah, this is already being checked below before parsing and after having confirmed that it is a statistic snapshot. LGTM then.
plugins/ocp/ocp-telemetry-decode.c
Outdated
| description_str); | ||
|
|
||
| struct json_object *pevent_descriptor_obj = | ||
| ((pevent_fifos_object != NULL)?json_create_object():NULL); |
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.
There should probably be spaces here, something like:
| ((pevent_fifos_object != NULL)?json_create_object():NULL); | |
| pevent_fifos_object ? json_create_object() : NULL; |
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.
Added the spaces, but decided to keep the parathesis.
plugins/ocp/ocp-telemetry-decode.h
Outdated
| __u8 stat_info; // Byte 6 | ||
| __u8 namespace_info; // Byte 7 | ||
| __le16 stat_data_size; // Bytes 9:8 | ||
| __le16 reserved2; // Bytes 11:10 |
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 would be more consistent as __u8 reserved2[2];, but it could alternatively just be updated to the OCP 2.6 definition as __le16 nsid.
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.
Updated it to __le16 nsid;
The Statistics Snapshot FIFO Event is in a slightly different format from the other FIFO Events. These changes account for the differences in the event formats. Signed-off-by: jeff-lien-wdc <[email protected]>
3693889 to
7f152c3
Compare
|
squashed both patches together. |
|
Thanks! |
The Statistics Snapshot FIFO Event is in a slightly different format from the other FIFO Events. These changes are required for the correct parsing of the statistics snapshot event.