Re: [PATCH v4 1/2] i2c-tools: Allow passing device file paths
"Brigham Campbell" <[email protected]>
| Newsgroups | org.kernel.vger.linux-i2c |
|---|---|
| Message-ID | <[email protected]> |
Hi Gero,
On Fri Jul 17, 2026 at 7:00 AM MDT, Gero Schwäricke wrote:
> On Thu Jul 16, 2026 at 4:49 PM CEST, Brigham Campbell wrote:
>> if (errno != ENOENT) {
>> fprintf(stderr, "Error: Could not open file "
>> "`%s': %s\n", i2cbus_arg, strerror(errno));
>> if (errno == EACCES)
>> fprintf(stderr, "Run as root?\n");
>> return file;
>> }
>>
>> fprintf(stderr, "Error: `%s' is not a bus number, name, or device file "
>> "path!\n", i2cbus_arg);
>
> I'm unsure about this: Yes, this reflects the behavior of
> `open_i2c_dev_by_nr()`, but we only call that after we have validated
> that the input is indeed an adapter number.
The input is not necessarily an adapter number. We call
open_i2c_dev_by_nr after parsing the argument as an unsigned long, but
that's no guarantee that the input is an adapter number. For example,
if a system has /dev/i2c-0 and /dev/i2c-1, `i2cdetect -F 100` will
dutifully try to open /dev/i2c-100 and eventually fail without trying to
interpret the number as a bus name. That's ok because it's just a
heuristic and we don't expect the collision of file paths, bus names,
and unsigned integers.
> With the path we don't do
> that, we just try to open the input as a path. We know it's not a valid
> integer, and not a valid adapter name, but it may still not be a path,
> maybe it's a mistyped adapter name.
>
> To that I'm not sure we can conclude that `errno != ENOENT` means it is
> indeed a path.
This is a good point. `errno != ENOENT` doesn't necessarily mean that
the parameter is a file and that i2c-tools should be able to open it.
> Looking at libgpiod (`gpiod_chip_open_lookup()`), they solved that by
> assuming that all paths must start with `/dev/`. Unsure if we want to go
> that route as well. It would definitely simplify things and I think for
> the sake of progress that whould be fine. Thoughts?
The approach you suggest assumes that when the user passes in a device
file path, the path is _not_ a relative path, devtmpfs is mounted at
`/dev/`, and if the path is a symlink, it's also in `/dev/`. Maybe these
assumptions are ok (after all, libgpiod makes them) but they seem
unnecessary.
What if we only indicate that the argument couldn't be opened as a file
specifically when open() returns EACCES? Otherwise, we'll print a
generic message, indicating that the argument couldn't be parsed as a
bus number, name, or device file path. Even when i2c-tools should have
been able to open the argument as a path, but couldn't because of some
unrelated error, the binary will print a less-specific error message
which is still applicable and correct:
file = open(i2cbus_arg, O_RDWR);
if (file >= 0 || quiet) return file;
if (errno == EACCES) {
fprintf(stderr, "Error: Could not open file `%s': %s\n"
"Run as root?\n", i2cbus_arg, strerror(errno));
return file;
}
fprintf(stderr, "Error: Couldn't interpret `%s' as a bus number, "
"name, or device file path!\n", i2cbus_arg);
return file;
Sorry to double down on the bikeshedding (again, I do really appreciate
the feedback)... I'm interested to hear what Wolfram has to say. I may
send out v5 (which includes the above code) along with an invitation to
either make editorial changes to the patch in case the maintainer has
something different in mind or request that I send out a v6. I have v5
queued up which includes the above snippet.
--
Brigham Campbell
https://brighamcampbell.com