Skip to content

Commit f55f187

Browse files
author
Martin Belanger
committed
nvmf: replace nvmf_get_discovery_log() with opaque-args API
Introduce struct nvmf_discovery_args as an ABI-stable opaque object with auto-generated setter/getter accessors (nvmf-accessors.{h,c,ld}). The mandatory controller argument moves to the function signature; optional parameters (timeout, max_retries, lsp) are set via accessors. Remove nvmf_get_discovery_wargs() entirely. Split the accessor generator output into a common NVMe triplet (accessors.{h,c,ld}) and an NVMe-oF-specific triplet (nvmf-accessors.{h,c,ld}) so that PCIe/embedded builds can exclude all fabrics code. The generator script is made generic; two meson targets (update-common-accessors, update-fabrics-accessors) are exposed under the update-accessors alias. Signed-off-by: Martin Belanger <[email protected]>
1 parent eb36af7 commit f55f187

16 files changed

Lines changed: 506 additions & 244 deletions

README.md

Lines changed: 11 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -303,90 +303,24 @@ ENTRY, then append the object file name to the meson.build "sources".
303303

304304
### Updating the libnvme accessor functions
305305

306-
libnvme exposes a set of getter and setter functions (accessors) for its core
307-
internal structs (`libnvme_path`, `libnvme_ns`, `libnvme_ctrl`, `libnvme_subsystem`,
308-
`libnvme_host`, `libnvme_fabric_options`). These are generated from the struct
309-
definitions in `libnvme/src/nvme/private.h` by the tool
310-
`libnvme/src/nvme/generate-accessors.c`.
306+
libnvme exposes auto-generated getter/setter accessor functions for its
307+
ABI-stable opaque structs. Two sets of accessors are maintained:
311308

312-
The generated files are committed to the source tree and are **not**
313-
regenerated during a normal build:
309+
| Set | Input header | Generated files |
310+
|-----|-------------|-----------------|
311+
| Common NVMe | `libnvme/src/nvme/private.h` | `src/nvme/accessors.{h,c}`, `src/accessors.ld` |
312+
| NVMe-oF | `libnvme/src/nvme/private-fabrics.h` | `src/nvme/nvmf-accessors.{h,c}`, `src/nvmf-accessors.ld` |
314313

315-
```
316-
libnvme/src/nvme/accessors.h # public API declarations (with Doxygen stubs)
317-
libnvme/src/nvme/accessors.c # implementations
318-
libnvme/src/nvme/accessors.ld # linker version script (manually maintained)
319-
```
320-
321-
#### When to regenerate
322-
323-
Regeneration is needed when a struct member is added, removed, or renamed in
324-
`private.h`, or when a struct is added to or removed from
325-
`generate-accessors-include.list` or excluded in `generate-accessors-exclude.list`.
326-
327-
#### How to regenerate
328-
329-
```shell
330-
$ make update-accessors
331-
```
332-
333-
or equivalently:
314+
The generated `.h` and `.c` files are committed to the source tree and are
315+
**not** regenerated during a normal build. To regenerate after modifying a
316+
`/*!generate-accessors*/` struct:
334317

335318
```shell
336319
$ meson compile -C .build update-accessors
337320
```
338321

339-
The script compiles the generator, runs it, and atomically updates
340-
`accessors.h` and `accessors.c` only when their content changes.
341-
Commit the updated files afterward:
342-
343-
```shell
344-
$ git add libnvme/src/nvme/accessors.h libnvme/src/nvme/accessors.c
345-
$ git commit -m "libnvme: regenerate accessors following <struct> changes"
346-
```
347-
348-
#### Maintaining accessors.ld
349-
350-
`accessors.ld` is a GNU linker version script that controls which accessor
351-
symbols are exported from `libnvme.so` and under which ABI version label they
352-
were introduced (e.g. `LIBNVME_ACCESSORS_3`).
353-
354-
This file is **not** updated automatically, because each symbol must be placed
355-
in the correct version section by the maintainer. Adding a symbol to an
356-
already-published version section would break binary compatibility for
357-
existing users of the library.
358-
359-
When `make update-accessors` detects that the symbol list has drifted from
360-
`accessors.ld`, it prints a report like the following:
361-
362-
```
363-
WARNING: accessors.ld needs manual attention.
364-
365-
Symbols to ADD (place in a new version section, e.g. LIBNVME_ACCESSORS_X_Y):
366-
libnvme_ctrl_get_new_field
367-
libnvme_ctrl_set_new_field
368-
```
369-
370-
New symbols must be added to a **new** version section that chains the
371-
previous one. For example, if the current latest section is
372-
`LIBNVME_ACCESSORS_3_0`, add a new section for the next release:
373-
374-
```
375-
LIBNVME_ACCESSORS_3_1 {
376-
global:
377-
libnvme_ctrl_get_new_field;
378-
libnvme_ctrl_set_new_field;
379-
} LIBNVME_ACCESSORS_3_0;
380-
```
381-
382-
Then commit `accessors.ld` together with the regenerated source files.
383-
384-
#### CI enforcement
385-
386-
A GitHub Actions workflow (`.github/workflows/check-accessors.yml`) runs on
387-
every push and pull request. It regenerates the accessor files and fails if
388-
the result differs from what is committed, ensuring the source tree never
389-
drifts silently.
322+
See [`libnvme/README.md`](libnvme/README.md#accessor-generation) for full
323+
details, including how to maintain the `.ld` version-script files.
390324

391325
## Dependency
392326

libnvme/README.md

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,3 +186,85 @@ meson setup .build -Db_sanitize=address && LD_PRELOAD=/lib64/libasan.so.6 ninja
186186
```
187187

188188
It's also possible to enable the undefined behavior sanitizer with `-Db_sanitize=undefined`. To enable both, use `-Db_sanitize=address,undefined`.
189+
190+
## Accessor generation
191+
192+
Some public structs in libnvme use auto-generated setter/getter accessor
193+
functions to provide ABI stability. Callers never access struct members
194+
directly; they use the generated accessors instead. The generated files are
195+
committed to the source tree and are **not** regenerated during a normal build.
196+
197+
Two sets of accessors are maintained — one for common NVMe structs and one for
198+
NVMe-oF-specific structs. The split exists so that non-fabrics (e.g. embedded
199+
or PCIe-only) builds can exclude all fabrics code entirely.
200+
201+
| Meson target | Input header | Generated files |
202+
|---|---|---|
203+
| `update-common-accessors` | `src/nvme/private.h` | `src/nvme/accessors.{h,c}`, `src/accessors.ld` |
204+
| `update-fabrics-accessors` | `src/nvme/private-fabrics.h` | `src/nvme/nvmf-accessors.{h,c}`, `src/nvmf-accessors.ld` |
205+
206+
### When to regenerate
207+
208+
Regeneration is needed whenever a `/*!generate-accessors*/` struct in
209+
`private.h` or `private-fabrics.h` has a member added, removed, or renamed.
210+
211+
### How to regenerate
212+
213+
To regenerate both sets at once:
214+
215+
```bash
216+
meson compile -C .build update-accessors
217+
```
218+
219+
Or regenerate only one set:
220+
221+
```bash
222+
meson compile -C .build update-common-accessors
223+
meson compile -C .build update-fabrics-accessors
224+
```
225+
226+
The script atomically updates the `.h` and `.c` files when their content
227+
changes. Commit the updated files afterward:
228+
229+
```bash
230+
git add libnvme/src/nvme/accessors.h libnvme/src/nvme/accessors.c
231+
git add libnvme/src/nvme/nvmf-accessors.h libnvme/src/nvme/nvmf-accessors.c
232+
git commit -m "libnvme: regenerate accessors following <struct> changes"
233+
```
234+
235+
### Maintaining the .ld version-script files
236+
237+
The `.ld` files (`src/accessors.ld` and `src/nvmf-accessors.ld`) are GNU
238+
linker version scripts that control which accessor symbols are exported from
239+
the shared library and under which ABI version label they were introduced
240+
(e.g. `LIBNVME_ACCESSORS_3`, `LIBNVMF_ACCESSORS_3`).
241+
242+
These files are **not** updated automatically, because each new symbol must be
243+
placed in the correct version section by the maintainer. Adding a symbol to an
244+
already-published version section would break binary compatibility for
245+
existing users of the library.
246+
247+
When the generator detects that the symbol list has drifted, it prints a
248+
report like the following:
249+
250+
```
251+
WARNING: accessors.ld needs manual attention.
252+
253+
Symbols to ADD (new version section, e.g. LIBNVME_ACCESSORS_X_Y):
254+
libnvme_ctrl_get_new_field
255+
libnvme_ctrl_set_new_field
256+
```
257+
258+
New symbols must be added to a **new** version section that chains the
259+
previous one. For example, if the current latest section is
260+
`LIBNVME_ACCESSORS_3`, add:
261+
262+
```
263+
LIBNVME_ACCESSORS_4 {
264+
global:
265+
libnvme_ctrl_get_new_field;
266+
libnvme_ctrl_set_new_field;
267+
} LIBNVME_ACCESSORS_3;
268+
```
269+
270+
Then commit the updated `.ld` file together with the regenerated source files.

libnvme/examples/discover-loop.c

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,13 @@ static void print_discover_log(struct nvmf_discovery_log *log)
5151

5252
int main()
5353
{
54-
struct nvmf_discovery_log *log;
54+
struct nvmf_discovery_log *log = NULL;
5555
struct libnvme_global_ctx *ctx;
5656
libnvme_host_t h;
5757
libnvme_ctrl_t c;
5858
int ret;
5959
struct libnvme_fabrics_config cfg;
60+
struct nvmf_discovery_args *args;
6061

6162
nvmf_default_config(&cfg);
6263

@@ -86,7 +87,13 @@ int main()
8687
return 1;
8788
}
8889

89-
ret = nvmf_get_discovery_log(c, &log, 4);
90+
ret = nvmf_discovery_args_create(&args);
91+
if (!ret) {
92+
nvmf_discovery_args_set_max_retries(args, 4);
93+
ret = nvmf_get_discovery_log(c, args, &log);
94+
nvmf_discovery_args_free(args);
95+
}
96+
9097
libnvme_disconnect_ctrl(c);
9198
libnvme_free_ctrl(c);
9299

libnvme/libnvme/nvme.i

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -777,25 +777,22 @@ struct libnvme_ns {
777777

778778
%newobject discover;
779779
struct nvmf_discovery_log *discover(int lsp = 0, int max_retries = 6) {
780-
const char *dev;
781780
struct nvmf_discovery_log *logp = NULL;
782-
struct libnvme_get_discovery_args args = {
783-
.c = $self,
784-
.args_size = sizeof(args),
785-
.max_retries = max_retries,
786-
.result = NULL,
787-
.timeout = NVME_DEFAULT_IOCTL_TIMEOUT,
788-
.lsp = lsp,
789-
};
781+
struct nvmf_discovery_args *args = NULL;
790782

791-
dev = libnvme_ctrl_get_name($self);
792-
if (!dev) {
783+
if (!libnvme_ctrl_get_name($self)) {
793784
discover_err = 1;
794785
return NULL;
795786
}
787+
discover_err = nvmf_discovery_args_create(&args);
788+
if (discover_err)
789+
return NULL;
790+
nvmf_discovery_args_set_lsp(args, lsp);
791+
nvmf_discovery_args_set_max_retries(args, max_retries);
796792
Py_BEGIN_ALLOW_THREADS /* Release Python GIL */
797-
discover_err = nvmf_get_discovery_wargs(&args, &logp);
793+
discover_err = nvmf_get_discovery_log($self, args, &logp);
798794
Py_END_ALLOW_THREADS /* Reacquire Python GIL */
795+
nvmf_discovery_args_free(args);
799796

800797
if (logp == NULL) discover_err = 2;
801798
return logp;

libnvme/src/libnvmf.ld

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,9 @@ LIBNVMF_3 {
2828
nvmf_exat_ptr_next;
2929
nvmf_free_uri;
3030
nvmf_get_default_trsvcid;
31+
nvmf_discovery_args_create;
32+
nvmf_discovery_args_free;
3133
nvmf_get_discovery_log;
32-
nvmf_get_discovery_wargs;
3334
nvmf_is_registration_supported;
3435
nvmf_nbft_free;
3536
nvmf_nbft_read_files;

libnvme/src/meson.build

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,12 @@ if want_fabrics
4545
sources += [
4646
'nvme/fabrics.c',
4747
'nvme/nbft.c',
48+
'nvme/nvmf-accessors.c',
4849
]
4950
headers += [
5051
'nvme/fabrics.h',
5152
'nvme/nbft.h',
53+
'nvme/nvmf-accessors.h',
5254
]
5355
endif
5456

@@ -85,6 +87,7 @@ endif
8587
nvme_ld = meson.current_source_dir() / 'libnvme.ld'
8688
nvmf_ld = meson.current_source_dir() / 'libnvmf.ld'
8789
accessors_ld = meson.current_source_dir() / 'accessors.ld'
90+
nvmf_accessors_ld = meson.current_source_dir() / 'nvmf-accessors.ld'
8891

8992
link_args = [
9093
'-Wl,--version-script=@0@'.format(nvme_ld),
@@ -93,9 +96,12 @@ link_args = [
9396

9497
libconf = configuration_data()
9598
if want_fabrics
96-
link_args += '-Wl,--version-script=@0@'.format(nvmf_ld)
99+
link_args += [
100+
'-Wl,--version-script=@0@'.format(nvmf_ld),
101+
'-Wl,--version-script=@0@'.format(nvmf_accessors_ld),
102+
]
97103
libconf.set('FABRICS_INCLUDE',
98-
'#include <nvme/fabrics.h>\n#include <nvme/nbft.h>')
104+
'#include <nvme/fabrics.h>\n#include <nvme/nbft.h>\n#include <nvme/nvmf-accessors.h>')
99105
else
100106
libconf.set('FABRICS_INCLUDE', '')
101107
endif

0 commit comments

Comments
 (0)