MDEV-36269: Improve error handling for `source` command #4455

MohamedM216 · 2025-11-22T08:05:05Z

The Jira issue number for this PR is: MDEV-36269

Description

This PR provides a descriptive error message when users accidentally use a directory with the source command instead of getting an ambiguous error message: Can't initialize batch_readline.

Release Notes

How can this PR be tested?

Create a local branch tracking the upstream branch 10.6.

git checkout -b 10.6 upstream/10.6

Then build and start the server as mentioned here

And after running these commands you will get the desired output (descriptive error message):

MariaDB [(none)]> source /tmp/test_source_dir;
ERROR: '/tmp/test_source_dir' is a directory, not a file
MariaDB [(none)]> source /tmp/nonexistent_file;
ERROR: Failed to open file '/tmp/nonexistent_file', error: 2
MariaDB [(none)]> exit
Bye

Basing the PR against the correct MariaDB version

This is a new feature or a refactoring, and the PR is based against the main branch.
This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

CLAassistant · 2025-11-22T08:05:12Z

All committers have signed the CLA.

MohamedM216 · 2025-11-22T14:06:41Z

The only unit test that fails here: buildbot/amd64-windows passes successfully locally.

grooverdan · 2025-11-24T00:33:55Z

client/mysql.cc

+    char buff[FN_REFLEN+60];
+    sprintf(buff, "'%s' is a directory, not a file", source_name);
+    return put_info(buff, INFO_ERROR, 0);
+  }


if you look in client/readline.cc at the batch_readline_init its almost doing the same check.

If you look at the other call of batch_readline_init in this fine where stdin is passed, it just errors based on the return result of the batch_readline_init.

I recommend doing the same, just handle batch_readline_init the same way. Extend message to include potential memory allocation failure.

For tests - recommend adding someting in mysql-test/main/mysql_client_test.test. Something with -e "source $MASTER_MYSOCK" perhaps.

Changes addressed here

For .test and .result files I tried to use the environment variable I found here:

server/CMakeLists.txt

Line 45 in 809e6f4

${CMAKE_SOURCE_DIR}/cmake ${CMAKE_SOURCE_DIR}/cmake/Internal/CPack)

like this $CMAKE_SOURCE_DIR, but the test handled /client directory as a file and threw this error: ERROR at line 1: Failed to open file '/server/client', error: 2. So I will use my local relative path just for now.

For the current implementation of .test and .result files I would be grateful if you take a look at them and help me figure out how to solve this error:

mohammad@mohammad:~/open-source/mariaDB/build-mariadb-server-debug$ mysql-test/mtr mysql-test/main/mysql_client_test-mysql-source-errors.test Logging: /home/mohammad/open-source/mariaDB/server/mysql-test/mariadb-test-run.pl mysql-test/main/mysql_client_test-mysql-source-errors.test VS config: vardir: /home/mohammad/open-source/mariaDB/build-mariadb-server-debug/mysql-test/var Checking leftover processes... Removing old var directory... Creating var directory '/home/mohammad/open-source/mariaDB/build-mariadb-server-debug/mysql-test/var'... Checking supported features... MariaDB Version 10.6.25-MariaDB-debug - SSL connections supported - binaries are debug compiled - binaries built with wsrep patch Collecting tests... Installing system database... ============================================================================== TEST RESULT TIME (ms) or COMMENT -------------------------------------------------------------------------- worker[01] Using MTR_BUILD_THREAD 300, with reserved ports 19000..19029 main.mysql_client_test-mysql-source-errors [ fail ] Test ended at 2025-11-24 13:56:06 CURRENT_TEST: main.mysql_client_test-mysql-source-errors ERROR at line 1: Failed to read from '/home/mohammad/open-source/mariaDB/server/client': it is a directory, or memory allocation failed --- /home/mohammad/open-source/mariaDB/server/mysql-test/main/mysql_client_test-mysql-source-errors.result 2025-11-24 13:55:59.185361398 +0200 +++ /home/mohammad/open-source/mariaDB/server/mysql-test/main/mysql_client_test-mysql-source-errors.reject 2025-11-24 13:56:06.678022007 +0200 @@ -5,4 +5,3 @@ # # Try to SOURCE the /client directory (should fail) # -ERROR: Failed to read from '/home/mohammad/open-source/mariaDB/server/client': it is a directory, or memory allocation failed Result length mismatch - saving '/home/mohammad/open-source/mariaDB/build-mariadb-server-debug/mysql-test/var/log/main.mysql_client_test-mysql-source-errors/' to '/home/mohammad/open-source/mariaDB/build-mariadb-server-debug/mysql-test/var/log/main.mysql_client_test-mysql-source-errors/' -------------------------------------------------------------------------- The servers were restarted 0 times Spent 0.000 of 4 seconds executing testcases Failure: Failed 1/1 tests, 0.00% were successful. Failing test(s): main.mysql_client_test-mysql-source-errors The log files in var/log may give you some hint of what went wrong. If you want to report this error, MariaDB's bug tracker is found at https://jira.mariadb.org mysql-test-run: *** ERROR: there were failing test cases mohammad@mohammad:~/open-source/mariaDB/build-mariadb-server-debug$

source . is probably going to be simplest solution. But your test won't work on Windows anyway, since the check for file type is under #ifndef _WIN32.

source . is probably going to be simplest solution.

Should it look like this:
in .test

--exec $MYSQL -e "source ."

and in .result

ERROR: Failed to read from '.': it is a directory, or memory allocation failed

?

I tried that but it still fails although I have ubuntu installed on my machine.

Correct. How it fails?

I think the issue is caused by the test framework expecting a strict match between the error message in the .result file and the actual error message produced during the test. You can see this in the following output:

worker[01] Using MTR_BUILD_THREAD 300, with reserved ports 19000..19029 main.mysql_client_test-mysql-source-errors [ fail ] Test ended at 2025-11-24 17:30:54 CURRENT_TEST: main.mysql_client_test-mysql-source-errors ERROR at line 1: Failed to read from '.': it is a directory, or memory allocation failed --- /home/mohammad/open-source/mariaDB/server/mysql-test/main/mysql_client_test-mysql-source-errors.result 2025-11-24 15:25:50.676723768 +0200 +++ /home/mohammad/open-source/mariaDB/server/mysql-test/main/mysql_client_test-mysql-source-errors.reject 2025-11-24 17:30:54.452501704 +0200 @@ -5,4 +5,3 @@ # # Try to SOURCE the current directory (should fail) # -ERROR: Failed to read from '.': it is a directory, or memory allocation failed Result length mismatch

However, when I run the server and test the changes, it displays the intended error message!

The issue is that when we use --exec $MYSQL -e "source .", the error message goes to stderr, but the mtr framework is only capturing stdout in the result comparison. So it should look like this: --exec $MYSQL -e "source ." 2>&1

new changes addressed here

svoj

Please check #3935, specifically #3935 (review). This code should be slightly refactored to look good.

MohamedM216 · 2025-11-24T12:39:46Z

Thank you both for the feedback. @svoj, I appreciate the refactoring suggestion. @grooverdan, since you initially reviewed this, I'd value your input on which approach to take before proceeding.

grooverdan

If you move the test to the end of mysql-test/main/mysql_client_test.test this will resolve the embedded test failure (there's no client/server in embedded mode).

Rebase all commits onto the 10.11 branch, and squash them, include a commit message saying why these changes occurred. When finished git push --force and edit the github PR title to have 10.11 as the target branch.

At the end of the tests in mysql-test/main/mysql_client_test.test, put a --echo End of 10.11 tests. This is a standard practice to make it easier to resolve merge conflicts where multiple branches added tests. mtr --record mysql_client_test will update the result file in case you haven't discovered this, it doesn't need to be a manual edit (though do verify the contents).

grooverdan · 2025-11-25T02:42:17Z

mysql-test/main/mysql_client_test-mysql-source-errors.test

+--echo # Try to SOURCE the current directory (should fail)
+--echo #
+
+--error 1


To resolve the difference in output on Windows:

Suggested change

--error 1

--replace_regex /(ERROR at line 1: Failed to).*/\1 REPLACED/

--error 1

grooverdan · 2025-11-25T02:44:27Z

client/mysql.cc

    my_fclose(sql_file,MYF(0));
-    return put_info("Can't initialize batch_readline", INFO_ERROR, 0);
+    char buff[FN_REFLEN+200];
+    sprintf(buff,


use a snprintf(sizeof(buff),...

grooverdan · 2025-11-25T02:47:42Z

client/mysql.cc

-    return put_info("Can't initialize batch_readline", INFO_ERROR, 0);
+    char buff[FN_REFLEN+200];
+    sprintf(buff,
+            "Failed to read from '%s': it is a directory, or memory allocation failed",


Use a modifier on %s to restrict the message to the right most portion of the source_name. Adjust buffer size accordingly.

Message should be ".. it is a directory, block device, or memory allocation failed". Adjust the other batch_readline_init error message to be consistent with this.

grooverdan · 2025-11-25T03:14:56Z

Please check #3935, specifically #3935 (review). This code should be slightly refactored to look good.

Works for me too. Duplicating error message isn't productive.

svoj

Very nice. Some minor polishing left.

svoj · 2025-11-25T15:32:54Z

client/readline.cc


-static bool init_line_buffer(LINE_BUFFER *buffer,File file,ulong size,
-			    ulong max_size);
+bool init_line_buffer(LINE_BUFFER *buffer, File file, ulong size, ulong max_size);


Please remove this line, it shouldn't be needed anymore.

svoj · 2025-11-25T15:34:14Z

mysql-test/main/mysql_client_test.test

+
+--echo #
+--echo # Try to SOURCE the current directory (should fail)
+--echo #


This is redundant, you already say "Test that SOURCE on an existent directory returns a clear error" a few lines above.

svoj · 2025-11-25T15:34:35Z

mysql-test/main/mysql_client_test.test

+
+--replace_regex /(ERROR at line 1: Failed to).*/\1 REPLACED/
+--error 1
+--exec $MYSQL -e "source ."


Forgot 2>&1? There's no error in the result file.

Maybe I misunderstood @grooverdan 's review.

svoj · 2025-11-25T15:38:53Z

mysql-test/main/mysql_client_test.test

+--echo # Try to SOURCE the current directory (should fail)
+--echo #
+
+--replace_regex /(ERROR at line 1: Failed to).*/\1 REPLACED/


We need to think more about it. Ideally the same error should be returned on Windows. No action needed at the moment.

svoj · 2025-11-25T15:41:42Z

client/my_readline.h

 } LINE_BUFFER;

-extern LINE_BUFFER *batch_readline_init(ulong max_size,FILE *file);
+extern LINE_BUFFER *batch_readline_init(ulong max_size, const char *path);


Please remove this line.

svoj · 2025-11-25T15:42:00Z

client/mysql.cc

  return -1;
 }

+LINE_BUFFER *batch_readline_init(ulong max_size, const char *path)


It should be static.

svoj · 2025-11-25T16:06:19Z

client/mysql.cc

+    if (path)
+      my_close(file, MYF(0));
+    put_info(buff, INFO_ERROR, 0);
+    return 0;


To avoid code duplication, MariaDB common practice would be to use something like this:

... goto err; err1: put_info(buff, INFO_ERROR, 0); err: if (path) my_close(file, MYF(0)); return 0;

These labels should be after return line_buff.

svoj

More issues outlined.

svoj · 2025-11-25T19:19:49Z

client/mysql.cc

+    {
+      my_snprintf(buff, sizeof(buff), "Failed to open file '%.*s', error: %d", 
+                  FN_REFLEN, path, my_errno);
+      goto err1;


I wouldn't goto err1 from this point, as it will have to issue close(-1). Just do put_info() here.

svoj · 2025-11-25T19:20:13Z

client/mysql.cc

+    my_snprintf(buff, sizeof(buff), "Failed to stat file '%.*s', error: %d", 
+                FN_REFLEN, path ? path : "stdin", my_errno);
+    goto err1;
+    goto err;


Here and everywhere else, remove second goto.

Do you mean replace it with :

if (path) my_close(file, MYF(0)); return 0;

or remove it totally?

EDIT: yeah I think I understand now. goto err1 will lead to run err:.

Correct. Second goto is never reachable anyway.

client/mysql.cc

svoj

Few more issues. Please also fix commit message according to https://github.com/MariaDB/server/blob/main/CODING_STANDARDS.md.

Specifically lines must be under 72 characters.

svoj · 2025-11-26T09:20:25Z

mysql-test/main/mysql_client_test.test

+--echo # Test that SOURCE on an existent directory returns a clear error
+--echo #
+
+# --replace_regex /(ERROR at line 1: Failed to).*/\1 REPLACED/


Please remove this line.

svoj · 2025-11-26T09:29:01Z

client/mysql.cc

  {
-    my_fclose(sql_file,MYF(0));
-    return put_info("Can't initialize batch_readline", INFO_ERROR, 0);
+    return 1;


It has to be return ignore_errors ? -1 : 1;

svoj · 2025-11-26T09:30:19Z

client/mysql.cc

+  }
+
+#ifdef _WIN32
+#else


No, please do #ifndef _WIN32.

client/mysql.cc

svoj · 2025-11-27T20:34:48Z

@MohamedM216 I pushed 056e85f with coding style fixes to your branch. Please get it "fixup"-ed if you agree. Or I can do that myself.

Also we need to sort out failure on Windows. It looks like some sort of segmentation fault or stack overflow due to our Windows specific code. I have no idea how to debug it effectively at the moment. So I slightly modified the code, let's see how it works.

With these things completed, I think I can approve this pr.

svoj · 2025-11-27T21:36:46Z

@MohamedM216 it appears I broke your last force push with my force push. Sorry for the mess. Feel free to update it now.

svoj · 2025-11-27T21:54:01Z

I think I found a bug: it calls my_close() against failed my_open() on Windows. Well, uhm... what if we do something like this:

    if ((file= my_open(path, O_RDONLY | O_BINARY, MYF(0))) < 0)
    {
#ifdef _WIN32
      if (my_errno == EACCES && my_stat(path, &input_file_stat, MYF(0)) &&
          MY_S_ISDIR(input_file_stat.st_mode))
        my_snprintf(buff, sizeof(buff), "Can't read from a directory '%.*s'",
                    FN_REFLEN, path ? path : "stdin");
      else
#endif
      my_snprintf(buff, sizeof(buff), "Failed to open file '%.*s', error: %d",
                  FN_REFLEN, path, my_errno);
      put_info(buff, INFO_ERROR, 0);
      return 0;
    }

MohamedM216 · 2025-11-27T23:33:19Z

@svoj new changes addressed here

svoj

LGTM.

svoj · 2025-11-28T08:44:26Z

client/mysql.cc

+      if (my_errno == EACCES && my_stat(path, &input_file_stat, MYF(0)) &&
+          MY_S_ISDIR(input_file_stat.st_mode))
+        my_snprintf(buff, sizeof(buff), "Can't read from a directory '%.*s'",
+                    FN_REFLEN, path ? path : "stdin");


Here path is always set, no need to path ? path : "stdin", missed this when copy-pasting.

I fixed that: https://github.com/MariaDB/server/compare/589115b882b47c6444790caac8fd3110642260a9..7cd2cea0c4dbd7e773e8c84f8386f8f5eabc97ce

svoj · 2025-11-28T09:27:16Z

@grooverdan will you want to have another look?

- refactor batch_readline_init to mysql.cc for proper error handling - add unit test and move it to the end of main/mysql_client_test.test to resolve embedded test failures - remove unnecessary code and duplicates to clean up implementation - redirect error message from stderr to stdout in .test file - use labels to avoid code duplication - handle windows check for block device - ensure file failing to open in windows because being a directory is different from any other reason for clear error message

MohamedM216 · 2025-12-03T06:38:34Z

@grooverdan will you want to have another look?

@grooverdan just a gentle ping on this when you have a moment. Thanks!

svoj added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Nov 22, 2025

carlos2martinize approved these changes Nov 23, 2025

View reviewed changes

grooverdan requested changes Nov 24, 2025

View reviewed changes

svoj requested changes Nov 24, 2025

View reviewed changes

MohamedM216 force-pushed the bugfix/mdev-36269 branch from 1f1cba1 to 5b6b826 Compare November 24, 2025 12:16

MohamedM216 requested a review from grooverdan November 24, 2025 12:33

MohamedM216 requested a review from svoj November 24, 2025 12:39

grooverdan requested changes Nov 25, 2025

View reviewed changes

MohamedM216 changed the title ~~MDEV-36269: Improve error handling for source command with directories~~ MDEV-36269: Improve error handling for source command Nov 25, 2025

MohamedM216 force-pushed the bugfix/mdev-36269 branch from 7725fe4 to 7c64e5e Compare November 25, 2025 12:19

MohamedM216 changed the base branch from 10.6 to 10.11 November 25, 2025 12:20

MohamedM216 force-pushed the bugfix/mdev-36269 branch from 7c64e5e to a0d870e Compare November 25, 2025 12:26

MohamedM216 requested a review from grooverdan November 25, 2025 12:27

svoj requested changes Nov 25, 2025

View reviewed changes

MohamedM216 force-pushed the bugfix/mdev-36269 branch from a0d870e to 62f4505 Compare November 25, 2025 18:56

svoj requested changes Nov 25, 2025

View reviewed changes

MohamedM216 force-pushed the bugfix/mdev-36269 branch from 62f4505 to 7ff95d4 Compare November 25, 2025 20:04

MohamedM216 requested a review from svoj November 25, 2025 21:01

svoj requested changes Nov 26, 2025

View reviewed changes

MohamedM216 force-pushed the bugfix/mdev-36269 branch from 7ff95d4 to 43af148 Compare November 26, 2025 12:38

MohamedM216 requested a review from svoj November 26, 2025 14:30

MohamedM216 force-pushed the bugfix/mdev-36269 branch 4 times, most recently from dfb9afe to 82ed328 Compare November 27, 2025 10:48

svoj force-pushed the bugfix/mdev-36269 branch from 056e85f to d02e3bc Compare November 27, 2025 20:56

MohamedM216 force-pushed the bugfix/mdev-36269 branch from d02e3bc to b91b86c Compare November 27, 2025 21:03

svoj force-pushed the bugfix/mdev-36269 branch from b91b86c to 6ebd647 Compare November 27, 2025 21:34

MohamedM216 force-pushed the bugfix/mdev-36269 branch 2 times, most recently from 528a18e to 84e21da Compare November 27, 2025 23:29

svoj approved these changes Nov 28, 2025

View reviewed changes

MohamedM216 force-pushed the bugfix/mdev-36269 branch 2 times, most recently from 589115b to 7cd2cea Compare November 28, 2025 13:35

MohamedM216 force-pushed the bugfix/mdev-36269 branch from 7cd2cea to 553bd3e Compare December 2, 2025 19:08

	--error 1
	--replace_regex /(ERROR at line 1: Failed to).*/\1 REPLACED/
	--error 1

Uh oh!

MDEV-36269: Improve error handling for source command #4455

Are you sure you want to change the base?

MDEV-36269: Improve error handling for source command #4455

Conversation

MohamedM216 commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

Uh oh!

CLAassistant commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MohamedM216 commented Nov 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svoj Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svoj Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MohamedM216 Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

MohamedM216 commented Nov 24, 2025

Uh oh!

grooverdan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grooverdan commented Nov 25, 2025

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MDEV-36269: Improve error handling for `source` command #4455

MDEV-36269: Improve error handling for `source` command #4455

MohamedM216 commented Nov 22, 2025 •

edited

Loading

CLAassistant commented Nov 22, 2025 •

edited

Loading

svoj Nov 24, 2025 •

edited

Loading

svoj Nov 24, 2025 •

edited

Loading

MohamedM216 Nov 24, 2025 •

edited

Loading

MohamedM216 Nov 25, 2025 •

edited

Loading

svoj left a comment •

edited

Loading