[PATCH i2c-tools v2] i2cdetect: add messages for errors during bus listing

[PATCH i2c-tools v2] i2cdetect: add messages for errors during bus listing

[Paul Wise]

Include appropriate commands for fixing the errors.

This makes it easier for new users to understand what is going on when
they have a problem listing i2c busses that they do not understand.

Inspired-by: https://lists.debian.org/msgid-search/E1poMGW-0002KT-8Q@enotuniq.net
---
 tools/i2cbusses.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

v2: fixed some whitespace styling, updated recipient addresses
diff --git a/tools/i2cbusses.c b/tools/i2cbusses.c
index d23ee7a..8d16905 100644
--- a/tools/i2cbusses.c
+++ b/tools/i2cbusses.c
@@ -137,6 +137,24 @@ static int sort_i2c_busses(const void *a, const void *b)
 	return adap1->nr - adap2->nr;
 }
 
+static int dir_has_entries(const char* path)
+{
+	struct dirent *de;
+	DIR *dir;
+	if ((dir = opendir(path))) {
+		while ((de = readdir(dir)) != NULL) {
+			if (!strcmp(de->d_name, "."))
+				continue;
+			if (!strcmp(de->d_name, ".."))
+				continue;
+			closedir(dir);
+			return 1;
+		}
+		closedir(dir);
+	}
+	return 0;
+}
+
 struct i2c_adap *gather_i2c_busses(void)
 {
 	char s[120];
@@ -144,6 +162,7 @@ struct i2c_adap *gather_i2c_busses(void)
 	DIR *dir, *ddir;
 	FILE *f;
 	char fstype[NAME_MAX], sysfs[NAME_MAX], n[NAME_MAX];
+	size_t sysfs_end = 0;
 	int foundsysfs = 0;
 	int len, count = 0;
 	struct i2c_adap *adapters;
@@ -199,6 +218,13 @@ struct i2c_adap *gather_i2c_busses(void)
 	/* look in sysfs */
 	/* First figure out where sysfs was mounted */
 	if ((f = fopen("/proc/mounts", "r")) == NULL) {
+		fprintf(stderr, "Error: Could not open /proc/mounts: "
+                                "%s\n", strerror(errno));
+		if (errno == ENOENT) {
+			fprintf(stderr, "Please mount procfs: "
+			        "%smount -t procfs proc /proc\n",
+			        getenv("SUDO_COMMAND") ? "sudo " : "");
+		}
 		goto done;
 	}
 	while (fgets(n, NAME_MAX, f)) {
@@ -210,15 +236,43 @@ struct i2c_adap *gather_i2c_busses(void)
 	}
 	fclose(f);
 	if (! foundsysfs) {
+		fprintf(stderr, "Error: Could not find sysfs mount\n");
+		fprintf(stderr, "Please mount sysfs: "
+		        "%smount -t sysfs sysfs /sys\n",
+                        getenv("SUDO_COMMAND") ? "sudo " : "");
 		goto done;
 	}
 
 	/* Bus numbers in i2c-adapter don't necessarily match those in
 	   i2c-dev and what we really care about are the i2c-dev numbers.
 	   Unfortunately the names are harder to get in i2c-dev */
+	sysfs_end = strlen(sysfs);
 	strcat(sysfs, "/class/i2c-dev");
-	if(!(dir = opendir(sysfs)))
+	if (!(dir = opendir(sysfs))) {
+		if (errno == ENOENT) {
+			/* Check if there are i2c bus devices in other dirs
+                           as when there are none the error isn't useful
+                           as loading i2c-dev also won't find devices */
+			int devices_present = 0;
+			strcpy(sysfs + sysfs_end, "/bus/i2c/devices");
+			devices_present = dir_has_entries(sysfs);
+			if (! devices_present) {
+				strcpy(sysfs + sysfs_end, "/class/i2c-adapter");
+				devices_present = dir_has_entries(sysfs);
+			}
+			if (devices_present) {
+				fprintf(stderr, "Error: Could not find dir "
+				        "`%s`\n", sysfs);
+				fprintf(stderr, "Please load i2c-dev: "
+				        "%smodprobe i2c-dev\n",
+					getenv("SUDO_COMMAND") ? "sudo " : "");
+			}
+		} else {
+			fprintf(stderr, "Error: Could not open dir "
+				"`%s': %s\n", sysfs, strerror(errno));
+		}
 		goto done;
+	}
 	/* go through the busses */
 	while ((de = readdir(dir)) != NULL) {
 		if (!strcmp(de->d_name, "."))
-- 
2.40.1

Re: [PATCH i2c-tools v2] i2cdetect: add messages for errors during bus listing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2944 bytes --]

Hi Paul,

thank you for this patch!

> This makes it easier for new users to understand what is going on when
> they have a problem listing i2c busses that they do not understand.

I like this basically. I do have questions, though.

> @@ -199,6 +218,13 @@ struct i2c_adap *gather_i2c_busses(void)
>  	/* look in sysfs */
>  	/* First figure out where sysfs was mounted */
>  	if ((f = fopen("/proc/mounts", "r")) == NULL) {
> +		fprintf(stderr, "Error: Could not open /proc/mounts: "
> +                                "%s\n", strerror(errno));
> +		if (errno == ENOENT) {
> +			fprintf(stderr, "Please mount procfs: "
> +			        "%smount -t procfs proc /proc\n",
> +			        getenv("SUDO_COMMAND") ? "sudo " : "");

Hmm, I wonder if a simple "Is it mounted?" isn't enough?

> +		fprintf(stderr, "Error: Could not find sysfs mount\n");
> +		fprintf(stderr, "Please mount sysfs: "
> +		        "%smount -t sysfs sysfs /sys\n",
> +                        getenv("SUDO_COMMAND") ? "sudo " : "");

Ditto.

>  		goto done;
>  	}
>  
>  	/* Bus numbers in i2c-adapter don't necessarily match those in
>  	   i2c-dev and what we really care about are the i2c-dev numbers.
>  	   Unfortunately the names are harder to get in i2c-dev */
> +	sysfs_end = strlen(sysfs);
>  	strcat(sysfs, "/class/i2c-dev");
> -	if(!(dir = opendir(sysfs)))
> +	if (!(dir = opendir(sysfs))) {
> +		if (errno == ENOENT) {
> +			/* Check if there are i2c bus devices in other dirs
> +                           as when there are none the error isn't useful
> +                           as loading i2c-dev also won't find devices */
> +			int devices_present = 0;
> +			strcpy(sysfs + sysfs_end, "/bus/i2c/devices");
> +			devices_present = dir_has_entries(sysfs);
> +			if (! devices_present) {
> +				strcpy(sysfs + sysfs_end, "/class/i2c-adapter");
> +				devices_present = dir_has_entries(sysfs);
> +			}
> +			if (devices_present) {
> +				fprintf(stderr, "Error: Could not find dir "
> +				        "`%s`\n", sysfs);
> +				fprintf(stderr, "Please load i2c-dev: "
> +				        "%smodprobe i2c-dev\n",
> +					getenv("SUDO_COMMAND") ? "sudo " : "");
> +			}

If there are no devices_present here, then we fail without printing
something? Is this intended?

> +		} else {
> +			fprintf(stderr, "Error: Could not open dir "
> +				"`%s': %s\n", sysfs, strerror(errno));

Despite the above detail, I think this adds quite some code (also
counting dir_has_entries). Since I think we need i2c-dev anyway, can't
we just do:

1) say "please load i2c-dev" if it is not loaded
2) say "could not open dir" if it is loaded

Yes, for rare cases this could mean that loading "i2c-dev" does not
solve the problem, but using i2ctools without i2c-dev is going to fail
at some point anyhow, so IMHO we can complain about this early?

Makes sense? Did I miss something?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i2c-tools v2] i2cdetect: add messages for errors during bus listing

[Paul Wise]

[-- Attachment #1: Type: text/plain, Size: 2457 bytes --]

On Wed, 2023-06-14 at 23:31 +0200, Wolfram Sang wrote:

> Hmm, I wonder if a simple "Is it mounted?" isn't enough?

It depends on the sophistication and mental state you want to require
from i2cdetect users. Some i2cdetect users might not know or currently
remember the exact commands needed to fix their issues. Personally, I
haven't memorised how to mount these directories, so I would need to go
look up the documentation. Others might go ask mailing lists, search
engines or AI assistants if they are online. So as I said on the Debian
ARM list, since the issues all have very simple solutions, I added the
commands to fix them instead of pointers to documentation about each
error, or expecting users to know about them. Given that the RPi has
lead to a large influx of users with potentially little Linux
experience doing GPIO stuff, this patch could reduce support requests.

https://lists.debian.org/msgid-search/8e2bfcf0375433acec188bbbf11fa9117076fe6b.camel@debian.org

Please don't merge this just yet though; thinking about this more
and reading the Debian thread a bit more, I think I want to send
a v3 patch that also tells users that pass the wrong i2c bus number
to run `i2cdetect -l` to find the right bus.

https://lists.debian.org/msgid-search/e496b7d9-e047-43df-9fe3-f65bdc37bf93@app.fastmail.com
https://lists.debian.org/msgid-search/E1poJwd-0002Go-RM@enotuniq.net


> If there are no devices_present here, then we fail without printing
> something? Is this intended?

There wouldn't be a failure, just exit without printing any busses and
without printing any error messages. Perhaps it should a print no
busses found error message in that case instead?

> Yes, for rare cases this could mean that loading "i2c-dev" does not
> solve the problem, but using i2ctools without i2c-dev is going to fail
> at some point anyhow, so IMHO we can complain about this early?

Sure, but that wouldn't necessarily be very user-friendly.

If there are no i2c busses present in the other i2c directories (which
IIRC are always present), then there are no i2c busses on the system
and so loading i2c-dev will not make i2cdetect print any busses, so
loading i2c-dev is pointless, so we don't ask the user to do that,
because it would be frustrating to be asked to do something that will
then not mean any change in the situation, aka zero devices output.

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i2c-tools v2] i2cdetect: add messages for errors during bus listing

[Jean Delvare]

Hi Wolfram, Paul,

First of all, sorry for not replying earlier to the v1 of this patch.

On Wed, 14 Jun 2023 23:31:05 +0200, Wolfram Sang wrote:
> Hi Paul,
> 
> thank you for this patch!
> 
> > This makes it easier for new users to understand what is going on when
> > they have a problem listing i2c busses that they do not understand.  
> 
> I like this basically. I do have questions, though.

I don't like it.

I don't think this is the purpose of i2c tools, which are very
specific, to tell the user how to perform generic system administration
tasks. If /proc or /sys is not mounted then you are facing a much
bigger problem than i2cdetect not working. Basically your system is
close to unusable, and this is completely unrelated to i2c-tools. I
can't even think of a realistic situation where these new error
messages would show up. So this is dead code to me.

Plus, manually mounting sysfs or procfs may solve your immediate
problem, but it is most certainly not the proper way to fix your
system. So this isn't even good advice.

The other thing this patch is doing is telling about missing i2c-dev.
Now this is something that can actually happen, and will happen. I
actually have an item about that on my todo list for over a decade now.
So I'm fine with that part, even though I think the proposed
implementation is too complex (I wouldn't bother checking if there are
i2c buses on the system).

However maybe we could be even more user-friendly. Considering that you
must be root to run these tools, and these tools can't work without
i2c-dev, I think we should simply load the i2c-dev module automatically
as needed. Either with a simple system("modprobe i2c-dev") call, or
maybe using libkmod if it has a public API (not sure).

-- 
Jean Delvare
SUSE L3 Support