lib: Fix unchecked return value

[ShubhamDesai]

This pull request address "unchecked return value from library". The issue is identified by coverity scan (CID : 1207295, 1207291,1207292,1207294, 1207293).

All instances of remove(filename) are replaced with

if (remove(filename) != 0) {
            G_fatal_error(_("Unable to remove file %s"), filename);
}

[ShubhamDesai]

not sure whether to use G_warning or G_fatal_error

[wenzeslaus]

Warning versus error is not clear to me either. RTreeNewIndex gets file descriptor from an open call on a temporary file name, but then the file is deleted. I understand this is cleanup, but the behavior is not clear to me and so I can't decide on whether ignoring, warning, or erroring is the right thing to do.

[ShubhamDesai]

Warning versus error is not clear to me either. RTreeNewIndex gets file descriptor from an open call on a temporary file name, but then the file is deleted. I understand this is cleanup, but the behavior is not clear to me and so I can't decide on whether ignoring, warning, or erroring is the right thing to do.

Initially i used G_fatal_error so with that all test cases passed on Mac and ubuntu but failed on Windows.
when i checked the testcases most of them displayed error message with Unable to remove file.
i think because in windows systems a file cannot be removed without closing it. So added another check where first file descriptor is closed and then an attempt is made to remove the file.

Anyways for now with G_fatal i could identify the issue for closing the file descriptor and then attempt to remove

[wenzeslaus]

i think because in windows systems a file cannot be removed without closing it.

Great catch. Closing seems indeed appropriate.

[nilason]

This is a tricky one. There is much more to this than first meets the eye. I believe the expected behaviour of remove() in the code is to remove the file only when there is no further use of it. As long as it is open, as the file is when remove() is called, it will not be actually removed; it is a kind of delayed file removal (which happen to not work on windows).

According to cppreference remove():

Deletes the file identified by the character string pointed to by pathname.

If the file is currently open by any process, the behavior of this function is implementation-defined. POSIX systems unlink the file name (directory entry), but the filesystem space used by the file is not reclaimed while it is open in any process and while other hard links to the file exist. Windows does not allow the file to be deleted in such cases.

Furthermore, if we read POSIX' description of remove():

The remove() function shall cause the file named by the pathname pointed to by path to be no longer accessible by that name. A subsequent attempt to open that file using that name shall fail, unless it is created anew.

It is my understanding that the file is open and still accessible through the file descriptor returned by open(), but not accessible through the file name. The file descriptor (fd) is added to Plus member with RTreeCreateTree().

The description of RTreeDestroyTree() reads:

This method releases all memory allocated to a RTree. It deletes all rectangles and all memory allocated for internal support data. Note that for a file-based RTree, the file is not deleted and not closed. The file can thus be used to permanently store an RTree.

The fact that RTreeCreateTree() works even after a failed open() with return value of -1 using in-memory rtree, does not simplify this issue.

I have currently no suggestion on how to move this forward, but we need to hold on to this PR until we find out a proper fix. Its present state is not ok.

[wenzeslaus]

Thank you @nilason for the detailed analysis.

Thank you @ShubhamDesai for working on this until now. Unless you have a specific suggestion, just move to a different issue.

[ShubhamDesai]

Thank you @nilason for the detailed analysis.

Thank you @ShubhamDesai for working on this until now. Unless you have a specific suggestion, just move to a different issue.

Thanks @nilason for the review.
Sure @wenzeslaus I will move onto different issue