maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Michael!

On Mar 29, Michael Widenius wrote:
> revision-id: f8901333a82 (mariadb-10.5.2-534-gf8901333a82)
> parent(s): a194c5cfd41
> author: Michael Widenius <michael.widenius@xxxxxxxxx>
> committer: Michael Widenius <michael.widenius@xxxxxxxxx>
> timestamp: 2021-03-24 15:53:35 +0200
> message:
> 
> Add support for minimum field width for strings to my_vsnprintf()
> 
> diff --git a/mysql-test/main/almost_full.result b/mysql-test/main/almost_full.result
> index b2d7092aa51..4b5c8417412 100644
> --- a/mysql-test/main/almost_full.result
> +++ b/mysql-test/main/almost_full.result
> @@ -7,24 +7,24 @@ INSERT INTO t1 SET b=repeat('a',600);
>  ERROR HY000: The table 't1' is full
>  CHECK TABLE t1 EXTENDED;
>  Table	Op	Msg_type	Msg_text
> -test.t1	check	warning	Datafile is almost full, 65448 of 65534 used
> +test.t1	check	warning	Datafile is almost full,      65448 of      65534 used

This happens because mi_check.c has

   mi_check_print_warning(param, "Datafile is almost full, %10s of %10s used",
      ...

could you please remove explicit width specification from these
messages? this looks rather ugly in warnings. This applies both to
MyISAM and Aria.

>  test.t1	check	status	OK
>  UPDATE t1 SET b=repeat('a', 800) where a=10;
>  ERROR HY000: The table 't1' is full
> diff --git a/mysql-test/main/grant4.result b/mysql-test/main/grant4.result
> index 981bbdeddf5..ba64010f1bb 100644
> --- a/mysql-test/main/grant4.result
> +++ b/mysql-test/main/grant4.result
> @@ -187,8 +187,8 @@ connection con1;
>  check table mysqltest_db1.t1;
>  Table	Op	Msg_type	Msg_text
>  mysqltest_db1.t1	check	warning	1 client is using or hasn't closed the table properly
> -mysqltest_db1.t1	check	error	Size of indexfile is: 1024        Should be: 2048
> -mysqltest_db1.t1	check	warning	Size of datafile is: 14       Should be: 7
> +mysqltest_db1.t1	check	error	Size of indexfile is:     1024        Should be: 2048
> +mysqltest_db1.t1	check	warning	Size of datafile is:        14       Should be: 7

Same here.

>  mysqltest_db1.t1	check	error	Corrupt
>  # The below statement should fail before repairing t1.
>  # Otherwise info about such repair will be missing from its result-set.
> diff --git a/mysql-test/main/myisam_recover.result b/mysql-test/main/myisam_recover.result
> index da96682186c..d3c385bc49b 100644
> --- a/mysql-test/main/myisam_recover.result
> +++ b/mysql-test/main/myisam_recover.result
> @@ -69,9 +69,9 @@ flush table t1;
>  # check table is needed to mark the table as crashed.
>  check table t1;
>  Table	Op	Msg_type	Msg_text
> -test.t1	check	warning	Size of datafile is: 42       Should be: 21
> -test.t1	check	error	Record-count is not ok; is 6   Should be: 3
> -test.t1	check	warning	Found 6 key parts. Should be: 3
> +test.t1	check	warning	Size of datafile is:        42       Should be: 21
> +test.t1	check	error	Record-count is not ok; is          6   Should be: 3
> +test.t1	check	warning	Found          6 key parts. Should be: 3

Same. Must be in many messages in mi_check.c and ma_check.c

>  test.t1	check	error	Corrupt
>  #
> diff --git a/strings/my_vsnprintf.c b/strings/my_vsnprintf.c
> index a2e3f9b738d..1621db2e412 100644
> --- a/strings/my_vsnprintf.c
> +++ b/strings/my_vsnprintf.c
> @@ -224,12 +224,27 @@ static char *backtick_string(CHARSET_INFO *cs, char *to, const char *end,
>  */
>  
>  static char *process_str_arg(CHARSET_INFO *cs, char *to, const char *end,
> -                             size_t width, char *par, uint print_type,
> -                             my_bool nice_cut)
> +                             longlong length_arg, size_t width, char *par,
> +                             uint print_type, my_bool nice_cut)
>  {
>    int well_formed_error;
>    uint dots= 0;
>    size_t plen, left_len= (size_t) (end - to) + 1, slen=0;
> +  my_bool left_fill= 1;
> +  size_t length;
> +
> +  /*
> +    The sign of the length argument specific the string should be right
> +    or left adjusted
> +  */
> +  if (length_arg < 0)

You're trying to support left alignment, but you have yourself added a
test that shows that it doesn't work.

So this code is dead, never used. See below.

> +  {
> +    length= -length_arg;
> +    left_fill= 0;
> +  }
> +  else
> +    length= (size_t) length_arg;
> +
>    if (!par)
>      par = (char*) "(null)";
>  
> @@ -693,7 +720,8 @@ size_t my_vsnprintf_ex(CHARSET_INFO *cs, char *to, size_t n,
>      if (*fmt == 's' || *fmt == 'T')			/* String parameter */
>      {
>        reg2 char *par= va_arg(ap, char *);
> -      to= process_str_arg(cs, to, end, width, par, print_type, (*fmt == 'T'));
> +      to= process_str_arg(cs, to, end, (longlong) length, width, par,

see? length is unsigned type size_t, the sign before length is ignored
by the parser. Whether you cast length to a signed type or not and
whether you checks for length < 0 or not, left alignment still won't
work. That's why you have added this test:

> +  test1("Negative width is ignored for strings <   x> <    y>",
> +        "Negative width is ignored for strings <%-4s> <%-5s>", "x", "y");

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx