Hi Tarun, Minor review comments below Tarun Sahu <[email protected]> writes:
> For the buses, that don't have nfit support, they use to > return "No such file or directory" for start-scrub/ > wait-scrub command. s/they use to// > > Though, non-nfit support buses do not support start-scrub/ > wait-scrub operation. I propose this patch to handle these > commands more gracefully by returning "Operation not > supported". > s/Though/Presently/ s/non-nfit support buses do not support/non-nfit support buses do not support/ > Previously: > $ ./ndctl start-scrub ndbus0 > error starting scrub: No such file or directory > > Now: > $ ./ndctl start-scrub ndbus0 > error starting scrub: Operation not supported > The code changes look good to me though. But it would be better if you can describe the test setup that resulted in above output. I was able to test the patch on PPC64LE guest with an nvdimm device with following expected results: # Valid ndbus $ sudo ./ndctl start-scrub ndbus0 error starting scrub: Operation not supported # Invalid ndbus $ sudo ./ndctl start-scrub ndbus5 error starting scrub: No such device or address Hence, > Signed-off-by: Tarun Sahu <[email protected]> Reviewed-by: Vaibhav Jain <[email protected]> Tested-by: Vaibhav Jain <[email protected]> > --- > ndctl/lib/libndctl.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > index ccca8b5..8bfad6a 100644 > --- a/ndctl/lib/libndctl.c > +++ b/ndctl/lib/libndctl.c > @@ -938,10 +938,14 @@ static void *add_bus(void *parent, int id, const char > *ctl_base) > if (!bus->wait_probe_path) > goto err_read; > > - sprintf(path, "%s/device/nfit/scrub", ctl_base); > - bus->scrub_path = strdup(path); > - if (!bus->scrub_path) > - goto err_read; > + if (ndctl_bus_has_nfit(bus)) { > + sprintf(path, "%s/device/nfit/scrub", ctl_base); > + bus->scrub_path = strdup(path); > + if (!bus->scrub_path) > + goto err_read; > + } else { > + bus->scrub_path = NULL; > + } > > sprintf(path, "%s/device/firmware/activate", ctl_base); > if (sysfs_read_attr(ctx, path, buf) < 0) > @@ -1377,6 +1381,9 @@ NDCTL_EXPORT int ndctl_bus_start_scrub(struct ndctl_bus > *bus) > struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus); > int rc; > > + if (bus->scrub_path == NULL) > + return -EOPNOTSUPP; > + > rc = sysfs_write_attr(ctx, bus->scrub_path, "1\n"); > > /* > @@ -1447,6 +1454,9 @@ NDCTL_EXPORT int ndctl_bus_poll_scrub_completion(struct > ndctl_bus *bus, > char in_progress; > int fd = 0, rc; > > + if (bus->scrub_path == NULL) > + return -EOPNOTSUPP; > + > fd = open(bus->scrub_path, O_RDONLY|O_CLOEXEC); > if (fd < 0) > return -errno; > -- > 2.35.1 > > -- Cheers ~ Vaibhav
