Code review comment for lp:~sergei.glushchenko/percona-xtrabackup/xb_bug483827

Revision history for this message
Alexey Kopytov (akopytov) wrote :

Sergei,

   - sections in my.cnf are called groups in MySQL documentation. So
     let's be consistent and call them groups as well, and name the
     options (both the innobackupex and the xtrabackup one)
     --defaults-group, rather than --section and --mysqld-section

   - it looks like get_option is always passed the same value as its
     second argument, so I wonder if it really needs that argument, or
     rather just use $option_mysqld_section (or whatever it will be
     renamed to, i.e. $option_defaults_group) internally.

   - s/laod/load/
   - s/accetps/accepts/
   - s/rstore_args/restore_args/. Though you don't really need that
     function, as tests are executed in a separate shell process, so
     modifications to variables have no effect on other tests anyway.

   - I know that code in main() that scan options for "--mysqld-section"
     was copy-pasted, but please format it according to InnoDB
     style. Because currently it's a terrible mix of all possible
     formatting styles.

   - in the same code, I don't think strstr() is necessary, because you
     already have the pointer to '=' (or terminating zero) in optend.

review: Needs Fixing

« Back to merge proposal