Re: [Maria-developers] f8901333a82: Add support for minimum field width for strings to my_vsnprintf()

Hi! On Mon, Mar 29, 2021 at 9:39 PM Sergei Golubchik wrote:

Hi, Michael!

On Mar 29, Michael Widenius wrote:

...

revision-id: f8901333a82 (mariadb-10.5.2-534-gf8901333a82) parent(s): a194c5cfd41 author: Michael Widenius committer: Michael Widenius 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