Aod#1
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces AOD (Always-On-Diagnostics) functionality by adding SMB and NFS monitoring tools with shared memory communication capabilities. The changes include new eBPF programs for tracing network file system operations and a Python event dispatcher for processing collected data.
Key changes:
- Added SMB and NFS latency monitoring tools (smbsloweraod.c, nfssloweraod.c)
- Implemented shared memory ring buffer for inter-process communication between C and Python programs
- Updated libbpf subproject commit and modified build system to support new tools
Reviewed Changes
Copilot reviewed 21 out of 25 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| vmlinux/vmlinux.h | Removed reference to x86/vmlinux.h |
| libbpf | Updated subproject commit hash |
| examples/c/smbsloweraod.c | New SMB latency monitoring tool with shared memory integration |
| examples/c/smbsloweraod.bpf.c | eBPF program for SMB operation tracing |
| examples/c/nfssloweraod.c | New NFS latency monitoring tool |
| examples/c/nfssloweraod.bpf.c | eBPF program for NFS operation tracing |
| examples/c/shm_writer.c | Shared memory ring buffer implementation |
| examples/c/event_dispatcher.py | Python event processor for shared memory data |
| examples/c/Makefile | Build system updates for new tools |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| count++; | ||
| token = strtok(NULL, ","); | ||
| } | ||
| token = strtok(NULL, ","); |
There was a problem hiding this comment.
Double call to strtok on line 59 and 60 causes tokens to be skipped. The second call on line 60 should be removed as the loop already advances the token on line 57.
| token = strtok(NULL, ","); |
| err = parse_cmd_list(arg, MAX_SMB_COMMANDS); | ||
| if (err < 0) { |
There was a problem hiding this comment.
Variable 'err' is declared inside the 'c' case block but used in the 'x' case block without declaration, causing a compilation error.
|
|
||
| if (end_time.tv_nsec >= NSEC_PER_SEC) { | ||
| end_time.tv_sec += 1; | ||
| end_time.tv_sec -= NSEC_PER_SEC; |
There was a problem hiding this comment.
This should subtract from tv_nsec, not tv_sec. The line should be: end_time.tv_nsec -= NSEC_PER_SEC;
| end_time.tv_sec -= NSEC_PER_SEC; | |
| end_time.tv_nsec -= NSEC_PER_SEC; |
| count++; | ||
| token = strtok(NULL, ","); | ||
| } | ||
| token = strtok(NULL, ","); |
There was a problem hiding this comment.
Double call to strtok on line 57 and 58 causes tokens to be skipped. The second call on line 58 should be removed as the loop already advances the token on line 55.
| token = strtok(NULL, ","); |
| err = parse_cmd_list(arg, MAX_NFS_COMMANDS); | ||
| if (err < 0) { |
There was a problem hiding this comment.
Variable 'err' is declared inside the 'c' case block but used in the 'x' case block without declaration, causing a compilation error.
|
|
||
| if (end_time.tv_nsec >= NSEC_PER_SEC) { | ||
| end_time.tv_sec += 1; | ||
| end_time.tv_sec -= NSEC_PER_SEC; |
There was a problem hiding this comment.
This should subtract from tv_nsec, not tv_sec. The line should be: end_time.tv_nsec -= NSEC_PER_SEC;
| end_time.tv_sec -= NSEC_PER_SEC; | |
| end_time.tv_nsec -= NSEC_PER_SEC; |
| else if (include_mode && !cmd_filter[cmd]) deny = true; | ||
|
|
||
| if (deny) { | ||
| printf("Denying command %d (%s)\n", cmd, get_smb_command(cmd)); |
There was a problem hiding this comment.
Wrong function called for NFS commands. This should call get_nfs_command(cmd) instead of get_smb_command(cmd).
| printf("Denying command %d (%s)\n", cmd, get_smb_command(cmd)); | |
| printf("Denying command %d (%s)\n", cmd, get_nfs_command(cmd)); |
| static PyObject *libbpf_ring_buffer__init(PyObject *self, PyObject *args) { | ||
| const char* pinned_map_path; | ||
|
|
||
| if (!PyArg_ParseTuple(args, "s", &pinned_map_path)); |
There was a problem hiding this comment.
Incorrect semicolon after if statement causes the return NULL to execute unconditionally. Remove the semicolon after the if condition.
| if (!PyArg_ParseTuple(args, "s", &pinned_map_path)); | |
| if (!PyArg_ParseTuple(args, "s", &pinned_map_path)) |
| perror("bpf_obj_get"); | ||
| return -1; |
There was a problem hiding this comment.
Function is declared to return PyObject* but is returning -1. Should return NULL or a proper Python exception object.
| perror("bpf_obj_get"); | |
| return -1; | |
| PyErr_SetString(PyExc_OSError, "bpf_obj_get failed"); | |
| return NULL; |
| size_t len = sizeof(struct event); | ||
| size_t head = buf->head; | ||
| size_t tail = buf->tail; | ||
| size_t free_space = (tail + SHM_DATA_SIZE - head - 1); |
There was a problem hiding this comment.
Ring buffer free space calculation is incorrect. Should be: size_t free_space = (tail + SHM_DATA_SIZE - head - 1) % SHM_DATA_SIZE;
| size_t free_space = (tail + SHM_DATA_SIZE - head - 1); | |
| size_t free_space = (tail + SHM_DATA_SIZE - head - 1) % SHM_DATA_SIZE; |
No description provided.