stream >> reading last line twice

Question

I am trying to parse /proc/partitions file.

major minor  #blocks  name

   8        0  976762584 sda
   8        1   99998720 sda1
   8        2          1 sda2
   8        3  103561216 sda3
   8        4  291514368 sda4
   8        5    1998848 sda5

This is the /proc/partitions file in my machine.

#include <boost/cstdint.hpp>
#include <fstream>
#include <boost/algorithm/string/trim.hpp>
#include <boost/format.hpp>

int main(){
  std::ifstream proc_partitions_stream("/proc/partitions");
  boost::int32_t disc_partition_line_count = -2; //-1 for headers, -1 for the empty line
  //so counter is 0 when it tries to read the real entries
  while(!proc_partitions_stream.fail()){
    if(disc_partition_line_count >= 0){
      boost::uint16_t   major, minor;
      boost::uint64_t   blocks;
      std::string   label;
      proc_partitions_stream >> major >> minor >> blocks >> label;
      std::cout << boost::format("%1% %2% %3% %4%") % major % minor % blocks % label << std::endl;
      boost::algorithm::trim(label);      
    }else{
      std::string line;
      std::getline(proc_partitions_stream, line);
    }
    ++disc_partition_line_count;
  }
    return 0;
}

But it reads the last line twice Here is the program

8 0 976762584 [sda]
8 1 99998720 [sda1]
8 2 1 [sda2]
8 3 103561216 [sda3]
8 4 291514368 [sda4]
8 5 1998848 [sda5]
8 5 1998848 [] << read the last line TWICE but didn't read the label

You never check your input extractions for success. And you're loop is no better than checking for eof in a while-conditional, [which is almost *always* wrong](http://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-considered-wrong) — WhozCraig, Nov 22 '13 at 10:12

john · Answer 1 · 2013-11-22T10:17:36.427

3

Because your while loop condition is wrong, you test for failure before you read, you should test for failure after you read. Even better would be to test the return from getline.

edited Nov 22 '13 at 10:17

answered Nov 22 '13 at 10:12

john

71,156
4
49
68

But then I've to create a stringstream after getline and extract values or split by space. Should I check after every `stream >> v` operation ? and why would it read the line twice ? it should always go forward. But it is going backward to the beginning of last line. – Neel Basu Nov 22 '13 at 10:19
1

@NeelBasu It's not reading twice. It just doesn't overwrite the variable values by the unsuccessful read. So you just *process* them twice. And you don't have to test every `>>`; testing the last one is sufficient. – Angew is no longer proud of SO Nov 22 '13 at 10:20
Ya Thanks. I see now. If I initialize them to 0 its not filling them with previous value again. – Neel Basu Nov 22 '13 at 10:21
I've tried checking after the last `>>` operation. I did put an `if(proc_partitions_stream.fail())break;` after `>> label` that gave same output – Neel Basu Nov 22 '13 at 15:09
Try `if (!proc_partitions_stream) break;`. I believe `fail()` is specifically for failure to convert, you have end of file. – john Nov 22 '13 at 15:17

score 2 · Answer 2 · answered Nov 22 '13 at 10:45

I would rewrite it to something more like:

#include <boost/cstdint.hpp>
#include <fstream>
#include <boost/algorithm/string/trim.hpp>
#include <boost/format.hpp>

int main(){
  std::ifstream proc_partitions_stream("/proc/partitions");

  for (int i=0; i<2; i++)
      proc_partitions_stream.ignore(max_len, '\n');

  boost::uint16_t   major, minor;
  boost::uint64_t   blocks;
  std::string   label;
  while (proc_partitions_stream >> major >> minor >> blocks >> label) {
      std::cout << boost::format("%1% %2% %3% %4%") % major % minor % blocks % label << "\n";
      //boost::algorithm::trim(label); // was present, but non-functional?
  }
  return 0;
}

Alternatively, define a small class to represent a disc partition, and overload operator>> and << for that, as well as a small function to skip lines from an istream:

class partition { 
    boost::uint16_t major, minor;
    boost uint64_t  blocks;
    std::string     label;
public:
     friend std::istream &operator>>(std::istream &is, partition &p) { 
         return is >> major >> minor >> blocks >> label;
     }

     friend std::ostream &operator<<(std::ostream &os, partition const &p) { 
         return os << boost::format("%1% %2% %3% %4%") % major % minor % blocks % label;
     }
};

std::istream &skiplines(std::istream &is, unsigned count) { 
   unsigned max_len = something; // see below
   return is.ignore(max_len, '\n');
}

Then in main you'd just have something like:

if (!skiplines(2)) {
    std::cerr << "Error!\n";
    return 1;
}

std::copy(std::istream_iterator<partition>(proc_partitions_stream),
          std::istream_iterator<partition>(),
          std::ostream_iterator<partition>(std::cout, "\n"));

As far as what value to use for max_len goes: quite a few people use std::numeric_limits<std::streamsize>::max(). I prefer something quite a bit smaller as a rule. It probably doesn't make any difference in this case (chances or mis-formatted input are small) but if you're just trying to skip a line, it's better to limit it to some amount that's at least halfway reasonable for a line. If you're just going to tell the user there's a problem anyway, there's no reason to waste their time waiting for you to read gigabytes of garbage before you do so.

stream >> reading last line twice

2 Answers2