put_info(..,INFO_ERROR,...); return false; doesn't exit in get_one_option()

[Andrew Hutchings]

In drizzle.cc function get_one_option() if put_inf0(..,INFO_ERROR,...); return false; is used it will not exit.

To reproduce this try:

drizzle --delimiter=\\

from drizzle.cc:

      /* Check that delimiter does not contain a backslash */
      if (!strstr(argument, "\\"))
      {
        strncpy(delimiter, argument, sizeof(delimiter) - 1);
      }
      else
      {
        put_info(_("DELIMITER cannot contain a backslash character"),
                 INFO_ERROR,0,0);
        return false;
      }

Possible fixes are an exit() in put_info() or to handle the return condition for get_one_option() in mysys/my_get_opt.c

[Padraig O'Sullivan (posulliv)]

I was able to reproduce this:

$ ../client/drizzle --user=root --port=9306 --delimiter=\\
ERROR:
DELIMITER cannot contain a backslash character
Welcome to the Drizzle client.. Commands end with ; or \g.
Your Drizzle connection id is 3
Server version: 7.0.0 Source distribution

Type 'help;' or '\h' for help. Type '\c' to clear the buffer.

drizzle>

Thanks, Padraig.

[Andrew Ettinger (sillydeveloper)]

Would it be considered unexpected behavior if the delimiter was simply reset to normal with a warning that \\ is not allowed? It's more robust, but not necessarily correct, behavior.

[Jay Pipes (jaypipes)]

Well, that would fix the bug :) But, I don't think it's correct behaviour. A real fix would be on the get_one_option() loop which validates the return false in some way and exit()s. Andrew, feel like tackling this one? Shouldn't be a difficult one... I'll go ahead and add it to the low-hanging-fruit milestone. Feel free to assign it to yourself if you want to. You can find me on IRC #drizzle if you run into any issues or want to discuss it further?

Cheers!

Jay

[Jay Pipes (jaypipes)]

Setting to low since this does not lose data and also only affects a tiny minority of users. Low hanging fruit will allow folks to pick this up if they want to...

[Jay Pipes (jaypipes)]

Hi Andrew! Just pinging you on this. Any change in the bug?

[Andrew Hutchings (linuxjedi)]

Hey guys,

OK, so I have a patch almost ready for this, but I have a question. At the moment in all apps when get_one_option() returns false it means error, but if an option doesn't match it returns 0, which is also equal to false ;)

An example would be --help in drizzled which isn't caught by get_one_option so returns 0.

So, do I:

1. make errors 'true'
2. no match for current option 'true'
3. change it to an int return and have multiple return values (something like 0 = no value (OK), -1 = error)?

I had a quick flick through the coding standards and couldn't see a correct answer.

[Brian Aker (brianaker)]

Hi!

On Mar 24, 2010, at 7:00 PM, LinuxJedi wrote:

> 3. change it to an int return and have multiple return values
> (something like 0 = no value (OK), -1 = error)?

Have it return an ENUM for possible values and flip through a case on
the returns.

It is awesome that you are fixing this :)

Cheers,
 -Brian

[Andrew Hutchings (linuxjedi)]

Hey Brian,

On Thu, 2010-03-25 at 02:18 +0000, Brian Aker wrote:
> On Mar 24, 2010, at 7:00 PM, LinuxJedi wrote:
>
> > 3. change it to an int return and have multiple return values
> > (something like 0 = no value (OK), -1 = error)?
>
> Have it return an ENUM for possible values and flip through a case on
> the returns.

Doh! For some reason the most obvious solution did not occur to me at
2AM :)

> It is awesome that you are fixing this :)

No worries, I was in the neighbourhood and noticed it still
lingering. :)

Many thanks!

Kind Regards
--
Andrew Hutchings - LinuxJedi - http://www.linuxjedi.co.uk/
Certified MySQL Developer, DBA & Cluster DBA
Zend PHP5 Certified Engineer