Refactor/replace code for more portability (part 1)#3355
Conversation
|
@bgoing-micron-oss the next PR ontop of the memory API changes. |
dprintf is not available on Windows, so open-code it using asprintf and write. Signed-off-by: Daniel Wagner <wagi@kernel.org>
|
@bjpaupor quick question on |
|
@bjpaupor Forget it, I just brought the feature back :) |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve Windows portability by removing/avoiding non-portable libc APIs (dprintf, getline, open_memstream) and plumbing a device-derived “product name” into the identify controller output path.
Changes:
- Replaced
dprintf()usage in libnvme logging with a buffer + write loop approach. - Replaced
getline()usage withfgets()in a couple of parsing/reading paths. - Extended the
id_ctrlprinting API to optionally include a derivedproduct_namein stdout/JSON output.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/micron/micron-nvme.c | Updates nvme_show_id_ctrl() call site to pass transport handle name. |
| nvme.c | Updates nvme_show_id_ctrl() call site to pass transport handle name. |
| nvme-print.h | Changes print_ops->id_ctrl and nvme_show_id_ctrl() signatures to carry product/device name context. |
| nvme-print.c | Computes product_name via nvme_product_name(devname) and passes it into the print backend. |
| nvme-print-stdout.c | Prints product_name before the controller identify output; removes open_memstream usage in effects log printing. |
| nvme-print-json.c | Adds product_name to JSON identify-controller output. |
| nvme-print-binary.c | Updates binary identify-controller print signature to match new print_ops. |
| nvme-models.h | Changes nvme_product_name() to accept a device name string rather than an integer id. |
| nvme-models.c | Replaces getline() with fgets(); adds device-name parsing to obtain controller instance id. |
| libnvme/src/nvme/log.c | Replaces dprintf() with asprintf() + write_all() implementation. |
| libnvme/src/nvme/linux.c | Replaces getline() with fgets() for reading product_uuid; adds additional validation. |
| libnvme/src/meson.build | Moves nvme/log.c into unconditional sources list (now built on Windows too). |
Comments suppressed due to low confidence (1)
libnvme/src/nvme/log.c:119
- Calling
perror()here introduces an unconditional stderr side effect from inside the library logging path, which can surprise consumers (and may recurse if stderr logging is also routed back into libnvme). Prefer returning silently, or reporting the failure via the existing libnvme logging mechanism/state rather than printing directly to stderr.
if (write_all(l->fd, log, strlen(log)) < 0)
perror("failed to write log entry");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
getline is not available on all platforms. Since the UUID input has a fixed and well-defined format, use fgets instead. This avoids the dynamic allocation from getline and improves portability Signed-off-by: Daniel Wagner <wagi@kernel.org>
stdout_effects_log_segment wants to print a header if there are any log entries. This is done using a memstream for - a "did anything print?" detector - plus a temporary accumulation buffer That's overkill here. Refactor this function with the side effect it also portable. Signed-off-by: Daniel Wagner <wagi@kernel.org>
The make the port on Windows simpler replace getline with fgets. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Make the interface easier to use on the callside by asking for the device name instead the id. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Somewhere in the past the support to print the product name was lost. Bring it back. Signed-off-by: Daniel Wagner <wagi@kernel.org>
|
let's go with this set of changes. more to come... |
A bunch of functions are not easy portable, thus try to get rid of the by either refactor/replace the code so these are not used anymore.
These are changes based on #3310