0

I'm trying to take the output from usb-devices and parsing it to assign it to a simple struct. However, I just can't get fscanf to properly read the values from the output. Here's the function in question:

struct Devices* getDevices() {

  int pipes[2];
  pid_t pid;

  if (pipe(pipes)==-1)
    die("Failed to create pipe");

  if ((pid = fork()) == -1)
    die("Could not fork process");

  if(pid == 0) {
    dup2 (pipes[1], STDOUT_FILENO);
    close(pipes[0]);
    close(pipes[1]);
    execl("/bin/usb-devices", "usb-devices", (char *)0);
  }
  else {
    close(pipes[1]);
    wait(NULL);
  }

  FILE *rawList = fdopen(pipes[0],"r");

  if (rawList == NULL)
    die("Failed to open file");

  fpos_t pos;
  int pos_init = 0;

  struct Devices *deviceList = malloc(sizeof(struct Devices));
  if(!deviceList) die ("Memory error");

  int i;
  int conn;
  int iBus = -1;
  int iPort = -1;

  for (i = 0; !feof(rawList); i++) {
    if (pos_init) fsetpos(rawList, &pos);

    conn = fscanf(rawList, "T:  Bus=%d Lev=%*d Prnt=%*d Port=%d Cnt=%*d Dev#=  %*d Spd=%*d MxCh= %*d",
          &iBus, &iPort);
    if (!conn) {
      printf("bus%d port%d num %i", iBus, iPort, i);
      deviceList->devNum[i].set = 1;
      deviceList->devNum[i].bus = iBus;
      deviceList->devNum[i].port = iPort;

      fgetpos (rawList,&pos); 
      pos_init = 1;
    }
  }        

  fclose(rawList);

  return deviceList;
}

Quickly explaining what's supposed to be happening. I'm creating a pipe so I can grap the output of the command I'm running, then after it runs I try to put it in a stream named rawList through fdopen. Then, I malloc a chunk of memory for the struct I will be assigning the values I get to.

Then, in a for loop it should check for the lines starting with "T:" which contain the data I want, and then it should take the integers that the output of usb-devices gives me and assign it to the struct per device. Finally, it should be saving the point it stopped reading to so it can keep reading from there to find the next line I'm looking for.

The printf is there to try and figure out what's going on. It ends up printing "-1" which is the value assigned before the for loop, which tells me fscanf isn't assigning any values to my variables. "i" just counts on until the program segfaults and ends up quitting. I'm not sure whether the value is meaningful, but Valgrind consistently tells me there are 77408 errors from 5 contexts (main is simply running this function, the only other things in the code are the struct declarations, the die function and main itself).

When the process finally quits, Valgrind gives me this:

==11278== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==11278==  Access not within mapped region at address 0x55F0040
==11278==    at 0x108C7A: getDevices (usbDeviceList.c:77)
==11278==    by 0x108D4C: main (usbDeviceList.c:103)

Line 77 is this one

deviceList->devNum[i].set = 1;

The last value of "i" that gets printed is 15644.

Any and all help is appreciated, I've asked around and no one seems to have the slightest clue as to what's happening. I'll list the SO resources I used to write this:

Grabbing output from exec

easy way to parse a text file?

Update:

As per request, here's the structs:

struct deviceData {
  int set;
  int bus;
  int port;
  char manufacturer[MAX_LENGTH_U];
  char product[MAX_LENGTH_U];
};

struct Devices {
  struct deviceData devNum[MAX_DEVICES_U];
};
Piero Vera
  • 1
  • 1
  • 1
  • This may not be your bug, but you wait for the child before you have read its output. If the child writes more output than fits in the OS pipe buffer, your program will deadlock: the child waiting for the buffer to be emptied before it continues, and the parent waiting for the child to exit before it continues. Otherwise, can you post a complete MCVE, that someone could actually compile and run? – Nate Eldredge Sep 26 '17 at 03:48
  • Let's see how `struct Devices` is declared. Is `devNum` an array, for which you have no checks as to whether you overrun it? Or a pointer, which you never initialize? – Nate Eldredge Sep 26 '17 at 03:51
  • And what's the point of the whole `fsetpos` business? You can't seek on a pipe (and if you ever checked the return value, you might have noticed this). And you never initialized `pos`. Do you have warnings enabled when you compile? (If not, for God's sake why not?) – Nate Eldredge Sep 26 '17 at 03:54
  • 2
    `popen` is much simpler than writing your own pipe+fork+exec+wait and equally standard (POSIX but not C). This won't alter your parsing problems though. @Nate: `pos` is not actually used uninitialized because of the `pos_init` logic -- but it is indeed useless on a pipe. Fortunately(?) OP always tries to 'move' to the current position, which is a no-op. Piero: since the output of `usb-devices` may vary or not exist for others I suggest temporarily saving it in a file, showing that file, and testing by `cat`ing that file (yes 'useless' `cat` so you have an actual pipe). – dave_thompson_085 Sep 26 '17 at 05:01
  • Nate, I have updated the question with the struct declarations. Also, the fsetpos is working on the stream not on the pipe, but perhaps I'm still doing it wrong, I'm not sure. No warnings with -Wall. Dave, I guess I can try putting it in a file, though I'm not sure what it will change. – Piero Vera Sep 26 '17 at 05:07
  • 1
    You cannot use fsetpos on a.pipe-backed stream. – n. m. could be an AI Sep 26 '17 at 08:59

1 Answers1

0

Your code won't emit any output when fscanf() actually works. Per the fscanf() documentation:

RETURN VALUE

Upon successful completion, these functions shall return the number of successfully matched and assigned input items. ...

This loop is wrong:

 for (i = 0; !feof(rawList); i++) {
    if (pos_init) fsetpos(rawList, &pos);

    // if this works, it returns 2
    conn = fscanf(rawList, "T:  Bus=%d Lev=%*d Prnt=%*d Port=%d Cnt=%*d Dev#=  %*d Spd=%*d MxCh= %*d",
          &iBus, &iPort);
    if (!conn) {

      // these only get called if conn is zero!
      printf("bus%d port%d num %i", iBus, iPort, i);
      deviceList->devNum[i].set = 1;
      deviceList->devNum[i].bus = iBus;
      deviceList->devNum[i].port = iPort;

      fgetpos (rawList,&pos); 
      pos_init = 1;
    }
  }

And as noted in the comments, fsetpos() on a pipe is useless.

Andrew Henle
  • 32,625
  • 3
  • 24
  • 56
  • Hey Andrew. I understand fsetpos is useless on a pipe, but the function is acting on rawList which is a stream, created from what was in the pipe via fdopen. Maybe it's still wrong, I'd just like some clarification on that. Thanks. – Piero Vera Sep 26 '17 at 21:39