1748 replies to this topic

[captnoord]

fixed file handle leak on read error in 'lib/service/servicedvd.cpp'

Attached Files

•  fixed-file-handle-leak-on-read-fail.patch   1003bytes   13 downloads

Edited by captnoord, 1 August 2014 - 19:44.

[captnoord]

Fixed while loop scope not being respected in 'lib/gui/elistbox.cpp'

 

This one is a wierd one, because currently 'something' gets executed.

situation could be compared to:

 

int number = 10;

while (--number);

{

    do_function(number); // is only called once, after the while has completed.

}

 

The guy who wrote that, could have meant:

 

int number = 10;

while (--number) // <--- ';' removed.

{

    do_function(number); // its called 10x

}

 

 

So that's what I did, currently can't test the change as my girl is watching tv

Attached Files

•  fixed-while-scope-not-beeing-respected.patch   717bytes   13 downloads

[littlesat]

Thanks.... I suppose Erik will pick it up....

[captnoord]

I really, really appreciate to see someone cleaning up.

 

Your welcome

[mirakels]

fixed file handle leak on read error in 'lib/service/servicedvd.cpp'

The code bcomes a bit more 'ugly' this way.

Such code warantsthe use of goto's, but in this case code could be simplified by using if (fread() && fread() && fread() && fread()) { ...}

 

I wonder why  'what = nothl(what);'  is done twice  (already in the original code)

[captnoord]

I agree, I could take some more time and actually do a bit of re-factoring.

[mirakels]

Fixed while loop scope not being respected in 'lib/gui/elistbox.cpp'

 

This one is a wierd one, because currently 'something' gets executed.

situation could be compared to:

 

int number = 10;

while (--number);

{

    do_function(number); // is only called once, after the while has completed.

}

 

The guy who wrote that, could have meant:

 

int number = 10;

while (--number) // <--- ';' removed.

{

    do_function(number); // its called 10x

}

 

 

So that's what I did, currently can't test the change as my girl is watching tv

Good catch captnoord. I introduced this 'bug  almost 2 and a half years ago! apparently this border case almost never happens.

I thing the patch is correct!

[mirakels]

applied the elistbox patch from captnoord.

How did you create the patch? 'git am' refused to apply it because of patch format error. Did you use 'git format-patch'?

 

(so applied it manually this time.)

[captnoord]

i'm a lazy gitk user and used the 'write commit to file' option. Next time I will create them using git format-patch.

[littlesat]

Again thanks!

[MiLo]

fixed file handle leak on read error in 'lib/service/servicedvd.cpp'

There is a CFile wrapper that prevents this kind of kludgy code. It's compatible with FILE* but since the destructor cleans up, you don't have to add all those "close" calls.

[captnoord]

fixed file handle leak on read error in 'lib/service/servicedvd.cpp'

There is a CFile wrapper that prevents this kind of kludgy code. It's compatible with FILE* but since the destructor cleans up, you don't have to add all those "close" calls.

Thats even better, but if we are doing that we should just search the code for any other use of FILE and replace it with CFile where possible.
Incremental improvement ftw.

[MiLo]

Thats even better, but if we are doing that we should just search the code for any other use of FILE and replace it with CFile where possible.

+1

[captnoord]

Thats even better, but if we are doing that we should just search the code for any other use of FILE and replace it with CFile where possible.

+1

I have looked into the matter a bit, I personally don't the CFile 'struct' very complete. I'm extending the CFile class
so that it fits the needs of what I have in mind.

Don't worry about patches for now as it is a little more work than I was hoping for.

[captnoord]

Btw, what is the lowest compiler version I should worry about?

[MiLo]

I have looked into the matter a bit, I personally don't the CFile 'struct' very complete. I'm extending the CFile class
so that it fits the needs of what I have in mind.

Yeah, you could add read/write methods to it so it looks a bit more OO, so you can write this:

CFile f("/tmp/file", "r");
ssize_t bytes = ::fread(buffer, sizeof(buffer), 1, f);

like this:

CFile f("/tmp/file", "r");
ssize_t bytes = f.read(buffer, sizeof(buffer), 1);

[MiLo]

Note that when you want to read binary files in larger chunks, you're much better of using "open()" and "read()" instead. FILE* is only good for parsing text files and processing files in tiny chunks.

[captnoord]

Don't worry, I know.

I was also thinking about something like this:

 

CFile f;

if (!f.open("/tmp/file", "r"))

   return false;

ssize_t bytes = f.read(buffer, sizeof(buffer), 1); 

Edited by captnoord, 4 August 2014 - 06:59.

[MiLo]

Currently, you should use:

CFile f("/tmp/file", "r");
if (!f)
  return false;

I would have preferred:

try
{
  CFile f("/tmp/file", "r");
  f.read(buffer, sizeof(buffer));
  ...
}
catch (const std::exception& ex)
{
  report_error_somewhere(ex.what());
}

but some things in the E2 code fail to compile when C++ exceptions are enabled.

Edited by MiLo, 4 August 2014 - 09:30.

[MiLo]

If you add an "open" method, it will need a "close" method as well, and "open" must (counterintuitively) call "close" too.

I don't see the added value of that, you might as well create a new object for a new file.