Discussion:
[Gpsbabel-code] A fix for skytraq "cannot set new location" bug
Ladislav Laska
2016-02-29 18:28:57 UTC
Permalink
Hello!

I just noticed that on my skytraq, I can't use configlog=, as it always tries to
set new location beforehand, even if I didn't specify any. This worked before
and is caused by this patch:

https://github.com/gpsbabel/gpsbabel/commit/c927faac0a6304ee650d903cf8bd19518f416900

(I found the reference here: http://comments.gmane.org/gmane.comp.hardware.gps.gpsbabel.general/8263 )

It looks that for some reason, the code now checks for pointer only, not if
there's any data in it. I've added the dereference check, which will be true
only if the string is non-zero length or NULL pointer. This should work as
expected.

I'm not really sure how to run tests, as I've tried make check, with several
modifications, and never noticed a fault (and haven't found docs online on how
to do it, but I haven't invested too much time to check for this one line
patch).

Thoughts?
--
S pozdravem Ladislav Láska <***@kam.mff.cuni.cz>
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
Robert Lipe
2016-02-29 20:31:34 UTC
Permalink
Hi, and welcome.

So we don't keep going back and forth on this, Ladislav and Konrad, can you
please work together that finds a solution that works for both of you?

Thanx,
RJL
Post by Ladislav Laska
Hello!
I just noticed that on my skytraq, I can't use configlog=, as it always tries to
set new location beforehand, even if I didn't specify any. This worked before
https://github.com/gpsbabel/gpsbabel/commit/c927faac0a6304ee650d903cf8bd19518f416900
http://comments.gmane.org/gmane.comp.hardware.gps.gpsbabel.general/8263 )
It looks that for some reason, the code now checks for pointer only, not if
there's any data in it. I've added the dereference check, which will be true
only if the string is non-zero length or NULL pointer. This should work as
expected.
I'm not really sure how to run tests, as I've tried make check, with several
modifications, and never noticed a fault (and haven't found docs online on how
to do it, but I haven't invested too much time to check for this one line
patch).
Thoughts?
--
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.org
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
tsteven4
2016-02-29 23:23:52 UTC
Permalink
diff --git a/skytraq.cc b/skytraq.cc
index 3130bf2..6516f85 100644
--- a/skytraq.cc
+++ b/skytraq.cc
@@ -1304,12 +1304,12 @@ skytraq_read(void)
{
int dlbaud;
- if (opt_set_location) {
+ if (opt_set_location && *opt_set_location) {
skytraq_set_location();
return;
}
- if (opt_configure_logging) {
+ if (opt_configure_logging && *opt_configure_logging) {
skytraq_configure_logging();
return;
}
Hi, and welcome.
So we don't keep going back and forth on this, Ladislav and Konrad,
can you please work together that finds a solution that works for both
of you?
Thanx,
RJL
On Mon, Feb 29, 2016 at 12:28 PM, Ladislav Laska
Hello!
I just noticed that on my skytraq, I can't use configlog=, as it always tries to
set new location beforehand, even if I didn't specify any. This worked before
https://github.com/gpsbabel/gpsbabel/commit/c927faac0a6304ee650d903cf8bd19518f416900
http://comments.gmane.org/gmane.comp.hardware.gps.gpsbabel.general/8263 )
It looks that for some reason, the code now checks for pointer only, not if
there's any data in it. I've added the dereference check, which will be true
only if the string is non-zero length or NULL pointer. This should work as
expected.
I'm not really sure how to run tests, as I've tried make check, with several
modifications, and never noticed a fault (and haven't found docs online on how
to do it, but I haven't invested too much time to check for this one line
patch).
Thoughts?
--
S pozdravem Ladislav Láska
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
<tel:%2B420%20739%20464%20167>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.org
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.org
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
Robert Lipe
2016-03-02 18:51:34 UTC
Permalink
That *seems* like it should be right, but wasn't that one of the iterations
that didn't work for someone?

Ladisalav, Konrad, can you please comment?
diff --git a/skytraq.cc b/skytraq.cc
index 3130bf2..6516f85 100644
--- a/skytraq.cc
+++ b/skytraq.cc
@@ -1304,12 +1304,12 @@ skytraq_read(void)
{
int dlbaud;
- if (opt_set_location) {
+ if (opt_set_location && *opt_set_location) {
skytraq_set_location();
return;
}
- if (opt_configure_logging) {
+ if (opt_configure_logging && *opt_configure_logging) {
skytraq_configure_logging();
return;
}
Hi, and welcome.
So we don't keep going back and forth on this, Ladislav and Konrad, can
you please work together that finds a solution that works for both of you?
Thanx,
RJL
Post by Ladislav Laska
Hello!
I just noticed that on my skytraq, I can't use configlog=, as it always tries to
set new location beforehand, even if I didn't specify any. This worked before
https://github.com/gpsbabel/gpsbabel/commit/c927faac0a6304ee650d903cf8bd19518f416900
http://comments.gmane.org/gmane.comp.hardware.gps.gpsbabel.general/8263 )
It looks that for some reason, the code now checks for pointer only, not if
there's any data in it. I've added the dereference check, which will be true
only if the string is non-zero length or NULL pointer. This should work as
expected.
I'm not really sure how to run tests, as I've tried make check, with several
modifications, and never noticed a fault (and haven't found docs online on how
to do it, but I haven't invested too much time to check for this one line
patch).
Thoughts?
--
S pozdravem Ladislav Láska <
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.org
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Ladislav Laska
2016-03-02 22:01:14 UTC
Permalink
Well, I've taken another look. What tsteven4 proposes is mostly right.

It looks like opt_* variables are always initialized (at least by some default
value), so the if (opt_*) condition does nothing at all.

As both of them are initialized to empty string, the *opt_set_location does
call the function if non-empty string is passed as parameter.

I also noticed, that skytraq_configure_logging does some strange stuff when
called with empty string, and skytraq_set_location does no checking on it's
input at all.

While I was at it, I made a bit bigger patch, that fixes the issue originally
submitted, and adds some input checking.

I also removed the non-necessary ifs, testing for opt_* == NULL, and replaced
them by asserts, just in case. Feel free to remove the asserts from the code, if
you don't like it (I'm unsure about the assert policy, but I've seen it used in
other places). As a bonus, these functions will now complain about wrong
parameters. Unfortunately, if you pass "configlog=", it's considered valid (as
it's the same as default).

I've tested configlog on my skytraq and it seems to work fine. Unforunately,
mine does not support target, so I can't test that.

Note that if we think patch originally removing "*opt_..." checking fixed
something, this may unfix it. I don't think it's something useful, as it might be
some side effect like writing junk to target on every gpsbabel invocation...
Post by Robert Lipe
That *seems* like it should be right, but wasn't that one of the iterations
that didn't work for someone?
Ladisalav, Konrad, can you please comment?
diff --git a/skytraq.cc b/skytraq.cc
index 3130bf2..6516f85 100644
--- a/skytraq.cc
+++ b/skytraq.cc
@@ -1304,12 +1304,12 @@ skytraq_read(void)
{
int dlbaud;
- if (opt_set_location) {
+ if (opt_set_location && *opt_set_location) {
skytraq_set_location();
return;
}
- if (opt_configure_logging) {
+ if (opt_configure_logging && *opt_configure_logging) {
skytraq_configure_logging();
return;
}
Hi, and welcome.
So we don't keep going back and forth on this, Ladislav and Konrad, can
you please work together that finds a solution that works for both of you?
Thanx,
RJL
Post by Ladislav Laska
Hello!
I just noticed that on my skytraq, I can't use configlog=, as it always tries to
set new location beforehand, even if I didn't specify any. This worked before
https://github.com/gpsbabel/gpsbabel/commit/c927faac0a6304ee650d903cf8bd19518f416900
http://comments.gmane.org/gmane.comp.hardware.gps.gpsbabel.general/8263 )
It looks that for some reason, the code now checks for pointer only, not if
there's any data in it. I've added the dereference check, which will be true
only if the string is non-zero length or NULL pointer. This should work as
expected.
I'm not really sure how to run tests, as I've tried make check, with several
modifications, and never noticed a fault (and haven't found docs online on how
to do it, but I haven't invested too much time to check for this one line
patch).
Thoughts?
--
S pozdravem Ladislav Láska <
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.org
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
--
S pozdravem Ladislav Láska <***@kam.mff.cuni.cz>
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
Robert Lipe
2016-03-02 23:05:10 UTC
Permalink
Post by Ladislav Laska
Well, I've taken another look. What tsteven4 proposes is mostly right.
It looks like opt_* variables are always initialized (at least by some default
value), so the if (opt_*) condition does nothing at all.
As both of them are initialized to empty string, the *opt_set_location does
call the function if non-empty string is passed as parameter.
I also noticed, that skytraq_configure_logging does some strange stuff when
called with empty string, and skytraq_set_location does no checking on it's
input at all.
Agreed that's distasteful.
Post by Ladislav Laska
While I was at it, I made a bit bigger patch, that fixes the issue originally
submitted, and adds some input checking.
I also removed the non-necessary ifs, testing for opt_* == NULL, and replaced
them by asserts, just in case. Feel free to remove the asserts from the code, if
Since asserts() can be compiled away and usually don't generate any useful
info to the user, we tend to prefer is_fatal() or explicit calls to Fatal()
for runtime errors as opposed to compile-time violations.

Yes, we do have some asserts() in lesser-used modules. I consider those
bugs.
Post by Ladislav Laska
you don't like it (I'm unsure about the assert policy, but I've seen it used in
other places). As a bonus, these functions will now complain about wrong
parameters. Unfortunately, if you pass "configlog=", it's considered valid (as
it's the same as default).
I've tested configlog on my skytraq and it seems to work fine.
Unforunately,
mine does not support target, so I can't test that.
Note that if we think patch originally removing "*opt_..." checking fixed
something, this may unfix it. I don't think it's something useful, as it might be
some side effect like writing junk to target on every gpsbabel
invocation...
Konrad seemed to think it did. That's why I'm trying to get to the bottom
of this instead of just playing patch ping-pong.

Konrad?

RJL
Post by Ladislav Laska
Post by Robert Lipe
That *seems* like it should be right, but wasn't that one of the
iterations
Post by Robert Lipe
that didn't work for someone?
Ladisalav, Konrad, can you please comment?
diff --git a/skytraq.cc b/skytraq.cc
index 3130bf2..6516f85 100644
--- a/skytraq.cc
+++ b/skytraq.cc
@@ -1304,12 +1304,12 @@ skytraq_read(void)
{
int dlbaud;
- if (opt_set_location) {
+ if (opt_set_location && *opt_set_location) {
skytraq_set_location();
return;
}
- if (opt_configure_logging) {
+ if (opt_configure_logging && *opt_configure_logging) {
skytraq_configure_logging();
return;
}
Hi, and welcome.
So we don't keep going back and forth on this, Ladislav and Konrad, can
you please work together that finds a solution that works for both of
you?
Post by Robert Lipe
Thanx,
RJL
On Mon, Feb 29, 2016 at 12:28 PM, Ladislav Laska <
Post by Ladislav Laska
Hello!
I just noticed that on my skytraq, I can't use configlog=, as it
always
Post by Robert Lipe
Post by Ladislav Laska
tries to
set new location beforehand, even if I didn't specify any. This worked before
https://github.com/gpsbabel/gpsbabel/commit/c927faac0a6304ee650d903cf8bd19518f416900
http://comments.gmane.org/gmane.comp.hardware.gps.gpsbabel.general/8263 )
Post by Robert Lipe
Post by Ladislav Laska
It looks that for some reason, the code now checks for pointer only,
not
Post by Robert Lipe
Post by Ladislav Laska
if
there's any data in it. I've added the dereference check, which will
be
Post by Robert Lipe
Post by Ladislav Laska
true
only if the string is non-zero length or NULL pointer. This should
work as
Post by Robert Lipe
Post by Ladislav Laska
expected.
I'm not really sure how to run tests, as I've tried make check, with several
modifications, and never noticed a fault (and haven't found docs
online
Post by Robert Lipe
Post by Ladislav Laska
on how
to do it, but I haven't invested too much time to check for this one
line
Post by Robert Lipe
Post by Ladislav Laska
patch).
Thoughts?
--
S pozdravem Ladislav Láska <
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
------------------------------------------------------------------------------
Post by Robert Lipe
Post by Ladislav Laska
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.org
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
------------------------------------------------------------------------------
Post by Robert Lipe
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Post by Robert Lipe
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.orgGpsbabel-code
@lists.sourceforge.nethttps://
lists.sourceforge.net/lists/listinfo/gpsbabel-code
--
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
Ladislav Laska
2016-03-03 08:16:21 UTC
Permalink
Sure. I agree that playing ping pong is bad. I will also remove the asserts and
submit a new patch.

I also looked at the code at time the original patch was included.

skytraq_set_location was definitely broken by the patch. Before the patch, it
would do nothing if not specified, and possibly write junk if non-valid
parameters were passed. After the patch it would write junk even if the
parameter was not specified. I guess this was by mistake, fixing some problem in
skytraq_configure_logging.

As for the configure_logging, it still introduces a bug. When configlog is empty
string, it still sends empty message (well, the message exactly as specified in
the function) to the device, resetting the configuration to some default.

I would still wait for Konrad, as the bug has been there for some time, and can
wait a bit more. But considering the commit message mentions some tests, maybe
the tests were wrong. I'm still not seeing the problem possibly fixed by that
patch.
Post by Robert Lipe
Post by Ladislav Laska
Well, I've taken another look. What tsteven4 proposes is mostly right.
It looks like opt_* variables are always initialized (at least by some default
value), so the if (opt_*) condition does nothing at all.
As both of them are initialized to empty string, the *opt_set_location does
call the function if non-empty string is passed as parameter.
I also noticed, that skytraq_configure_logging does some strange stuff when
called with empty string, and skytraq_set_location does no checking on it's
input at all.
Agreed that's distasteful.
Post by Ladislav Laska
While I was at it, I made a bit bigger patch, that fixes the issue originally
submitted, and adds some input checking.
I also removed the non-necessary ifs, testing for opt_* == NULL, and replaced
them by asserts, just in case. Feel free to remove the asserts from the code, if
Since asserts() can be compiled away and usually don't generate any useful
info to the user, we tend to prefer is_fatal() or explicit calls to Fatal()
for runtime errors as opposed to compile-time violations.
Yes, we do have some asserts() in lesser-used modules. I consider those
bugs.
Post by Ladislav Laska
you don't like it (I'm unsure about the assert policy, but I've seen it used in
other places). As a bonus, these functions will now complain about wrong
parameters. Unfortunately, if you pass "configlog=", it's considered valid (as
it's the same as default).
I've tested configlog on my skytraq and it seems to work fine. Unforunately,
mine does not support target, so I can't test that.
Note that if we think patch originally removing "*opt_..." checking fixed
something, this may unfix it. I don't think it's something useful, as it might be
some side effect like writing junk to target on every gpsbabel invocation...
Konrad seemed to think it did. That's why I'm trying to get to the bottom
of this instead of just playing patch ping-pong.
Konrad?
RJL
Post by Ladislav Laska
Post by Robert Lipe
That *seems* like it should be right, but wasn't that one of the
iterations
Post by Robert Lipe
that didn't work for someone?
Ladisalav, Konrad, can you please comment?
diff --git a/skytraq.cc b/skytraq.cc
index 3130bf2..6516f85 100644
--- a/skytraq.cc
+++ b/skytraq.cc
@@ -1304,12 +1304,12 @@ skytraq_read(void)
{
int dlbaud;
- if (opt_set_location) {
+ if (opt_set_location && *opt_set_location) {
skytraq_set_location();
return;
}
- if (opt_configure_logging) {
+ if (opt_configure_logging && *opt_configure_logging) {
skytraq_configure_logging();
return;
}
Hi, and welcome.
So we don't keep going back and forth on this, Ladislav and Konrad, can
you please work together that finds a solution that works for both of
you?
Post by Robert Lipe
Thanx,
RJL
On Mon, Feb 29, 2016 at 12:28 PM, Ladislav Laska <
Post by Ladislav Laska
Hello!
I just noticed that on my skytraq, I can't use configlog=, as it
always
Post by Robert Lipe
Post by Ladislav Laska
tries to
set new location beforehand, even if I didn't specify any. This worked before
https://github.com/gpsbabel/gpsbabel/commit/c927faac0a6304ee650d903cf8bd19518f416900
http://comments.gmane.org/gmane.comp.hardware.gps.gpsbabel.general/8263 )
Post by Robert Lipe
Post by Ladislav Laska
It looks that for some reason, the code now checks for pointer only,
not
Post by Robert Lipe
Post by Ladislav Laska
if
there's any data in it. I've added the dereference check, which will
be
Post by Robert Lipe
Post by Ladislav Laska
true
only if the string is non-zero length or NULL pointer. This should
work as
Post by Robert Lipe
Post by Ladislav Laska
expected.
I'm not really sure how to run tests, as I've tried make check, with several
modifications, and never noticed a fault (and haven't found docs
online
Post by Robert Lipe
Post by Ladislav Laska
on how
to do it, but I haven't invested too much time to check for this one
line
Post by Robert Lipe
Post by Ladislav Laska
patch).
Thoughts?
--
S pozdravem Ladislav Láska <
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
------------------------------------------------------------------------------
Post by Robert Lipe
Post by Ladislav Laska
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.org
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
------------------------------------------------------------------------------
Post by Robert Lipe
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Post by Robert Lipe
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.orgGpsbabel-code
@lists.sourceforge.nethttps://
lists.sourceforge.net/lists/listinfo/gpsbabel-code
--
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
--
S pozdravem Ladislav Láska <***@kam.mff.cuni.cz>
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
Konrad Gräfe
2016-03-14 10:00:26 UTC
Permalink
Sorry for the delay, I was in vacations.

When the application crashed on my machine, both 'opt_set_location' and
'opt_configure_logging' were definitely not initialized.

If they are supposed to be initialized, the bug I tried to fix must be
somewhere else.

However, checking for both, nullness and emptiness, should work for me too.
Post by Ladislav Laska
Sure. I agree that playing ping pong is bad. I will also remove the asserts and
submit a new patch.
I also looked at the code at time the original patch was included.
skytraq_set_location was definitely broken by the patch. Before the patch, it
would do nothing if not specified, and possibly write junk if non-valid
parameters were passed. After the patch it would write junk even if the
parameter was not specified. I guess this was by mistake, fixing some problem in
skytraq_configure_logging.
As for the configure_logging, it still introduces a bug. When configlog is empty
string, it still sends empty message (well, the message exactly as specified in
the function) to the device, resetting the configuration to some default.
I would still wait for Konrad, as the bug has been there for some time, and can
wait a bit more. But considering the commit message mentions some tests, maybe
the tests were wrong. I'm still not seeing the problem possibly fixed by that
patch.
Post by Robert Lipe
Post by Ladislav Laska
Well, I've taken another look. What tsteven4 proposes is mostly right.
It looks like opt_* variables are always initialized (at least by some default
value), so the if (opt_*) condition does nothing at all.
As both of them are initialized to empty string, the *opt_set_location does
call the function if non-empty string is passed as parameter.
I also noticed, that skytraq_configure_logging does some strange stuff when
called with empty string, and skytraq_set_location does no checking on it's
input at all.
Agreed that's distasteful.
Post by Ladislav Laska
While I was at it, I made a bit bigger patch, that fixes the issue originally
submitted, and adds some input checking.
I also removed the non-necessary ifs, testing for opt_* == NULL, and replaced
them by asserts, just in case. Feel free to remove the asserts from the code, if
Since asserts() can be compiled away and usually don't generate any useful
info to the user, we tend to prefer is_fatal() or explicit calls to Fatal()
for runtime errors as opposed to compile-time violations.
Yes, we do have some asserts() in lesser-used modules. I consider those
bugs.
Post by Ladislav Laska
you don't like it (I'm unsure about the assert policy, but I've seen it used in
other places). As a bonus, these functions will now complain about wrong
parameters. Unfortunately, if you pass "configlog=", it's considered valid (as
it's the same as default).
I've tested configlog on my skytraq and it seems to work fine. Unforunately,
mine does not support target, so I can't test that.
Note that if we think patch originally removing "*opt_..." checking fixed
something, this may unfix it. I don't think it's something useful, as it might be
some side effect like writing junk to target on every gpsbabel invocation...
Konrad seemed to think it did. That's why I'm trying to get to the bottom
of this instead of just playing patch ping-pong.
Konrad?
RJL
Post by Ladislav Laska
Post by Robert Lipe
That *seems* like it should be right, but wasn't that one of the
iterations
Post by Robert Lipe
that didn't work for someone?
Ladisalav, Konrad, can you please comment?
diff --git a/skytraq.cc b/skytraq.cc
index 3130bf2..6516f85 100644
--- a/skytraq.cc
+++ b/skytraq.cc
@@ -1304,12 +1304,12 @@ skytraq_read(void)
{
int dlbaud;
- if (opt_set_location) {
+ if (opt_set_location && *opt_set_location) {
skytraq_set_location();
return;
}
- if (opt_configure_logging) {
+ if (opt_configure_logging && *opt_configure_logging) {
skytraq_configure_logging();
return;
}
Hi, and welcome.
So we don't keep going back and forth on this, Ladislav and Konrad, can
you please work together that finds a solution that works for both of
you?
Post by Robert Lipe
Thanx,
RJL
On Mon, Feb 29, 2016 at 12:28 PM, Ladislav Laska <
Post by Ladislav Laska
Hello!
I just noticed that on my skytraq, I can't use configlog=, as it
always
Post by Robert Lipe
Post by Ladislav Laska
tries to
set new location beforehand, even if I didn't specify any. This worked before
https://github.com/gpsbabel/gpsbabel/commit/c927faac0a6304ee650d903cf8bd19518f416900
http://comments.gmane.org/gmane.comp.hardware.gps.gpsbabel.general/8263 )
Post by Robert Lipe
Post by Ladislav Laska
It looks that for some reason, the code now checks for pointer only,
not
Post by Robert Lipe
Post by Ladislav Laska
if
there's any data in it. I've added the dereference check, which will
be
Post by Robert Lipe
Post by Ladislav Laska
true
only if the string is non-zero length or NULL pointer. This should
work as
Post by Robert Lipe
Post by Ladislav Laska
expected.
I'm not really sure how to run tests, as I've tried make check, with several
modifications, and never noticed a fault (and haven't found docs
online
Post by Robert Lipe
Post by Ladislav Laska
on how
to do it, but I haven't invested too much time to check for this one
line
Post by Robert Lipe
Post by Ladislav Laska
patch).
Thoughts?
--
S pozdravem Ladislav Láska <
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
------------------------------------------------------------------------------
Post by Robert Lipe
Post by Ladislav Laska
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.org
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
------------------------------------------------------------------------------
Post by Robert Lipe
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Post by Robert Lipe
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.orgGpsbabel-code
@lists.sourceforge.nethttps://
lists.sourceforge.net/lists/listinfo/gpsbabel-code
--
Katedra Aplikované Matematiky, MFF UK tel.: +420 739 464 167
Loading...