On v0.9.12, I am observing an issue with XTGeoIP exports: the *.iv4 files do not get written.
The sample command I use to reproduce this on Ubuntu 20.04 is very basic: location export --format xt_geoip --directory /usr/share/xt_geoip
All IPv6 outputs are written successfully, but upon reaching IPv4 it errors with: (25, 'Inappropriate ioctl for device')
A quicker test case is to add argument `--family ipv4` so that IPv6 is hopped over and it tries IPv4 right away.
With strace, it seems to be: openat(AT_FDCWD, "/usr/share/xt_geoip/A1.iv4", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 4 fstat(4, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0 ioctl(4, TCGETS, 0x7ffd234c3260) = -1 ENOTTY (Inappropriate ioctl for device)
Please let me know if I can provide any further information.
Best regards, Valters Jansons
Hello Valters,
Hmm, this should not happen like this.
I am not even sure why this is trying to do any ioctls for terminals on the output file.
Is this reproducible with any of the other formats, too, or is it only xt_geoip?
-Michael
On 29 Mar 2022, at 15:07, Valters Jansons valter.jansons@gmail.com wrote:
On v0.9.12, I am observing an issue with XTGeoIP exports: the *.iv4 files do not get written.
The sample command I use to reproduce this on Ubuntu 20.04 is very basic: location export --format xt_geoip --directory /usr/share/xt_geoip
All IPv6 outputs are written successfully, but upon reaching IPv4 it errors with: (25, 'Inappropriate ioctl for device')
A quicker test case is to add argument `--family ipv4` so that IPv6 is hopped over and it tries IPv4 right away.
With strace, it seems to be: openat(AT_FDCWD, "/usr/share/xt_geoip/A1.iv4", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 4 fstat(4, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0 ioctl(4, TCGETS, 0x7ffd234c3260) = -1 ENOTTY (Inappropriate ioctl for device)
Please let me know if I can provide any further information.
Best regards, Valters Jansons
On Tue, Mar 29, 2022 at 5:10 PM Michael Tremer michael.tremer@ipfire.org wrote:
Hmm, this should not happen like this.
I am not even sure why this is trying to do any ioctls for terminals on the output file.
Is this reproducible with any of the other formats, too, or is it only xt_geoip?
Indeed, I could not find what seems to cause this, and have not been able to work around it.
I did not yet try other exporters - I have been focusing on xt_geoip as we use it for a system's firewall rules. But considering the IPv6 files for xt_geoip are being written correctly, it feels to me like others exporters might work. Will check.
Wanted to bring this to your attention before spending more time on it, as you maybe were aware.
-Valters
On Tue, Mar 29, 2022 at 5:16 PM Valters Jansons valter.jansons@gmail.com wrote:
On Tue, Mar 29, 2022 at 5:10 PM Michael Tremer michael.tremer@ipfire.org wrote:
Is this reproducible with any of the other formats, too, or is it only xt_geoip?
But considering the IPv6 files for xt_geoip are being written correctly, it feels to me like others exporters might work. Will check.
Indeed, `export --format ipset --directory ~/ipset` works perfectly fine, and the *v4.ipset files have content.
This seems to be xt_geoip for IPv4 only. Don't see any specific path difference between IPv6 and IPv4 though, and I doubt that Ubuntu 20.04 has something to do with it, but you never know I suppose.
Will try to dig deeper.
-Valters
I am wondering if maybe it has been broken previously but we never picked it up. Say, from ce1e53c "export: Sightly refactor export logic" in some magical case.
Michael, in any case, can you repro what I am seeing on the current release?
-Valters
Hello Valters,
Yes I can reproduce this on Debian 11 with the latest build from the Debian repository.
-Michael
On 29 Mar 2022, at 15:43, Valters Jansons valter.jansons@gmail.com wrote:
I am wondering if maybe it has been broken previously but we never picked it up. Say, from ce1e53c "export: Sightly refactor export logic" in some magical case.
Michael, in any case, can you repro what I am seeing on the current release?
-Valters
Hello Valters,
Here is the fix:
https://git.ipfire.org/?p=location/libloc.git;a=commitdiff;h=47b55e7060f6714...
The error was shown incorrectly because this was the last syscall that failed (but it was executed by Python and not us).
The real reason is that the wrong prefix was used for IPv4 addresses which resulted in a sanity check failing. However, I did not catch that very well because it is incredibly unlikely to fail.
Please confirm to me whether or not this patch resolves the problem.
I was also going to ask on this list whether people would rely on support for xt_geoip. IPFire has just replaced xt_geoip with ipset and so we don’t need the code any more which is probably why I didn’t catch this problem earlier - I have added a small test though. Writing more tests, better development documentation and returning better error codes in some places is on our TODO list, but we are incredibly lacking time and human resources to allocate to this. Can you help?
-Michael
On 29 Mar 2022, at 16:03, Michael Tremer michael.tremer@ipfire.org wrote:
Hello Valters,
Yes I can reproduce this on Debian 11 with the latest build from the Debian repository.
-Michael
On 29 Mar 2022, at 15:43, Valters Jansons valter.jansons@gmail.com wrote:
I am wondering if maybe it has been broken previously but we never picked it up. Say, from ce1e53c "export: Sightly refactor export logic" in some magical case.
Michael, in any case, can you repro what I am seeing on the current release?
-Valters
On Wed, Mar 30, 2022 at 6:36 PM Michael Tremer michael.tremer@ipfire.org wrote:
Hello Valters,
Here is the fix:
https://git.ipfire.org/?p=location/libloc.git;a=commitdiff;h=47b55e7060f6714...
The error was shown incorrectly because this was the last syscall that failed (but it was executed by Python and not us).
The real reason is that the wrong prefix was used for IPv4 addresses which resulted in a sanity check failing. However, I did not catch that very well because it is incredibly unlikely to fail.
Please confirm to me whether or not this patch resolves the problem.
Michael, thank you for finding the bug! I will rebuild with that commit applied, and try to test later today.
I was also going to ask on this list whether people would rely on support for xt_geoip. IPFire has just replaced xt_geoip with ipset and so we don’t need the code any more which is probably why I didn’t catch this problem earlier - I have added a small test though. Writing more tests, better development documentation and returning better error codes in some places is on our TODO list, but we are incredibly lacking time and human resources to allocate to this. Can you help?
Due to organizational changes, I sadly don't have as much time for external projects as I did before, and therefore cannot help out with writing test cases. If you make the decision to drop the xt_geoip format, we would of course be sad, but should be able to figure it out. If that were the case, is there any expectation for database backwards compatibility -- on how long the old versions would be able to stay operational, and work with newer database files?
-Valters
Hello,
On 30 Mar 2022, at 16:59, Valters Jansons valter.jansons@gmail.com wrote:
On Wed, Mar 30, 2022 at 6:36 PM Michael Tremer michael.tremer@ipfire.org wrote:
Hello Valters,
Here is the fix:
https://git.ipfire.org/?p=location/libloc.git;a=commitdiff;h=47b55e7060f6714...
The error was shown incorrectly because this was the last syscall that failed (but it was executed by Python and not us).
The real reason is that the wrong prefix was used for IPv4 addresses which resulted in a sanity check failing. However, I did not catch that very well because it is incredibly unlikely to fail.
Please confirm to me whether or not this patch resolves the problem.
Michael, thank you for finding the bug! I will rebuild with that commit applied, and try to test later today.
I was also going to ask on this list whether people would rely on support for xt_geoip. IPFire has just replaced xt_geoip with ipset and so we don’t need the code any more which is probably why I didn’t catch this problem earlier - I have added a small test though. Writing more tests, better development documentation and returning better error codes in some places is on our TODO list, but we are incredibly lacking time and human resources to allocate to this. Can you help?
Due to organizational changes, I sadly don't have as much time for external projects as I did before, and therefore cannot help out with writing test cases. If you make the decision to drop the xt_geoip format, we would of course be sad, but should be able to figure it out. If that were the case, is there any expectation for database backwards compatibility -- on how long the old versions would be able to stay operational, and work with newer database files?
I don’t necessarily want to drop it, but since resources are scarce, I don’t see any reason to maintain something that nobody is using. Since you are using it, and because it works, I don’t see any reason to immediately drop it.
The database format isn’t very flexible, so there is a lot of work to do if that was to be changed. We have a mechanism built in for this, but we need a very strong reason to touch this at all. This is where we get the performance from :)
So, no worries that support for xt_geoip will go away. But please keep reporting any problems that you encounter.
Best, -Michael
-Valters
On 30 Mar 2022, at 16:59, Valters Jansons valter.jansons@gmail.com wrote: On Wed, Mar 30, 2022 at 6:36 PM Michael Tremer michael.tremer@ipfire.org wrote:
Please confirm to me whether or not this patch resolves the problem.
Michael, thank you for finding the bug! I will rebuild with that commit applied, and try to test later today.
Got around to testing it this morning, and can confirm the commit resolves the export issue. Thank you!
I was also going to ask on this list whether people would rely on support for xt_geoip. IPFire has just replaced xt_geoip with ipset and so we don’t need the code any more which is probably why I didn’t catch this problem earlier - I have added a small test though. Writing more tests, better development documentation and returning better error codes in some places is on our TODO list, but we are incredibly lacking time and human resources to allocate to this. Can you help?
Due to organizational changes, I sadly don't have as much time for external projects as I did before, and therefore cannot help out with writing test cases. If you make the decision to drop the xt_geoip format, we would of course be sad, but should be able to figure it out. If that were the case, is there any expectation for database backwards compatibility -- on how long the old versions would be able to stay operational, and work with newer database files?
I don’t necessarily want to drop it, but since resources are scarce, I don’t see any reason to maintain something that nobody is using. Since you are using it, and because it works, I don’t see any reason to immediately drop it.
The database format isn’t very flexible, so there is a lot of work to do if that was to be changed. We have a mechanism built in for this, but we need a very strong reason to touch this at all. This is where we get the performance from :)
So, no worries that support for xt_geoip will go away. But please keep reporting any problems that you encounter.
That is nice to hear, and is very appreciated. Of course if we see something more, I will report.
Again - thank you!
-Valters
-
Michael Tremer -
Valters Jansons