A fix for skytraq "cannot set new location" bug

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

A fix for skytraq "cannot set new location" bug

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
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                          <[hidden email]>
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code

skytraq-fix.patch (297 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A fix for skytraq "cannot set new location" bug

Robert Lipe-4
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 <[hidden email]> wrote:
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                          <[hidden email]>
Katedra Aplikované Matematiky, MFF UK               tel.: <a href="tel:%2B420%20739%20464%20167" value="+420739464167">+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
[hidden email]
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
Reply | Threaded
Open this post in threaded view
|

Re: A fix for skytraq "cannot set new location" bug

tsteven4-2
I think this will work for everyone:
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;
   }


On 2/29/2016 1:31 PM, Robert Lipe wrote:
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 <[hidden email]> wrote:
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                          <[hidden email]>
Katedra Aplikované Matematiky, MFF UK               tel.: <a moz-do-not-send="true" href="tel:%2B420%20739%20464%20167" value="+420739464167">+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
[hidden email]
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
[hidden email]
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
Reply | Threaded
Open this post in threaded view
|

Re: A fix for skytraq "cannot set new location" bug

Robert Lipe-4
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?

On Mon, Feb 29, 2016 at 5:23 PM, tsteven4 <[hidden email]> wrote:
I think this will work for everyone:
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;
   }


On 2/29/2016 1:31 PM, Robert Lipe wrote:
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 <[hidden email]> wrote:
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                          <[hidden email][hidden email]>
Katedra Aplikované Matematiky, MFF UK               tel.: <a href="tel:%2B420%20739%20464%20167" value="+420739464167" target="_blank">+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
[hidden email]
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
[hidden email]
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
Reply | Threaded
Open this post in threaded view
|

Re: A fix for skytraq "cannot set new location" bug

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.

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...

On Wed, Mar 02, 2016 at 12:51:34PM -0600, Robert Lipe wrote:

> 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?
>
> On Mon, Feb 29, 2016 at 5:23 PM, tsteven4 <[hidden email]> wrote:
>
> > I think this will work for everyone:
> >
> > 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;
> >    }
> >
> >
> >
> > On 2/29/2016 1:31 PM, Robert Lipe wrote:
> >
> > 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 <[hidden email]>
> > wrote:
> >
> >> 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                          <
> >> <[hidden email]>[hidden email]>
> >> 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
> >> [hidden email]
> >> 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.orgGpsbabel-code@...://lists.sourceforge.net/lists/listinfo/gpsbabel-code
> >
> >
> >
--
S pozdravem Ladislav Láska                          <[hidden email]>
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code

skytraq-fix-v2.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: A fix for skytraq "cannot set new location" bug

Robert Lipe-4


On Wed, Mar 2, 2016 at 4:01 PM, Ladislav Laska <[hidden email]> wrote:
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.

 

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.

 
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
 

On Wed, Mar 02, 2016 at 12:51:34PM -0600, Robert Lipe wrote:
> 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?
>
> On Mon, Feb 29, 2016 at 5:23 PM, tsteven4 <[hidden email]> wrote:
>
> > I think this will work for everyone:
> >
> > 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;
> >    }
> >
> >
> >
> > On 2/29/2016 1:31 PM, Robert Lipe wrote:
> >
> > 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 <[hidden email]>
> > wrote:
> >
> >> 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                          <
> >> <[hidden email]>[hidden email]>
> >> 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
> >> [hidden email]
> >> 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://[hidden email]://lists.sourceforge.net/lists/listinfo/gpsbabel-code
> >
> >
> >

--
S pozdravem Ladislav Láska                          <[hidden email]>
Katedra Aplikované Matematiky, MFF UK               tel.: <a href="tel:%2B420%20739%20464%20167" value="+420739464167">+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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
Reply | Threaded
Open this post in threaded view
|

Re: A fix for skytraq "cannot set new location" bug

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.

On Wed, Mar 02, 2016 at 05:05:10PM -0600, Robert Lipe wrote:

> On Wed, Mar 2, 2016 at 4:01 PM, Ladislav Laska <[hidden email]>
> wrote:
>
> > 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.
>
>
>
> >
> > 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.
>
>
>
> > 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
>
>
> >
> > On Wed, Mar 02, 2016 at 12:51:34PM -0600, Robert Lipe wrote:
> > > 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?
> > >
> > > On Mon, Feb 29, 2016 at 5:23 PM, tsteven4 <[hidden email]> wrote:
> > >
> > > > I think this will work for everyone:
> > > >
> > > > 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;
> > > >    }
> > > >
> > > >
> > > >
> > > > On 2/29/2016 1:31 PM, Robert Lipe wrote:
> > > >
> > > > 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 <
> > [hidden email]>
> > > > wrote:
> > > >
> > > >> 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                          <
> > > >> <[hidden email]>[hidden email]>
> > > >> 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
> > > >> [hidden email]
> > > >> 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.orgGpsbabel-code
> > @lists.sourceforge.nethttps://
> > lists.sourceforge.net/lists/listinfo/gpsbabel-code
> > > >
> > > >
> > > >
> >
> > --
> > S pozdravem Ladislav Láska                          <[hidden email]
> > >
> > Katedra Aplikované Matematiky, MFF UK               tel.: +420 739 464 167
> >

--
S pozdravem Ladislav Láska                          <[hidden email]>
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
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
Reply | Threaded
Open this post in threaded view
|

Re: A fix for skytraq "cannot set new location" bug

Konrad Gräfe
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.

Am Do, 03.03.2016 um 09:16 schrieb 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.
>
> On Wed, Mar 02, 2016 at 05:05:10PM -0600, Robert Lipe wrote:
>> On Wed, Mar 2, 2016 at 4:01 PM, Ladislav Laska <[hidden email]>
>> wrote:
>>
>>> 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.
>>
>>
>>
>>>
>>> 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.
>>
>>
>>
>>> 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
>>
>>
>>>
>>> On Wed, Mar 02, 2016 at 12:51:34PM -0600, Robert Lipe wrote:
>>>> 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?
>>>>
>>>> On Mon, Feb 29, 2016 at 5:23 PM, tsteven4 <[hidden email]> wrote:
>>>>
>>>>> I think this will work for everyone:
>>>>>
>>>>> 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;
>>>>>    }
>>>>>
>>>>>
>>>>>
>>>>> On 2/29/2016 1:31 PM, Robert Lipe wrote:
>>>>>
>>>>> 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 <
>>> [hidden email]>
>>>>> wrote:
>>>>>
>>>>>> 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                          <
>>>>>> <[hidden email]>[hidden email]>
>>>>>> 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
>>>>>> [hidden email]
>>>>>> 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.orgGpsbabel-code
>>> @lists.sourceforge.nethttps://
>>> lists.sourceforge.net/lists/listinfo/gpsbabel-code
>>>>>
>>>>>
>>>>>
>>>
>>> --
>>> S pozdravem Ladislav Láska                          <[hidden email]
>>>>
>>> Katedra Aplikované Matematiky, MFF UK               tel.: +420 739 464 167
>>>
>

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140
_______________________________________________
Gpsbabel-code mailing list  http://www.gpsbabel.org
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code