maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Sergey!

On Oct 23, Sergey Petrunya wrote:
> Hello,
> 
> Could somebody please review the below? It's a very simple patch.
> 
> I also need comments about which version this should go into. I've made the
> patch againist 5.1 just in case, but my opinion is that this should go into
> 5.2 (under rationale that 5.1 is stable and one can live without this patch).

as discussed on the phone - it should go in 5.3

Now, the patch.

Disclaimer - I didn't grep for error messages to verify that you patched
all places where ER_TABLEACCESS_DENIED_ERROR or
ER_COLUMNACCESS_DENIED_ERROR are used. I am sure you have done it
yourself already :)

There is only one problem.

-        eng "%-.16s command denied to user '%-.48s'@'%-.64s' for table '%-.192s'"
+        eng "%-.16s command denied to user '%-.48s'@'%-.64s' for table '%-.192s'.'%-.192s'"

you've added a new %s argument to an existing error message. Generally
it's something we are never supposed to do. It makes error message files
backward incompatible - old mysqld running on new .sys files will most
certainly crash. In our - MariaDB - case it also means that running
vanilla MySQL on our .sys files will crash too.

I'd prefer you to keep old error message untouched, even if
inconvenient. Two possible solutions. Add new error message with the
table and db name. Or use an utility formatting function, like

       my_error(ER_COLUMNACCESS_DENIED_ERROR, MYF(0),
                "ANY", thd->security_ctx->priv_user,
-               thd->security_ctx->host_or_ip, field_name, tab);
+               thd->security_ctx->host_or_ip, field_name, format_name(db, tab, buf));

where format_name() will do something like

  sprintf(buf, "%s'.'%s", db, tab);
  return buf;

This second solution is, I think, preferred, as it simplifies future
merges of errmst.txt - in particular, it won't cause problems with
shifted error numbers.

>   MWL#152: Show database name in error messages
>   - Add database name to ER_TABLEACCESS_DENIED_ERROR and ER_COLUMNACCESS_DENIED_ERROR message strings.
>   - Amend the code to provide db name
>   - Update test results

Regards,
Sergei