maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Mon, Mar 29, 2021 at 9:39 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> 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",
>       ...

Yes, and that is exactly how I want it.  It is same output we have for
aria_check and
myisamchk and there it makes even more sense.

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

No, see above.

This is however not related to the patch at all.  We did not support
minimum width before
and this is something that is sometimes needed.

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

It is exactly how it i want it.  One can get +100 of these errors and
it is hopeless to read
them if things are not aligned.

<cut>

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

Yes, as I did not need left alignment and if someone wants to add it,
they are welcome to
do that.

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

It is used if someone (wrongly?) uses a negative argument.
If the code would not be there, we would probably get
a crash. Would you prefer that instead of ignoring it like now?

> > +  {
> > +    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");

 I added the test to ensure that it this issue is documented and to
ensure we do not get a crash when using negative numbers.
I never claimed that left alignment should work with the patch.
I can add to the commit message that this patch only supports right
alignment of the string for now.

Regards,
Monty