2
\$\begingroup\$

I don't program much in c, but cobbled together some code from different places in google. I'm sure this is backwards especially in how it works with strings and directory files. Please let me know what the best practices would have been.

/* usbresetall -- reset all of the usb devices on bus 001 */

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/ioctl.h>

#include <dirent.h> 
#include <string.h>

#include <linux/usbdevice_fs.h>


int main(int argc, char **argv)
{
    int fd;
    int rc;

    DIR *d;
    char filename[256];  
    struct dirent *dir;

    d = opendir("/dev/bus/usb/001/");
    strcpy(filename,"/dev/bus/usb/001/");
    
    if (d) {
        while ((dir = readdir(d)) != NULL) {
            strcat(filename,dir->d_name);
            printf("%s\n", filename);
            
        fd = open(filename, O_WRONLY);
        if (fd < 0) {
        perror("Error opening output file");
        } else {

        printf("Resetting USB device %s\n", filename);
        rc = ioctl(fd, USBDEVFS_RESET, 0);
        if (rc < 0) {
            perror("Error in ioctl");
        } else printf("Reset successful\n");
            
        close(fd);
            }         
            
            strcpy(filename,"/dev/bus/usb/001/");
        }
        closedir(d);
    }

    return 0;
}
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to CodeReview. Does your code work as expected? \$\endgroup\$ – Peter Csala Apr 12 at 6:05
  • \$\begingroup\$ It does work. I don't like how it gets around using strings and filenames, but it does work. \$\endgroup\$ – Rasheed Apr 12 at 16:07
1
\$\begingroup\$

Use consistent indentation

I don't know if the indentation style of the code you pasted here is as you wrote it, but if it looks the same in your code editor, you should definitely try to fix it and make it more consistent. I would not bother trying to fix this manually, instead use either code formatting functions of your editor, or use an external tool such as GNU indent, ClangFormat or Artistic Style to reformat your source code.

Return a non-zero exit code on error

When you encounter an error you call perror(), which is great, but then you let the program continue to run, and eventually you will always return 0. It would be better if your program would return a non-zero exit code when it has encountered an error, in particular return EXIT_FAILURE, which is declared in <stdlib.h>.

Don't hardcode the USB bus number

You only open USB bus 1. What if you attach your mouse to a different USB port, which might be on a different bus? I would make it so that the USB bus number has to be passed as a command line option. This makes the program a lot more flexible.

Beware of buffer overflows

While arguably the /dev/bus/usb pathnames can never be longer than 255 bytes, it is bad practice to make any assumptions. At the very least, make sure you never overflow this buffer, by using strncpy() and strncat() instead of strcpy() and strcat() (read the documentation of these functions carefully and be aware what happens if the destination buffer is not large enough to hold the input), or use snprintf(). However, there is another way to sidestep this issue entirely:

Avoid the need for concatenating paths

Instead of manipulating filename all the time, you can avoid all this by calling chdir() to go into the USB bus's directory, like so:

if (chdir("/dev/bus/usb/001/") == -1) {
    perror("Error accessing USB bus directory");
    return EXIT_FAILURE;
}

DIR *d = opendir(".");

if (!d) {
    perror("Error reading USB bus directory");
    return EXIT_FAILURE;
}

struct dirent *ent;

while ((ent = readdir(d)) != NULL) {
    int fd = open(ent->d_name, O_WRONLY);
    ...

Declare variables as close as possible to where they are used

In C99 and later, you can declare a variable anywhere. It is considered good practice to declare a variable close to where it is used, and ideally you also declare and initialize it in one go. I've used this in the example above.

\$\endgroup\$
1
  • \$\begingroup\$ Great, this is very helpful. \$\endgroup\$ – Rasheed Apr 12 at 16:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.