I (and probably others) are experimenting with AI code review and dumentation tools. It is worthwhile to have a baseline reference for coding assistants (such as Claude, Copilot, Cursor, etc.) when working with the iproute2 codebase. This document is a first draft and covers: - Coding style based on Linux kernel guidelines with iproute2-specific exceptions. - JSON output requirements using print_XXX helpers with PRINT_ANY - Command-line argument parsing (strcmp for new code, not matches()) - Kernel compatibility and uapi header update procedures - Patch submission guidelines including DCO requirements This helps ensure AI-generated contributions follow project conventions and reduces review burden from style issues. Changes and revisions are welcome and expected. Signed-off-by: Stephen Hemminger --- AGENTS.md | 690 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 690 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..00349192 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,690 @@ +# AGENTS.md - AI Agent Guidelines for iproute2 + +## Project Overview + +iproute2 is a collection of userspace utilities for controlling and monitoring +networking in the Linux kernel. It includes tools like `ip`, `tc`, `ss`, `bridge`, +and others. The project is tightly coupled with Linux kernel networking development. + +**Important**: iproute2 is userspace code, not kernel code. While the coding style +is similar to the Linux kernel, some kernel-specific conventions do not apply. + +## Repository Information + +- **Stable repository**: `git://git.kernel.org/pub/scm/network/iproute2/iproute2.git` +- **Development repository**: `git://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git` +- **Mailing list**: `netdev@vger.kernel.org` +- **Wiki**: https://wiki.linuxfoundation.org/networking/iproute2 + +## Coding Style + +iproute2 follows Linux kernel coding style with some exceptions specific to +userspace code. Reference: https://www.kernel.org/doc/html/latest/process/coding-style.html + +### Indentation + +- Use tabs for indentation (tabs are 8 characters) +- Do not use spaces for indentation (except in continuation lines) +- Switch and case labels are aligned in the same column: + +```c +switch (suffix) { +case 'G': +case 'g': + mem <<= 30; + break; +case 'M': +case 'm': + mem <<= 20; + break; +default: + break; +} +``` + +### Line Length + +- Preferred limit: 100 columns +- Longer lines are acceptable if breaking them reduces readability +- Never break user-visible strings (e.g., error messages) - they must be grep-able + +### Braces + +**Functions**: Opening brace on a new line: + +```c +int function(int x) +{ + body of function +} +``` + +**Control structures** (if, switch, for, while, do): Opening brace on same line: + +```c +if (x is true) { + we do y +} +``` + +**Single statements**: Do not use braces where a single statement will do: + +```c +if (condition) + action(); +``` + +**Consistency rule**: If one branch needs braces, use braces on all branches: + +```c +if (condition) { + do_this(); + do_that(); +} else { + otherwise(); +} +``` + +### Spaces + +Use a space after these keywords: + + if, switch, case, for, do, while + +Do NOT use a space after: + + sizeof, typeof, alignof, __attribute__ + +Examples: +```c +s = sizeof(struct file); /* correct */ +s = sizeof( struct file ); /* WRONG */ +``` + +Use one space around binary and ternary operators: +```c += + - < > * / % | & ^ <= >= == != ? : +``` + +No space after unary operators: +```c +& * + - ~ ! sizeof typeof alignof __attribute__ +``` + +No space around `.` and `->` structure member operators. + +### Pointer Declarations + +The `*` is adjacent to the variable name, not the type: + +```c +char *linux_banner; /* correct */ +char* linux_banner; /* WRONG */ +unsigned long long memparse(char *ptr, char **retptr); +``` + +### Naming + +- Use lowercase with underscores for names: `count_active_users` +- Do NOT use CamelCase or Hungarian notation +- Global variables and functions need descriptive names +- Local variables should be short: `i` for loop counter, `tmp` for temporary +- Avoid `master/slave` and `blacklist/whitelist` terminology + +### File Headers + +Every source file must start with an SPDX license identifier: + +```c +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * filename.c Brief description + * + * Authors: Name + */ +``` + +### Comments + +Multi-line comments use this format: + +```c +/* + * This is the preferred style for multi-line + * comments in iproute2 source code. + * + * Description: A column of asterisks on the left side, + * with beginning and ending almost-blank lines. + */ +``` + +Comments should explain WHAT the code does, not HOW. Avoid comments inside +function bodies - if needed, the function may be too complex. + +### Variable Declarations + +**Note**: Unlike the Linux kernel, iproute2 does NOT require "Christmas tree" +(reverse Christmas tree / reverse fir tree) style for variable declarations. +Variables do not need to be ordered by decreasing line length. + +Acceptable: +```c +int ret; +struct nlmsghdr *answer; +const char *filter_dev = NULL; +__u32 filt_mask = IFLA_STATS_FILTER_BIT(IFLA_STATS_AF_SPEC); +``` + +Use one declaration per line to allow for comments on each item. + +### Structure Initialization + +Use designated initializers for structures: + +```c +struct iplink_req req = { + .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)), + .n.nlmsg_flags = NLM_F_REQUEST, + .i.ifi_family = preferred_family, +}; +``` + +### Functions + +- Functions should be short and do one thing +- Limit local variables to 5-10 per function +- Separate functions with one blank line +- Use goto for centralized cleanup (see below) + +### Centralized Exit with goto + +Use goto for cleanup when a function has multiple exit points: + +```c +int fun(int a) +{ + int result = 0; + char *buffer; + + buffer = malloc(SIZE); + if (!buffer) + return -ENOMEM; + + if (condition1) { + while (loop1) { + ... + } + result = 1; + goto out_free_buffer; + } + ... +out_free_buffer: + free(buffer); + return result; +} +``` + +Use descriptive label names like `out_free_buffer:` not `err1:`. + +### Macros + +- Macro names defining constants should be CAPITALIZED +- Enums are preferred over #define for related constants +- Prefer inline functions over function-like macros +- Multi-statement macros must use do-while: + +```c +#define MACROFUN(a, b, c) \ + do { \ + if (a == 5) \ + do_this(b, c); \ + } while (0) +``` + +### Error Messages + +Use `fprintf(stderr, ...)` for error messages. This is critical to avoid +corrupting JSON output on stdout: + +```c +fprintf(stderr, "Error: argument of \"%s\" must be \"on\" or \"off\", not \"%s\"\n", + msg, realval); +``` + +Do NOT use `printf()` or `fprintf(stdout, ...)` for error messages. + +## Command-Line Argument Parsing + +### Legacy Code vs New Code + +**Critical distinction for argument parsing:** + +- **Legacy code**: Uses `matches()` function which allows abbreviations +- **New code**: Must use `strcmp()` for exact string comparison only + +The `matches()` function has caused problems with abbreviations in the past. +For all new code, use only full string comparison: + +```c +/* CORRECT - New code should use strcmp() */ +if (strcmp(*argv, "dev") == 0) { + NEXT_ARG(); + /* ... */ +} + +/* LEGACY - matches() allows abbreviations, DO NOT use in new code */ +if (matches(*argv, "broadcast") == 0) { /* matches "b", "br", "bro", etc. */ + /* ... */ +} +``` + +### Argument Processing Macros + +Use the standard macros for argument iteration: + +```c +NEXT_ARG(); /* Move to next argument, exit with error if none */ +NEXT_ARG_OK(); /* Check if next argument exists */ +PREV_ARG(); /* Move back one argument */ +``` + +### Common Patterns + +```c +while (argc > 0) { + if (strcmp(*argv, "dev") == 0) { + NEXT_ARG(); + dev = *argv; + } else if (strcmp(*argv, "mtu") == 0) { + NEXT_ARG(); + if (get_unsigned(&mtu, *argv, 0)) + invarg("Invalid \"mtu\" value\n", *argv); + } else if (strcmp(*argv, "help") == 0) { + usage(); + } else { + fprintf(stderr, "Unknown argument: %s\n", *argv); + exit(-1); + } + argc--; argv++; +} +``` + +## JSON Output Support + +**All commands must support JSON output.** Use the helper functions from +`json_print.h` to ensure consistent output in both regular and JSON modes. + +### Key Principles + +1. **Avoid special-casing JSON** - Let the library handle the differences +2. **Use print_XXX helpers** - Do not use `fprintf(fp, ...)` directly for display output +3. **Error messages to stderr** - Use `fprintf(stderr, ...)` for errors to avoid corrupting JSON +4. **Pair open/close calls** - Every `open_json_object()` needs `close_json_object()`, + every `open_json_array()` needs `close_json_array()` + +### Initializing JSON Context + +```c +#include "json_print.h" + +/* At the start of output */ +new_json_obj(json); /* json is a global flag set by -json option */ + +/* At the end of output */ +delete_json_obj(); +``` + +### Correct Usage Pattern + +**Use `PRINT_ANY` to handle both JSON and text output in one call:** + +```c +/* GOOD - single call handles both modes */ +print_uint(PRINT_ANY, "mtu", "mtu %u ", mtu); +print_string(PRINT_ANY, "name", "%s ", name); + +/* BAD - do not special-case JSON and text separately */ +print_uint(PRINT_JSON, "foo", NULL, bar); +print_uint(PRINT_FP, NULL, "foo %u", bar); +``` + +The correct pattern uses `PRINT_ANY` with both a JSON key and a format string, +letting the library handle which output mode is active. + +### Color Support + +Use color variants for these value types to improve readability: + +- **Interface names** (e.g., "eth0", "enp0s3") +- **MAC addresses** (e.g., "1a:0e:d4:cd:70:81") +- **IPv4 addresses** +- **IPv6 addresses** +- **Operational state values** + +```c +print_color_string(PRINT_ANY, COLOR_IFNAME, "ifname", "%s", ifname); +print_color_string(PRINT_ANY, COLOR_MAC, "address", "%s ", mac_addr); +print_color_string(PRINT_ANY, oper_state_color(state), "operstate", "%s ", state_str); +``` + +### Output Functions + +Use `PRINT_ANY` for output that works in both JSON and text modes: + +```c +/* Simple values */ +print_string(PRINT_ANY, "name", "%s ", name); +print_uint(PRINT_ANY, "mtu", "mtu %u ", mtu); +print_u64(PRINT_ANY, "bytes", "%llu", bytes); +print_bool(PRINT_ANY, "up", "%s", is_up); +print_on_off(PRINT_ANY, "enabled", "%s ", enabled); +``` + +### JSON Objects and Arrays + +Objects and arrays must be properly paired: + +```c +/* Objects - MUST pair open/close */ +open_json_object("linkinfo"); +print_string(PRINT_ANY, "kind", " %s ", kind); +close_json_object(); /* Required! */ + +/* Arrays - MUST pair open/close */ +open_json_array(PRINT_ANY, is_json_context() ? "flags" : "<"); +print_string(PRINT_ANY, NULL, flags ? "%s," : "%s", "UP"); +close_json_array(PRINT_ANY, "> "); /* Required! */ +``` + +### Conditional Output + +For output that must differ between JSON and text modes (use sparingly): + +```c +if (is_json_context()) { + print_uint(PRINT_JSON, "operstate_index", NULL, state); +} else { + print_0xhex(PRINT_FP, NULL, "state %#llx", state); +} +``` + +### Output Type Enum + +```c +enum output_type { + PRINT_FP = 1, /* Text output only */ + PRINT_JSON = 2, /* JSON output only */ + PRINT_ANY = 4, /* Both text and JSON (preferred) */ +}; +``` + +### Available Print Functions + +From `json_print.h`: +- `print_string()`, `print_uint()`, `print_int()`, `print_u64()`, `print_s64()` +- `print_bool()`, `print_on_off()`, `print_null()` +- `print_hex()`, `print_0xhex()` +- `print_float()`, `print_rate()`, `print_tv()` (timeval) +- `print_hu()` (unsigned short), `print_hhu()` (unsigned char) +- `print_luint()` (unsigned long), `print_lluint()` (unsigned long long) +- `print_nl()` - prints newline in non-JSON context only +- Color variants: `print_color_string()`, `print_color_uint()`, etc. + +### Non-JSON Output Format + +The non-JSON (text) output format should be aligned with the command-line +arguments. The output field names should match or closely correspond to +the argument names users provide: + +``` +$ ip link show dev eth0 +2: eth0: mtu 1500 qdisc fq_codel state UP + link/ether 00:11:22:33:44:55 brd ff:ff:ff:ff:ff:ff +``` + +### Human-Readable vs Raw Values + +Commands that output rates, times, or byte counts should: + +- **Plaintext output**: Use existing helpers to format into human-readable terms +- **JSON output**: Print raw numeric values (for script consumption) + +JSON output is intended for scripts and programmatic parsing, so raw values +are more useful than formatted strings. + +Example: +``` +$ tc qdisc show dev enp7s0 +qdisc fq_codel 0: parent :4 limit 10240p flows 1024 quantum 1514 target 5ms + +$ tc -j -p qdisc show dev enp7s0 +[ { + "kind": "fq_codel", + "handle": "0:", + "parent": ":4", + "options": { + "limit": 10240, + "flows": 1024, + "quantum": 1514, + "target": 4999, + "interval": 99999, + "memory_limit": 33554432, + "ecn": true, + "drop_batch": 64 + } + } ] +``` + +Note how `target` shows as `5ms` in plaintext but `4999` (microseconds) in JSON. + +Use helpers like `print_rate()` which automatically handle this distinction. + +## Documentation + +### No Kernel Docbook Format + +iproute2 does **not** use the kernel's docbook documentation format. +Function documentation should use simple C comments: + +```c +/* + * Brief description of what the function does. + * + * Longer description if needed. + * Returns 0 on success, negative on failure. + */ +static int my_function(int argc, char **argv) +``` + +### Usage Functions + +Each command should have a `usage()` function that displays help: + +```c +static void usage(void) __attribute__((noreturn)); + +static void usage(void) +{ + fprintf(stderr, + "Usage: ip address {add|change|replace} IFADDR dev IFNAME\n" + " ip address del IFADDR dev IFNAME\n" + " ip address show [ dev IFNAME ]\n" + ...); + exit(-1); +} +``` + +## Submitting Patches + +### Patch Format + +Patches follow the Linux kernel patch submission guidelines: +https://www.kernel.org/doc/html/latest/process/submitting-patches.html + +### Subject Line + +Use the appropriate prefix based on the target tree: + +``` +Subject: [PATCH iproute2] component: brief description +Subject: [PATCH iproute2-next] component: brief description +``` + +Examples: +``` +Subject: [PATCH iproute2-next] ip: fix syntax for rules +Subject: [PATCH iproute2] tc: fix memory leak in filter parsing +``` + +### Commit Message + +- First line: brief summary (50 chars or less) +- Blank line +- Detailed description wrapped at 72 characters +- Signed-off-by line + +``` +ip: add support for new feature + +Detailed explanation of what this patch does and why. +Reference any relevant kernel commits if this adds support +for new kernel features. + +Signed-off-by: Your Name +``` + +### Signed-off-by and Developer Certificate of Origin + +The `Signed-off-by:` line certifies that you wrote the code or have the right +to submit it, following the Developer's Certificate of Origin (DCO): +https://developercertificate.org/ + +By adding your Signed-off-by, you certify: +- The contribution was created by you, or +- It is based on previous work with a compatible license, or +- It was provided to you by someone who certified the above + +Use `git commit -s` to automatically add your Signed-off-by line. + +### Sending Patches + +Send patches to the netdev mailing list: + +``` +git send-email --to=netdev@vger.kernel.org your-patch.patch +``` + +## Testing + +- Test both JSON and non-JSON output modes +- Test with various kernel versions (features may not be available on older kernels) +- Verify error handling with invalid inputs +- Check for memory leaks with valgrind + +## Common Patterns + +### Netlink Request Structure + +```c +struct { + struct nlmsghdr n; + struct ifaddrmsg ifa; + char buf[256]; +} req = { + .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifaddrmsg)), + .n.nlmsg_flags = NLM_F_REQUEST | flags, + .n.nlmsg_type = cmd, + .ifa.ifa_family = preferred_family, +}; +``` + +### Adding Netlink Attributes + +```c +addattr_l(&req.n, sizeof(req), IFA_LOCAL, &addr.data, addr.bytelen); +addattr32(&req.n, sizeof(req), IFA_RT_PRIORITY, metric); +addattr_nest(&req.n, sizeof(req), IFLA_PROP_LIST | NLA_F_NESTED); +addattr_nest_end(&req.n, proplist); +``` + +### Standard Error Helpers + +```c +invarg("invalid value", *argv); /* Invalid argument value */ +duparg("device", *argv); /* Duplicate argument */ +duparg2("dev", *argv); /* Duplicate argument variant */ +missarg("required argument"); /* Missing required argument */ +nodev(devname); /* Device not found */ +``` + +## Kernel Compatibility + +iproute2 aims to be compatible across a wide range of kernel versions. A newer +version of iproute2 will work with older kernels (though new features may not +be available), and older iproute2 versions work with newer kernels (but cannot +access new features). + +### Sanitized Kernel Headers (uapi) + +The `include/uapi/` directory contains sanitized kernel headers that define +the userspace API for networking. These headers are specific to iproute2 and +allow the tools to be built with support for features that may not yet be +present in the build system's kernel headers. + +These headers are generated from the kernel source tree using: + +``` +make headers_install +``` + +**Important rules for uapi updates:** + +1. **Separate patches** - Updates to `include/uapi/` must be in a separate patch + from the new functionality that uses them + +2. **Upstream first** - Changes to uapi headers will only be accepted once the + corresponding kernel patch has been merged upstream. Do not submit iproute2 + patches that depend on unmerged (or potentially rejected) kernel changes + +3. **Reference kernel commits** - When updating uapi headers, reference the + upstream kernel commit in your patch description + +When adding support for new kernel features: + +1. Wait for the kernel patch to be merged upstream +2. Submit a patch updating the relevant headers in `include/uapi/` +3. Submit a separate patch adding the iproute2 functionality +4. The code should handle older kernels gracefully - new attributes sent to + older kernels may be silently ignored or return an error +5. Test with both old and new kernel versions when possible + +### Runtime Feature Detection + +Since iproute2 may run on kernels older than what it was built against: + +- Check return values from netlink requests - `EOPNOTSUPP` or similar errors + indicate the kernel doesn't support a feature +- Don't assume features exist - the kernel may silently ignore unknown attributes +- Provide helpful error messages when features aren't available + +## Files and Directories + +- `ip/` - ip command and subcommands +- `tc/` - traffic control utilities +- `bridge/` - bridge control utilities +- `misc/` - miscellaneous utilities +- `lib/` - shared library code +- `include/` - header files +- `include/uapi/` - sanitized kernel headers (from `make headers_install`) + +## Security Considerations + +- Validate all user input +- Be careful with buffer sizes +- Check return values from system calls and library functions +- See SECURITY.md for vulnerability reporting procedures + +## License + +iproute2 is licensed under GPL-2.0-or-later. Some files may have dual licensing +(GPL-2.0 OR BSD-2-Clause) - check the SPDX identifier in each file. -- 2.51.0