| Message ID | 20230526002445.57064-1-pabs3@bonedaddy.net |
|---|---|
| State | New |
| Headers | show |
| Series | [i2c-tools,v2] i2cdetect: add messages for errors during bus listing | expand |
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
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, "."))