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

[Wolfram Sang <wsa@kernel.org>]

[-- 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 --]