| List: | Commits | « Previous MessageNext Message » | |
| From: | Øystein Grøvlen | Date: | February 17 2009 1:57pm |
| Subject: | Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2745) WL#4630 | ||
| View as plain text | |||
Ingo,
Here are my comments/questions for mysqlbackup.cc. (Sorry for the
delay). I am yet to look at the tests. I have not used the
request/suggestion format for this round of comments. I will do that
in a second round when I see your response. (Together with summing up
the first part of the review.)
My comments is based on the first version of the patch. I have not
yet looked at the newest patch.
All my comments are in-line.
Ingo Struewing wrote:
> #At file:///home2/mydev/bzrroot/mysql-6.0-wl4534-7/
>
> 2745 Ingo Struewing 2008-11-25
> WL#4630 - ST: MySQL Backup Client Program - Milestone 1
>
> The MySQL Backup Client Program is a command line client
> program, that examines and displays the contents of a
> backup image.
>
> By command line options you can select different types
> of information to view.
>
> It does also have a search capability for finding
> specific database objects in the backup image.
>
> For a detailed explanation of the options, run
>
> mysqlbackup --help
> added:
> client/backup_stream.c
> client/backup_stream.h
> client/mysqlbackup.cc
> mysql-test/suite/backup/r/backup_client.result
> mysql-test/suite/backup/r/backup_client_binlog.result
> mysql-test/suite/backup/r/backup_client_coverage.result
> mysql-test/suite/backup/t/backup_client.test
> mysql-test/suite/backup/t/backup_client_binlog.test
> mysql-test/suite/backup/t/backup_client_coverage.test
> modified:
> .bzrignore
> client/CMakeLists.txt
> client/Makefile.am
> include/my_sys.h
> mysql-test/mysql-test-run.pl
> mysys/my_malloc.c
> mysys/my_static.c
> mysys/safemalloc.c
...
> === added file 'client/mysqlbackup.cc'
> --- a/client/mysqlbackup.cc 1970-01-01 00:00:00 +0000
> +++ b/client/mysqlbackup.cc 2008-11-25 19:33:34 +0000
> @@ -0,0 +1,1710 @@
> +/* Copyright (C) 2001-2004 MySQL AB
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; version 2 of the License.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
02111-1307 USA */
> +
> +/*
> + MySQL Backup Utility
> +*/
> +
> +/* Include client related stuff, which includes a lot of common
stuff. */
> +#include "client_priv.h"
> +
> +/* As long as we don't connect to the server, we are not a
MYSQL_CLIENT. */
> +#undef MYSQL_CLIENT
> +
> +/* my_init_time() */
> +#include "my_time.h"
> +
> +/* MYSQL_SERVER_VERSION */
> +#include "mysql_version.h"
> +
> +/* Include from the stream access functions. */
> +#include "backup_stream.h"
> +
> +/* isspace() */
> +#include <ctype.h>
> +
> +/*
> + Configuration file.
> +*/
> +static const char *load_default_groups[]= { "mysqlbackup",
> +#ifdef MYSQL_CLIENT
> + "client",
> +#endif
Since MYSQL_CLIENT was undef just above, is this just for future
extensions?
> + NULL };
> +
> +/*
> + Command line options.
> +*/
> +
> +/* General options. */
> +static uint opt_verbose;
> +static ulong opt_open_files_limit;
> +#ifndef DBUG_OFF
> +static const char *default_dbug_option = "d:t:o,/tmp/mysqlbackup.trace";
> +#endif
> +
> +#ifdef MYSQL_CLIENT
> +/* Options required by a MySQL client (connecting to a server). */
> +static const char *opt_database;
As far as I can tell, this is usually called opt_databases in other
clients. Any reason it should be different here?
> +static const char *opt_host;
> +static char *opt_pass;
> +static int opt_port;
> +static uint opt_protocol;
> +static const char *opt_sock;
> +static const char *opt_user;
> +#endif
> +
> +/* mysqlbackup specific options. */
> +static bool opt_mysqlbackup_catalog_summary;
> +static bool opt_mysqlbackup_catalog_details;
> +static bool opt_mysqlbackup_metadata_statements;
> +static bool opt_mysqlbackup_metadata_extra;
> +static bool opt_mysqlbackup_snapshots;
> +static bool opt_mysqlbackup_data_chunks;
> +static bool opt_mysqlbackup_data_totals;
> +static bool opt_mysqlbackup_summary;
> +static bool opt_mysqlbackup_exact;
> +static const char *opt_mysqlbackup_search;
> +enum options_mysqlbackup
> +{
> + OPT_MYSQLBACKUP_CATALOG_SUMMARY=1024,
What is the magic of 1024?
> + OPT_MYSQLBACKUP_CATALOG_DETAILS,
> + OPT_MYSQLBACKUP_METADATA_STATEMENTS,
> + OPT_MYSQLBACKUP_METADATA_EXTRA,
> + OPT_MYSQLBACKUP_SNAPSHOTS,
> + OPT_MYSQLBACKUP_DATA_CHUNKS,
> + OPT_MYSQLBACKUP_DATA_TOTALS,
> + OPT_MYSQLBACKUP_SUMMARY,
> + OPT_MYSQLBACKUP_ALL,
> + OPT_MYSQLBACKUP_EXACT,
> + OPT_MYSQLBACKUP_SEARCH,
> +
> + /* End of options terminator. */
> + OPT_MYSQLBACKUP_LAST
> +};
> +
> +static struct my_option my_long_options[] =
> +{
> + /* General options. */
Isn't there some way that clients can share this code? It does not
seem right to have to copy&paste this for every new client.
> + {"help", '?', "Display this help and exit.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +#ifdef __NETWARE__
> + {"autoclose", OPT_AUTO_CLOSE, "Auto close the screen on exit for
Netware.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +#endif
> +#ifndef DBUG_OFF
> + {"debug", '#', "Output debug log.", (uchar**) &default_dbug_option,
> + (uchar**) &default_dbug_option, 0, GET_STR, OPT_ARG, 0, 0, 0, 0,
0, 0},
> +#endif
> + {"verbose", 'v', "Print verbose information.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> + {"version", 'V', "Print version and exit.", 0, 0, 0, GET_NO_ARG,
NO_ARG, 0,
> + 0, 0, 0, 0, 0},
> + {"open_files_limit", OPT_OPEN_FILES_LIMIT,
> + "Used to reserve file descriptors for usage by this program.",
> + (uchar**) &opt_open_files_limit, (uchar**) &opt_open_files_limit,
> + 0, GET_ULONG, REQUIRED_ARG, MY_NFILE, 8, OS_FILE_LIMIT, 0, 1, 0},
> +
> +#ifdef MYSQL_CLIENT
> + /* Options required by a MySQL client (connecting to a server). */
> + {"database", 'd', "Unused.",
> + (uchar**) &opt_database, (uchar**) &opt_database,
> + 0, GET_STR_ALLOC, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> + {"host", 'h', "Unused.",
> + (uchar**) &opt_host, (uchar**) &opt_host,
> + 0, GET_STR_ALLOC, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> + {"password", 'p', "Unused.",
> + 0, 0, 0, GET_STR, OPT_ARG, 0, 0, 0, 0, 0, 0},
> + {"port", 'P', "Unused.",
> + (uchar**) &opt_port, (uchar**) &opt_port, 0, GET_INT, REQUIRED_ARG,
> + 0, 0, 0, 0, 0, 0},
> + {"protocol", OPT_MYSQL_PROTOCOL, "Unused.",
> + 0, 0, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> + {"socket", 'S', "Unused.",
> + (uchar**) &opt_sock, (uchar**) &opt_sock,
> + 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> + {"user", 'u', "Unused.",
> + (uchar**) &opt_user, (uchar**) &opt_user,
> + 0, GET_STR_ALLOC, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> +#endif /*MYSQL_CLIENT*/
> +
> + /* mysqlbackup specific options. */
> + {"catalog-summary", OPT_MYSQLBACKUP_CATALOG_SUMMARY,
> + "Print summary from the database objects catalog.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"catalog-details", OPT_MYSQLBACKUP_CATALOG_DETAILS,
> + "Print details from the database objects catalog.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"metadata-statements", OPT_MYSQLBACKUP_METADATA_STATEMENTS,
> + "Print SQL statements to create the database objects.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"metadata-extra", OPT_MYSQLBACKUP_METADATA_EXTRA,
> + "Print extra meta data for the database objects.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"snapshots", OPT_MYSQLBACKUP_SNAPSHOTS,
> + "Print information about snapshots contained in the backup image.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"data-chunks", OPT_MYSQLBACKUP_DATA_CHUNKS,
> + "Print length of every data chunk contained in the backup image.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"data-totals", OPT_MYSQLBACKUP_DATA_TOTALS,
> + "Print length of data contained in the backup image for each
object.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"summary", OPT_MYSQLBACKUP_SUMMARY,
> + "Print summary information from end of the backup image.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"all", OPT_MYSQLBACKUP_ALL,
> + "Print everything except snapshots and data-chunks.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"exact", OPT_MYSQLBACKUP_EXACT,
> + "Print exact number of bytes instead of human readable form.",
> + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> +
> + {"search", OPT_MYSQLBACKUP_SEARCH,
> + "Search object in the backup image. "
> + "Name can be object or database.object. "
> + "Quoting of database and/or object with \", ', or ` is allowed. "
> + "Wildcards % and _ are available. "
> + "Use with --metadata-* options to see meta data. "
> + "Plain name finds global objects, name1.name2 finds per db objects.",
> + (uchar**) &opt_mysqlbackup_search, (uchar**) &opt_mysqlbackup_search,
> + 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> +
> + /* End of options terminator. */
> + {0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}
> +};
> +
> +#include <help_start.h>
> +
> +static void print_version(void)
> +{
> + printf("%s Ver %s for %s at %s\n", my_progname, MYSQL_SERVER_VERSION,
> + SYSTEM_TYPE, MACHINE_TYPE);
> + NETWARE_SET_SCREEN_MODE(1);
> +}
> +
> +
> +static void usage(void)
> +{
> + print_version();
> + puts("By Sun Microsystems, for your professional use\n\
> +This software comes with NO WARRANTY: This is free software,\n\
> +and you are welcome to modify and redistribute it under the GPL
license\n");
> +
Have you checked that this is correct? Should not some copyright
notice with year be included (e.g., see mysqld)?
> + printf("Displays information from a backup image.\n\n");
> + printf("Usage: %s [options] backup-image-file\n", my_progname);
> + my_print_help(my_long_options);
> + my_print_variables(my_long_options);
> +}
> +
> +#include <help_end.h>
> +
> +
> +/**
> + Treat some options specially.
> +
> + Called for each option that appears on the command line.
> +*/
> +
> +extern "C" my_bool
> +get_one_option(int optid, const struct my_option *opt
__attribute__((unused)),
> + char *argument)
> +{
> +#ifdef MYSQL_CLIENT
> + bool tty_password=0;
> +#endif
> +
> + switch (optid) {
> + /* General options. */
> +#ifdef __NETWARE__
> + case OPT_AUTO_CLOSE:
> + setscreenmode(SCR_AUTOCLOSE_ON_EXIT);
> + break;
> +#endif
> +#ifndef DBUG_OFF
> + case '#':
> + DBUG_PUSH(argument ? argument : default_dbug_option);
> + break;
> +#endif
> + case 'v':
> + opt_verbose++;
> + break;
> + case 'V':
> + print_version();
> + exit(0);
> + case '?':
> + usage();
> + exit(0);
> +
> +#ifdef MYSQL_CLIENT
> + /* Options required by a MySQL client (connecting to a server). */
> + case 'p':
> + if (argument)
> + {
> + my_free(opt_pass,MYF(MY_ALLOW_ZERO_PTR));
> + char *start=argument;
> + opt_pass= my_strdup(argument,MYF(MY_FAE));
> + while (*argument) *argument++= 'x'; /* Destroy argument */
> + if (*start)
> + start[1]=0; /* Cut length of argument */
> + }
> + else
> + tty_password=1;
> + break;
> + case OPT_MYSQL_PROTOCOL:
> + opt_protocol= find_type_or_exit(argument, &sql_protocol_typelib,
> + opt->name);
> + break;
> +#endif /*MYSQL_CLIENT*/
> +
> + /* mysqlbackup specific options. */
> + case OPT_MYSQLBACKUP_CATALOG_SUMMARY:
> + opt_mysqlbackup_catalog_summary= TRUE;
> + break;
> + case OPT_MYSQLBACKUP_CATALOG_DETAILS:
> + opt_mysqlbackup_catalog_details= TRUE;
> + break;
> + case OPT_MYSQLBACKUP_METADATA_STATEMENTS:
> + opt_mysqlbackup_metadata_statements= TRUE;
> + break;
> + case OPT_MYSQLBACKUP_METADATA_EXTRA:
> + opt_mysqlbackup_metadata_extra= TRUE;
> + break;
> + case OPT_MYSQLBACKUP_SNAPSHOTS:
> + opt_mysqlbackup_snapshots= TRUE;
> + break;
> + case OPT_MYSQLBACKUP_DATA_CHUNKS:
> + opt_mysqlbackup_data_chunks= TRUE;
> + break;
> + case OPT_MYSQLBACKUP_DATA_TOTALS:
> + opt_mysqlbackup_data_totals= TRUE;
> + break;
> + case OPT_MYSQLBACKUP_SUMMARY:
> + opt_mysqlbackup_summary= TRUE;
> + break;
> + case OPT_MYSQLBACKUP_ALL:
> + opt_mysqlbackup_catalog_summary= TRUE;
> + opt_mysqlbackup_catalog_details= TRUE;
> + opt_mysqlbackup_metadata_statements= TRUE;
> + opt_mysqlbackup_metadata_extra= TRUE;
> + opt_mysqlbackup_data_totals= TRUE;
> + opt_mysqlbackup_summary= TRUE;
> + break;
> + case OPT_MYSQLBACKUP_EXACT:
> + opt_mysqlbackup_exact= TRUE;
> + break;
> + }
> +
> +#ifdef MYSQL_CLIENT
> + if (tty_password)
> + opt_pass= get_tty_password(NullS);
> +#endif /*MYSQL_CLIENT*/
> +
> + return 0;
> +}
> +
> +
> +/**
> + Print a message to stderr with optional prefix.
> +
> + Prefix the message with the text "ERROR: " and optionally an extra
> + prefix.
> +
> + Flush stdout before printing to stderr. And stderr after it.
> +
> + The format string should include newlines ('\n') where required
> + to end a print line. This function adds additional newlines to
> + let the message stand out.
> +
> + @param[in] prefix optional prefix or empty string
> + @param[in] format printf-style format string
> + @param[in] args list of arguments for the format string
> +*/
> +
> +static void errm_with_prefix(const char *prefix, const char *format,
> + va_list args)
> +{
> + DBUG_ASSERT(prefix);
> + DBUG_ASSERT(format);
> +
> + /*
> + Before printing the error message, flush all prior output.
> + stdout may be buffered depending on platform and output device.
> + */
> + fflush(stdout);
> +
> + /* Write prefix. */
> + fprintf(stderr, "\nERROR: %s", prefix); /* prefix has newline if
needed. */
> +
> + /* Write error message. */
> + vfprintf(stderr, format, args);
> +
> + /* Append a newline to make it stand out. */
> + fprintf(stderr, "\n");
> +
> + /*
> + Flush the error message, so that it appears as soon as possible.
> + stderr may be buffered depending on platform and output device.
> + */
> + fflush(stderr);
> +}
> +
> +
> +/**
> + Print a message to stderr.
> +
> + The format string should include newlines ('\n') where required
> + to end a print line.
> +
> + @param[in] format printf-style format string
> + @param[in] ... printf-style varargs
> +*/
> +
> +extern "C" void errm(const char *format, ...)
ATTRIBUTE_FORMAT(printf, 1, 2);
> +extern "C" void errm(const char *format, ...)
> +{
> + va_list args;
> + DBUG_ASSERT(format);
> +
> + va_start(args, format);
> + errm_with_prefix("", format, args);
> + va_end(args);
> +}
> +
> +
> +/**
> + Print a formatted time string.
> +*/
> +
> +int print_time(bstream_time_t *time)
> +{
> + DBUG_ASSERT(time);
> + return printf("%04d-%02d-%02d %02d:%02d:%02d UTC",
> + time->year + 1900, time->mon + 1, time->mday,
> + time->hour, time->min, time->sec);
> +}
> +
> +
> +/**
> + Print a human readable number.
I do not think "number" is right here. Isn't it a size in bytes?
> +
> + Numbers are shown as +-0..16384 with a postfix of "bytes",
> + or as +-16..16384 with a postfix of "KB", "MB", "GB", "TB", "PB",
or "EB".
> + If one day longlong is > 64 bit, there can be higher numbers with
"EB".
> + The range is not +-0..1024 or +-1..1024 because we would lose too much
> + accuracy. Using +-16..16384, we have at least two significant digits.
> + And, by chance, 16384 EB - 1 is exactly the highest possible value
in a
> + 64-bit value (if used unsigned, which we don't do here).
> +*/
> +
> +static char *llstr_human(longlong value, char *buff)
> +{
> + const char *postfixes[]= {"bytes", "KB", "MB", "GB", "TB", "PB",
"EB", NULL};
> + const char **postfix_p= postfixes;
> + const char *src;
> + char *dst;
> + DBUG_ASSERT(buff);
> +
> + if (!opt_mysqlbackup_exact)
> + {
> + /* Shrink value while it's > 16K and postfix list is not at end. */
> + while (((value > 16383) || (value < -16384)) && *(++postfix_p))
> + value/= 1024;
> + /*
> + In case that we terminated the loop due to end of the postfixes
> + list, we need to compensate the loop's ++postfix_p so that we
> + point to the last non-NULL postfix. Note that this can only happen
> + on machines where longlong has more than 64 bits.
> + */
> + if (!*postfix_p)
> + --postfix_p; /* purecov: inspected */
I am not very fond of dead code like this. It is not possible to
test, and it took me some time to verify that it is actually correct.
I would guess that if longlong ever becomes something more than 64
bits, there are bigger problems that need to be solved ...
> + }
> + /* Convert the number into a string. */
> + (void) llstr(value, buff);
> + /* Find end of destination string. */
> + for (dst= buff; *dst; dst++) {}
> + /* Get pointer to the source string. */
> + src= *postfix_p;
> + /* Append postfix, including the terminating '\0'. */
> + *(dst++)= ' ';
> + while ((*(dst++)= *(src++))) {}
> + /* Result is in buff. */
> + return buff;
> +}
> +
> +
> +/* Flags for print_item(). */
> +#define PRI_NAME 0x0001
> +#define PRI_META 0x0002
> +
> +
> +/**
> + Print item information.
> +
> + @parameter[in] indent indentation spaces
> + @parameter[in] item_arg reference to some item
> + @parameter[in] what flags describing what to print
> + PRI_NAME item name
> + PRI_META meta data
> +*/
> +
> +void print_item(uint indent, void *item_arg, uint what)
> +{
> + struct st_bstream_item_info *item= (struct
st_bstream_item_info*) item_arg;
> + struct st_blob *db_name= NULL;
> + struct st_blob *name;
> + struct st_backup_metadata *mdata= NULL;
> + const char *type_s;
> + DBUG_ENTER("print_item");
> + DBUG_ASSERT(item);
> +
> + name= &item->name;
> +
> + switch (item->type) {
> +
> + case BSTREAM_IT_CHARSET:
> + case BSTREAM_IT_USER:
> + case BSTREAM_IT_TABLESPACE:
> + {
> + struct st_backup_global *bup_global= (struct st_backup_global*)
item;
> + type_s= bup_global->glb_typename;
> + mdata= &bup_global->glb_metadata;
> + DBUG_PRINT("bupstrm", ("%s", type_s));
> + break;
> + }
In the above code block, the order of the statements is different from
the others below. I think it would be good with some symmetry here.
> + case BSTREAM_IT_DB:
> + {
> + struct st_backup_database *bup_database= (struct
st_backup_database*) item;
> + DBUG_PRINT("bupstrm", ("database"));
> + type_s= "Database";
> + mdata= &bup_database->db_metadata;
> + break;
> + }
> +
> + case BSTREAM_IT_TABLE:
> + {
> + struct st_backup_table *bup_table= (struct st_backup_table*) item;
> + DBUG_PRINT("bupstrm", ("table"));
> + type_s= "Table";
> + db_name= &bup_table->tbl_item.base.db->base.name;
> + mdata= &bup_table->tbl_metadata;
> + break;
> + }
> +
> + case BSTREAM_IT_PRIVILEGE:
> + case BSTREAM_IT_VIEW:
> + case BSTREAM_IT_SPROC:
> + case BSTREAM_IT_SFUNC:
> + case BSTREAM_IT_EVENT:
> + case BSTREAM_IT_TRIGGER:
> + {
> + struct st_backup_perdb *bup_perdb= (struct st_backup_perdb*) item;
> +
> + switch (item->type) {
> + case BSTREAM_IT_PRIVILEGE:
> + DBUG_PRINT("bupstrm", ("privilege"));
> + type_s= "Privilege";
> + break;
> + case BSTREAM_IT_VIEW:
> + DBUG_PRINT("bupstrm", ("view"));
> + type_s= "View";
> + break;
> + case BSTREAM_IT_SPROC:
> + DBUG_PRINT("bupstrm", ("sproc"));
> + type_s= "Sproc";
> + break;
> + case BSTREAM_IT_SFUNC:
> + DBUG_PRINT("bupstrm", ("sfunc"));
> + type_s= "Sfunc";
> + break;
Is sproc and sfunc well known abbreviations? Does the s mean stored or sql?
I think it would be good to be more explicit here.
> + case BSTREAM_IT_EVENT:
> + DBUG_PRINT("bupstrm", ("event"));
> + type_s= "Event";
> + break;
> + case BSTREAM_IT_TRIGGER:
> + DBUG_PRINT("bupstrm", ("trigger"));
> + type_s= "Trigger";
> + break;
> + default: break; /* purecov: deadcode */
> + }
> + db_name= &bup_perdb->perdb_item.db->base.name;
> + mdata= &bup_perdb->perdb_metadata;
> + break;
> + }
> +
> + /* purecov: begin inspected */
> + default:
> + {
> + DBUG_PRINT("bupstrm", ("unknown item type"));
> + type_s= "Unknown item type";
> + break;
> + }
> + /* purecov: end */
> + }
> +
> + if (what & PRI_NAME)
> + {
> + if (db_name)
> + printf("%*s%-9s '%.*s'.'%.*s'\n",
> + indent, "", type_s,
> + BBLS(db_name), BBLS(name));
> + else
> + printf("%*s%-9s '%.*s'\n",
> + indent, "", type_s,
> + BBLS(name));
> + }
> +
> + if ((what & PRI_META) && mdata)
> + {
> + if (opt_mysqlbackup_metadata_statements)
> + {
> + if (db_name)
> + printf("%*s%s '%.*s'.'%.*s' statement: '%.*s'\n",
> + indent, "", type_s,
> + BBLS(db_name), BBLS(name), BBLS(&mdata->md_query));
> + else
> + printf("%*s%s '%.*s' statement: '%.*s'\n",
> + indent, "", type_s,
> + BBLS(name), BBLS(&mdata->md_query));
> + }
> +
> + if (opt_mysqlbackup_metadata_extra)
> + {
> + if (db_name)
> + printf("%*s%s '%.*s'.'%.*s' extra data length: %lu\n",
> + indent, "", type_s,
> + BBLS(db_name), BBLS(name), BBL(&mdata->md_data));
> + else
> + printf("%*s%s '%.*s' extra data length: %lu\n",
> + indent, "", type_s,
> + BBLS(name), BBL(&mdata->md_data));
> + }
> + }
> +
> + DBUG_VOID_RETURN;
> +}
> +
> +
> +/**
> + Print error from parsing the search name.
> +
> + @param[in] format printf-style format string
> + @param[in] args list of arguments for the format string
> +*/
> +
> +static void error_parse_search_name(const char *format, ...)
> + ATTRIBUTE_FORMAT(printf, 1, 2);
> +static void error_parse_search_name(const char *format, ...)
I do not quite understand what is going on here, but is this function
really necesssary? It seems like all you achieve is 3 less times of
typing "Cannot parse search name.\n". If you really wanted to save
typing, you could save more by using a macro.
> +{
> + va_list args;
> + DBUG_ASSERT(format);
> +
> + va_start(args, format);
> + errm_with_prefix("Cannot parse search name.\n", format, args);
> + va_end(args);
> +}
> +
> +
> +/**
> + Parse the search name.
> +
> + Split search name into database name, if present, and object name.
> +
> + @param[in] search_name search name
> + @param[out] search_database_name_p database name
> + @param[out] search_object_name_p object name
> +
> + @return status
> + @retval 0 ok
> + @retval != 0 error
> +*/
> +
> +static int parse_search_name(const char *search_name,
> + char **search_database_name_p,
> + char **search_object_name_p)
> +{
It seems to me like you are re-inventing the wheel here. Parsing for
quoted strings etc. must have been done before. Can't some existing
code be reused?
> + char *database= NULL;
> + char *object= NULL;
> + const char *token;
> + const char *tend;
> + int rc= 1; /* Assume error. */
> + char quote;
> + DBUG_ENTER("parse_search_name");
> + DBUG_ASSERT(search_name);
> + DBUG_ASSERT(search_database_name_p);
> + DBUG_ASSERT(search_object_name_p);
> + DBUG_PRINT("mysqlbackup", ("searching '%s'\n", search_name));
> +
> + /* Skip leading space. */
> + for (token= search_name; isspace(*token); token++) {}
> + /*
> + Check for quoted search string.
> + If unquoted, terminate by dot or space.
> + */
> + quote= '.';
> + switch (*token) {
> + case '\'':
> + case '\"':
> + case '`':
> + quote= *(token++);
> + }
> + /* Find end of token. */
> + if (quote == '.')
> + for (tend= token; *tend && (*tend != quote) && !isspace(*tend);
tend++) {}
> + else
> + for (tend= token; *tend && (*tend != quote); tend++) {}
> + /* Copy token into object name. */
> + object= (char*) my_memdup(token, (size_t) (tend - token + 1),
> + MYF(MY_WME));
> + object[tend - token]= '\0';
> + if (*tend)
> + {
> + /* Found quote or dot or space. */
> + if (quote != '.')
> + {
> + /* Found quote. Skip it. */
> + tend++;
> + }
> + /* Skip trailing space. */
> + for (; isspace(*tend); tend++) {}
> + if (*tend == '.')
> + {
> + /* Object name was a database name. */
> + database= object;
> + object= NULL;
> + /* Skip dot. */
> + tend++;
> + /* We need an object name. Skip space. */
> + for (token= tend; isspace(*token); token++) {}
Is it supposed to be OK with spaces after the dot?
> + /* Check for quoted search string. If unquoted, search for end. */
> + quote= '\0';
> + switch (*token) {
> + case '\'':
> + case '\"':
> + case '`':
> + quote= *(token++);
> + }
> + /* Find end of token. */
> + if (quote == '\0')
> + for (tend= token; *tend && !isspace(*tend); tend++) {}
> + else
> + for (tend= token; *tend && (*tend != quote); tend++) {}
A lot of duplicate code here. I suggest extracting the quoted string
stuff into a separate method which can be called several times.
> + /* Copy token into object name. */
> + object= (char*) my_memdup(token, (size_t) (tend - token + 1),
> + MYF(MY_WME));
> + object[tend - token]= '\0';
> + /* Check for correct syntax. */
> + if (*tend)
> + {
> + /* Found quote or space. */
> + if (quote != '\0')
> + {
> + /* Found quote. Skip it. */
> + tend++;
> + }
> + /* Skip trailing space. */
> + for (; isspace(*tend); tend++) {}
> + /* Now we must be at string end. */
> + if (*tend)
> + {
> + error_parse_search_name("Unrecognized stuff at '%s'\n", tend);
> + goto end;
stuff? I suggest content or something more "respectful".
> + }
> + }
> + else
> + {
> + /* End of string. */
> + if (quote != '\0')
> + {
> + error_parse_search_name("Search name improperly quoted.\n");
> + goto end;
> + }
> + }
> + }
> + else
> + {
> + if (*tend)
> + {
> + error_parse_search_name("Unrecognized stuff at '%s'\n", tend);
> + goto end;
> + }
> + }
> + }
> + else
> + {
> + /* End of string. */
> + if (quote != '.')
> + {
> + error_parse_search_name("Search name improperly quoted.\n");
> + goto end;
> + }
> + }
> +
> + /* Everything looks fine. */
> + *search_database_name_p= database;
> + *search_object_name_p= object;
> + rc= 0;
> +
> + end:
> + if (rc)
> + {
> + my_free(object, MYF(MY_ALLOW_ZERO_PTR));
> + my_free(database, MYF(MY_ALLOW_ZERO_PTR));
> + }
> + DBUG_RETURN(rc);
> +}
> +
> +
> +/**
> + Search backup object.
> +
> + @param[in] search_name search name
> + @param[out] search_database_name database name, may be NULL
> + @param[out] search_object_name object name
> +
> + @return status
> + @retval 0 ok
> + @retval != 0 error
> +*/
> +
> +void
> +search_object(struct st_backup_catalog *bup_catalog,
> + char *search_database_name,
> + char *search_object_name)
> +{
> + struct st_backup_global *bup_global;
> + struct st_backup_database *bup_database;
> + struct st_blob *name;
> + uint search_database_name_len;
> + uint search_object_name_len;
> + uint idx;
> + DBUG_ENTER("search_object");
> + DBUG_ASSERT(bup_catalog);
> + /* search_database_name may be NULL. */
> + DBUG_ASSERT(search_object_name);
> +
> + printf("\n");
> + if (search_database_name)
> + printf("Searching '%s'.'%s'\n", search_database_name,
search_object_name);
I suggest "Searching for ..."
> + else
> + printf("Searching '%s'\n", search_object_name);
Ditto.
> +
> + if (search_database_name)
> + search_database_name_len= strlen(search_database_name);
> + search_object_name_len= strlen(search_object_name);
> +
> + if (!search_database_name)
> + {
> + DYNAMIC_ARRAY **search_array_p;
> + DYNAMIC_ARRAY *search_array_array[]= {
> + &bup_catalog->cat_charsets,
> + &bup_catalog->cat_users,
> + &bup_catalog->cat_tablespaces,
> + NULL
> + };
> +
I suggest a comment here. Later blocks of this method have one.
> + for (search_array_p= search_array_array; *search_array_p;
search_array_p++)
> + {
> + for (idx= 0; idx < (*search_array_p)->elements; idx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_global= *((struct st_backup_global**)
> + dynamic_array_ptr(*search_array_p, idx));
> + name= &bup_global->glb_item.name;
> + if (!my_wildcmp(&my_charset_latin1, (char*) name->begin,
> + (char*) name->end, (char*) search_object_name,
> + (char*) search_object_name +
search_object_name_len,
> + '\\', '_', '%'))
> + {
> + print_item(2, bup_global, PRI_NAME | PRI_META);
> + }
> + }
> + }
> +
> + /* Search database. */
> + for (idx= 0; idx < bup_catalog->cat_databases.elements; idx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_database= *((struct st_backup_database**)
> + dynamic_array_ptr(&bup_catalog->cat_databases,
idx));
> + name= &bup_database->db_item.base.name;
> + if (!my_wildcmp(&my_charset_latin1, (char*) name->begin,
> + (char*) name->end, (char*) search_object_name,
> + (char*) search_object_name +
search_object_name_len,
> + '\\', '_', '%'))
> + {
> + print_item(2, bup_database, PRI_NAME | PRI_META);
> + }
> + }
> + goto end;
> + }
> +
> + /* Search object within databases. */
> + for (idx= 0; idx < bup_catalog->cat_databases.elements; idx++)
> + {
> + struct st_backup_database *bup_database;
> +
> + /* Note that the array contains pointers only. */
> + bup_database= *((struct st_backup_database**)
> + dynamic_array_ptr(&bup_catalog->cat_databases,
idx));
> + name= &bup_database->db_item.base.name;
> + if (!my_wildcmp(&my_charset_latin1, (char*) name->begin,
> + (char*) name->end, (char*) search_database_name,
> + (char*) search_database_name +
search_database_name_len,
> + '\\', '_', '%'))
> + {
> + DYNAMIC_ARRAY **search_array_p;
> + DYNAMIC_ARRAY *search_array_array[]= {
> + &bup_database->db_tables,
> + &bup_database->db_perdbs,
> + NULL
> + };
> + struct st_backup_perdb *bup_perdb;
> + uint jdx;
You are repeating the same pattern here. Please, consider if some
common code can be extracted into a separate method.
> +
> + for (search_array_p= search_array_array;
> + *search_array_p;
> + search_array_p++)
> + {
> + for (jdx= 0; jdx < (*search_array_p)->elements; jdx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_perdb= *((struct st_backup_perdb**)
> + dynamic_array_ptr(*search_array_p, jdx));
> + name= &bup_perdb->perdb_item.base.name;
> + if (!my_wildcmp(&my_charset_latin1, (char*) name->begin,
> + (char*) name->end, (char*) search_object_name,
> + (char*) search_object_name +
search_object_name_len,
> + '\\', '_', '%'))
> + {
> + print_item(4, bup_perdb, PRI_NAME | PRI_META);
> + }
> + }
> + }
> + }
> + }
> +
> + end:
> + DBUG_VOID_RETURN;
> +}
> +
> +
> +/**
> + Read and print summary.
> +
> + The summary section may be inside the image header or at image end.
> +
> + @param[in] image_handle image handle reference
> + @param[in] bup_catalog catalog reference
> +
> + @return status
> + @retval BSTREAM_OK ok
> + @retval != BSTREAM_OK error
> +*/
> +
> +enum enum_bstream_ret_codes
> +read_and_print_summary(void *image_handle,
> + struct st_backup_catalog *bup_catalog)
> +{
> + struct st_bstream_image_header *hdr= &bup_catalog->cat_header;
> + enum enum_bstream_ret_codes brc;
> + DBUG_ENTER("read_and_print_summary");
> + DBUG_ASSERT(image_handle);
> + DBUG_ASSERT(bup_catalog);
> +
> + /*
> + Read summary.
> + */
> + brc= backup_read_summary(image_handle, bup_catalog);
Why is bup_catalog the parameter here? It seems backup_read_summary
only uses the header internally, so why not use the header instead? I
guess the same holds for this method.
> + if (brc != BSTREAM_OK)
> + {
> + goto end;
> + }
> +
> + /*
> + Print summary.
> + */
> + if (opt_mysqlbackup_summary)
Is it necessary to read the summary if it is not supposed to be
printed? Anyway, why not let the caller call backup_read_summary
directly and just provide a method for printing? A general design
advice is to let methods have just one responsibility.
> + {
> + printf("\n");
> + printf("Summary:\n");
> + printf("\n");
> + printf("Creation time: ");
> + print_time(&hdr->start_time);
> + printf("\n");
> + printf("Validity time: ");
> + print_time(&hdr->vp_time);
> + printf("\n");
> + printf("Finish time: ");
> + print_time(&hdr->end_time);
> + printf("\n");
> +
> + /*
> + Binlog coordinates are present, if the BSTREAM_FLAG_BINLOG flag is
> + set in the header *and* table data are in the image.
> + DBUG_ASSERT(!(hdr->flags & BSTREAM_FLAG_BINLOG) ||
> + hdr->binlog_pos.file || hdr->binlog_group.file);
> + */
Why is this disabled?
> + if ((hdr->flags & BSTREAM_FLAG_BINLOG) &&
> + (hdr->binlog_pos.file || hdr->binlog_group.file))
> + {
> + printf("Binlog coordinates: %s:%lu\n",
> + hdr->binlog_pos.file ? hdr->binlog_pos.file : "[NULL]",
> + hdr->binlog_pos.pos);
> + printf("Binlog group coords: %s:%lu\n",
> + hdr->binlog_group.file ? hdr->binlog_group.file : "[NULL]",
> + hdr->binlog_group.pos);
> + }
> + else
> + {
> + printf("No binlog information\n");
> + }
> + }
> +
> + end:
> + DBUG_RETURN(brc);
> +}
> +
> +
> +/*
> + =============
> + Main program.
> + =============
> +*/
> +
> +/**
> + Main
> +
> + @parameter[in] argc argument count
> + @parameter[in] argv argument vector
> +
> + @return status
> + @retval 0 ok
> + @retval != 0 error
> +*/
> +
> +int main(int argc, char** argv)
This method is very long. I suggest splitting into several methods,
each with a single responsibility.
> +{
> + char **defaults_argv;
> + void *image_handle;
> + char *search_database_name= NULL;
> + char *search_object_name= NULL;
> + struct st_backup_catalog *bup_catalog= NULL;
> + struct st_bstream_image_header *hdr;
> + enum enum_bstream_ret_codes brc;
> + int rc;
> + uint idx;
> + int errpos= 0;
> + char llbuff[22];
Some explanations for the different local variables would be good.
In particular, I would like to know the significance of the different
values for rc.
> +
> + /* Preparations */
> + MY_INIT(argv[0]);
> + DBUG_ENTER("main");
> + DBUG_PROCESS(argv[0]);
> + my_init_time(); // for time functions
> + load_defaults("my", load_default_groups, &argc, &argv);
> + defaults_argv= argv;
> + errpos= 10;
> + rc= handle_options(&argc, &argv, my_long_options, get_one_option);
> + if (rc)
> + {
> + /* Error message reported by mysys. */
> + rc= 1;
> + goto use_err;
> + }
> + errpos= 20;
> + if (argc != 1)
> + {
> + errm("incorrect number of arguments.\n");
> + rc= 1;
> + goto use_err;
> + }
> + if (opt_mysqlbackup_search)
> + {
> + if (parse_search_name(opt_mysqlbackup_search, &search_database_name,
> + &search_object_name))
> + {
> + rc= 1;
> + goto use_err;
> + }
> + errpos= 25;
> + }
> + my_set_max_open_files(opt_open_files_limit);
> +
> + /*
> + Allocate catalog.
> + */
> + bup_catalog= backup_catalog_allocate();
> + if (!bup_catalog)
> + {
> + rc= 2;
> + goto end;
> + }
> + errpos= 30;
> +
> + /*
> + Open backup image stream.
> + */
> + image_handle= backup_image_open(argv[0], bup_catalog);
> + if (!image_handle)
> + {
> + rc= 2;
> + goto end;
> + }
> + errpos= 40;
> +
> + /*
> + Print backup image stream header.
> + */
> + hdr= &bup_catalog->cat_header;
> + printf("\n");
> + printf("Image path: '%s'\n",
> + bup_catalog->cat_image_path);
> + printf("Image size: %s\n",
> + llstr_human(bup_catalog->cat_image_size, llbuff));
> + printf("Image compression: %s\n", bup_catalog->cat_zalgo);
> + printf("Image version: %u\n", hdr->version);
> + printf("Creation time: ");
> + print_time(&hdr->start_time);
> + printf("\n");
> + printf("Server version: %d.%d.%d (%.*s)\n",
> + hdr->server_version.major, hdr->server_version.minor,
> + hdr->server_version.release, BBLS(&hdr->server_version.extra));
> + printf("Server byte order: %s\n",
> + hdr->flags & BSTREAM_FLAG_BIG_ENDIAN ?
> + "big-endian" : "little-endian");
> + if (hdr->flags && opt_verbose)
> + {
> + const char *separators[]= {"", ", "};
> + int separator= 0;
This separator trick is pretty need, but it took me some time to
understand so a comment would be good.
> +
> + printf("Image options: ");
> + if (hdr->flags & BSTREAM_FLAG_INLINE_SUMMARY)
> + {
> + /* purecov: begin inspected */
> + printf("%sINLINE_SUMMARY", separators[separator]);
> + separator= 1;
> + /* purecov: end */
> + }
> + if (hdr->flags & BSTREAM_FLAG_BIG_ENDIAN)
> + {
> + /* purecov: begin inspected */
> + printf("%sBIG_ENDIAN", separators[separator]);
> + separator= 1;
> + /* purecov: end */
> + }
> + if (hdr->flags & BSTREAM_FLAG_BINLOG)
> + {
> + printf("%sBINLOG", separators[separator]);
> + separator= 1;
> + }
> + printf("\n");
> + }
> +
> + if (hdr->flags & BSTREAM_FLAG_INLINE_SUMMARY)
Is it not possible to test this?
> + {
> + /* purecov: begin inspected */
> + brc= read_and_print_summary(image_handle, bup_catalog);
> + if (brc != BSTREAM_OK)
> + {
> + rc= 2;
> + goto end;
> + }
> + /* purecov: end */
> + }
> +
> + /*
> + Save image reading time if the rest is not wanted.
> + Sorry for the spaghetti here. The reason is that it has beed
> + decided not to display the first two character sets from the
> + catalog with the other catalog items:
> + The first two character sets are special:
> + 1. Character set to use for interpreting the backup file.
> + 2. Server character set.
> + These do not count as catalog items. They are to display in
> + the header section. To make the output somewhat user-friendly,
> + we display them before the optional snapshots section.
> + So if we have to display the snapshot section, but nothing
> + from the catalog, we can skip catalog reading here.
> + */
> + if (opt_mysqlbackup_search ||
> + opt_mysqlbackup_snapshots ||
> + opt_mysqlbackup_catalog_summary ||
> + opt_mysqlbackup_catalog_details ||
> + opt_mysqlbackup_metadata_statements ||
> + opt_mysqlbackup_metadata_extra ||
> + opt_mysqlbackup_data_chunks ||
> + opt_mysqlbackup_data_totals ||
> + (opt_mysqlbackup_summary && !(hdr->flags &
BSTREAM_FLAG_INLINE_SUMMARY)))
> + {
> + /*
> + Read catalog.
> + This can take some time. So flush what we have so far.
> + */
> + fflush(stdout);
> + brc= backup_read_catalog(image_handle, bup_catalog);
> + if (brc != BSTREAM_OK)
> + {
> + rc= 2;
> + goto end;
> + }
> + }
> +
> + /*
> + The first two character sets are special:
> + 1. Character set to use for interpreting the backup file.
> + 2. Server character set.
> + These do not count as catalog items.
> + List them separately.
> + */
> + if (bup_catalog->cat_charsets.elements >= 2)
> + {
> + struct st_backup_global *bup_global;
> +
> + /* Note that the array contains pointers only. */
> + bup_global= *((struct st_backup_global**)
> + dynamic_array_ptr(&bup_catalog->cat_charsets, 1));
> + printf("Server charset: '%.*s'\n",
BBLS(&bup_global->glb_item.name));
> + }
> + if (opt_verbose && (bup_catalog->cat_charsets.elements >= 1))
> + {
> + struct st_backup_global *bup_global;
> +
> + /* Note that the array contains pointers only. */
> + bup_global= *((struct st_backup_global**)
> + dynamic_array_ptr(&bup_catalog->cat_charsets, 0));
> + printf("Backup image chrset: '%.*s'\n",
BBLS(&bup_global->glb_item.name));
> + }
> +
> + if (opt_mysqlbackup_snapshots)
> + {
> + printf("Snapshot count: %u\n", hdr->snap_count);
> + printf("\n");
> + printf("Snapshots:\n");
> + printf("\n");
> + for (idx= 0; idx < hdr->snap_count; idx++)
> + {
> + struct st_bstream_snapshot_info *snapshot= hdr->snapshot +
idx;
> + const char *snap_type;
> +
> + switch (snapshot->type) {
> + case BI_NATIVE: snap_type= "native from";
> + break;
> + case BI_DEFAULT: snap_type= "logical from locked tables";
> + break;
> + case BI_CS: snap_type= "logical from consistent snapshot";
> + break;
> + /* purecov: begin inspected */
> + case BI_NODATA: snap_type= "nodata";
> + break;
> + default: snap_type= "unknown/illegal";
> + break;
> + /* purecov: end */
> + }
> + if (snapshot->type == BI_NATIVE)
> + printf(" Snapshot %u type %s '%.*s' version %u.%u\n",
> + idx, snap_type,
> + BBLS(&snapshot->engine.name),
> + snapshot->engine.major, snapshot->engine.minor);
> + else
> + printf(" Snapshot %u type %s\n", idx, snap_type);
> + printf(" Snapshot %u version %u\n", idx, snapshot->version);
> + if (snapshot->options)
> + printf(" Snapshot %u options %u\n",
> + idx, snapshot->options); /* purecov: inspected */
> + printf(" Snapshot %u tables %lu\n", idx, snapshot->table_count);
> + /*
> + List tables included in this snapshot.
> + If there is a table, there must also be a database.
> + */
> + DBUG_ASSERT(!snapshot->table_count ||
> + bup_catalog->cat_databases.elements);
> + if (snapshot->table_count)
> + {
> + uint kdx;
> +
> + for (kdx= 0; kdx < bup_catalog->cat_databases.elements; kdx++)
> + {
> + struct st_backup_database *bup_database;
> + struct st_blob *db_name;
> +
> + /* Note that the array contains pointers only. */
> + bup_database= *((struct st_backup_database**)
> +
dynamic_array_ptr(&bup_catalog->cat_databases, kdx));
> + db_name= &bup_database->db_item.base.name;
> + if (bup_database->db_tables.elements)
> + {
> + struct st_backup_table *bup_table;
> + uint jdx;
> +
> + for (jdx= 0; jdx < bup_database->db_tables.elements; jdx++)
> + {
> + struct st_blob *name;
> +
> + /* Note that the array contains pointers only. */
> + bup_table= *((struct st_backup_table**)
> +
dynamic_array_ptr(&bup_database->db_tables, jdx));
> + if (bup_table->tbl_item.snap_num == idx)
> + {
> + name= &bup_table->tbl_item.base.base.name;
> + printf(" Snapshot %u table '%.*s'.'%.*s'\n",
> + idx, BBLS(db_name), BBLS(name));
> + }
> + }
> + }
> + }
> + }
> + }
> + }
> +
> + /*
> + Print catalog.
> + */
> + if (opt_mysqlbackup_catalog_summary)
> + {
> + ulong sum_table;
> + ulong sum_perdb;
> +
> + printf("\n");
> + printf("Catalog summary:\n");
> + printf("\n");
> + if (opt_verbose && (bup_catalog->cat_charsets.elements > 2))
> + {
> + /*
> + The first two character sets are special:
> + 1. Character set to use for interpreting the backup file.
> + 2. Server character set.
> + These do not count as catalog items.
> + */
> + /* purecov: begin inspected */
> + printf(" Character sets: %u\n",
> + bup_catalog->cat_charsets.elements - 2);
> + /* purecov: end */
> + }
> +
> + if (bup_catalog->cat_users.elements)
> + {
> + /* purecov: begin inspected */
> + printf(" Users: %u\n",
> + bup_catalog->cat_users.elements);
> + /* purecov: end */
> + }
> +
> + if (bup_catalog->cat_tablespaces.elements)
> + {
> + printf(" Tablespaces: %u\n",
> + bup_catalog->cat_tablespaces.elements);
> + }
> +
> + if (bup_catalog->cat_databases.elements)
> + {
> + printf(" Databases: %u\n",
> + bup_catalog->cat_databases.elements);
> + sum_table= 0;
> + sum_perdb= 0;
> + for (idx= 0; idx < bup_catalog->cat_databases.elements; idx++)
> + {
> + struct st_backup_database *bup_database;
> +
> + /* Note that the array contains pointers only. */
> + bup_database= *((struct st_backup_database**)
> +
dynamic_array_ptr(&bup_catalog->cat_databases, idx));
> + sum_table+= bup_database->db_tables.elements;
> + sum_perdb+= bup_database->db_perdbs.elements;
> + }
> + if (sum_table)
> + printf(" Tables: %lu\n", sum_table);
> + if (sum_perdb)
> + printf(" Non-table db objects: %lu\n", sum_perdb);
> + }
> + }
> +
> + if (opt_mysqlbackup_catalog_details)
> + {
> + struct st_backup_global *bup_global;
> + struct st_backup_database *bup_database;
> +
> + printf("\n");
> + printf("Catalog details:\n");
> + printf("\n");
> +
> + /* Charsets. */
> + if (opt_verbose)
> + {
> + /*
> + The first two character sets are special:
> + 1. Character set to use for interpreting the backup file.
> + 2. Server character set.
> + These do not count as catalog items.
I have been told this a few times already. This might be an
indication that some code could be extracted and shared, or just that
this comment is superfluous.
> + */
> + for (idx= 2; idx < bup_catalog->cat_charsets.elements; idx++)
> + {
> + /* purecov: begin inspected */
> + /* Note that the array contains pointers only. */
> + bup_global= *((struct st_backup_global**)
> + dynamic_array_ptr(&bup_catalog->cat_charsets,
idx));
> + print_item(2, bup_global, PRI_NAME);
> + /* purecov: end */
> + }
> + }
> +
> + /* Users. */
> + for (idx= 0; idx < bup_catalog->cat_users.elements; idx++)
> + {
> + /* purecov: begin inspected */
> + /* Note that the array contains pointers only. */
> + bup_global= *((struct st_backup_global**)
> + dynamic_array_ptr(&bup_catalog->cat_users, idx));
> + print_item(2, bup_global, PRI_NAME);
> + /* purecov: end */
> + }
> +
> + /* Table spaces. */
> + for (idx= 0; idx < bup_catalog->cat_tablespaces.elements; idx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_global= *((struct st_backup_global**)
> + dynamic_array_ptr(&bup_catalog->cat_tablespaces,
idx));
> + print_item(2, bup_global, PRI_NAME);
> + }
> +
> + /* Databases. */
> + for (idx= 0; idx < bup_catalog->cat_databases.elements; idx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_database= *((struct st_backup_database**)
> + dynamic_array_ptr(&bup_catalog->cat_databases,
idx));
> + print_item(2, bup_database, PRI_NAME);
> +
> + /* Tables. */
> + if (bup_database->db_tables.elements)
> + {
> + struct st_backup_table *bup_table;
> + uint jdx;
> +
> + for (jdx= 0; jdx < bup_database->db_tables.elements; jdx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_table= *((struct st_backup_table**)
> + dynamic_array_ptr(&bup_database->db_tables,
jdx));
> + print_item(4, bup_table, PRI_NAME);
> + }
> + }
> +
> + /* Perdbs. */
> + if (bup_database->db_perdbs.elements)
> + {
> + struct st_backup_perdb *bup_perdb;
> + uint jdx;
> +
> + for (jdx= 0; jdx < bup_database->db_perdbs.elements; jdx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_perdb= *((struct st_backup_perdb**)
> + dynamic_array_ptr(&bup_database->db_perdbs,
jdx));
> + print_item(4, bup_perdb, PRI_NAME);
> + }
> + }
> + }
> + }
> +
> + /*
> + If user wants to search in catalog, but is not interested in meta
> + data, we can do the search now, before reading the meta data.
> + */
> + if (opt_mysqlbackup_search &&
> + !opt_mysqlbackup_metadata_statements &&
> + !opt_mysqlbackup_metadata_extra)
> + {
> + search_object(bup_catalog, search_database_name,
search_object_name);
> + goto end;
> + }
I do not understand why the searching is split in two places. Why
could not searching be done here even if one is interested in metadata?
> +
> + /* Save image reading time if the rest is not wanted. */
> + if (!opt_mysqlbackup_search &&
> + !opt_mysqlbackup_metadata_statements &&
> + !opt_mysqlbackup_metadata_extra &&
> + !opt_mysqlbackup_data_chunks &&
> + !opt_mysqlbackup_data_totals &&
> + (!opt_mysqlbackup_summary || (hdr->flags &
BSTREAM_FLAG_INLINE_SUMMARY)))
> + goto end;
> +
> + /*
> + Read meta data.
> + This can take some time. So flush what we have so far.
> + */
> + fflush(stdout);
> + brc= backup_read_metadata(image_handle, bup_catalog);
> + if (brc != BSTREAM_OK)
> + {
> + rc= 2;
> + goto end;
> + }
> +
> + if (opt_mysqlbackup_search)
> + {
> + search_object(bup_catalog, search_database_name,
search_object_name);
> + goto end;
> + }
> +
> + /*
> + Print meta data.
> + */
> + if (opt_mysqlbackup_metadata_statements ||
opt_mysqlbackup_metadata_extra)
> + {
> + printf("\n");
> + printf("Meta data:\n");
> +
> + /* Charsets don't have meta data. */
> + /* Users don't have meta data. */
> +
> + /* Tablespaces. */
> + for (idx= 0; idx < bup_catalog->cat_tablespaces.elements; idx++)
> + {
> + struct st_backup_global *bup_global;
> +
> + /* Note that the array contains pointers only. */
> + bup_global= *((struct st_backup_global**)
> + dynamic_array_ptr(&bup_catalog->cat_tablespaces,
idx));
> + printf("\n");
> + print_item(2, bup_global, PRI_META);
> + }
> +
> + /* Databases and their items. */
> + for (idx= 0; idx < bup_catalog->cat_databases.elements; idx++)
> + {
> + struct st_backup_database *bup_database;
> +
> + /* Note that the array contains pointers only. */
> + bup_database= *((struct st_backup_database**)
> + dynamic_array_ptr(&bup_catalog->cat_databases,
idx));
> + printf("\n");
> + print_item(2, bup_database, PRI_META);
> +
> + /* Tables. */
> + {
> + struct st_backup_table *bup_table;
> + uint jdx;
> +
> + for (jdx= 0; jdx < bup_database->db_tables.elements; jdx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_table= *((struct st_backup_table**)
> + dynamic_array_ptr(&bup_database->db_tables,
jdx));
> + printf("\n");
> + print_item(4, bup_table, PRI_META);
> + }
> + }
> +
> + /* Perdb items. */
> + {
> + struct st_backup_perdb *bup_perdb;
> + uint jdx;
> +
> + for (jdx= 0; jdx < bup_database->db_perdbs.elements; jdx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_perdb= *((struct st_backup_perdb**)
> + dynamic_array_ptr(&bup_database->db_perdbs,
jdx));
> + printf("\n");
> + print_item(4, bup_perdb, PRI_META);
> + }
> + }
> + }
> + }
> +
> + /* Save image reading time if the rest is not wanted. */
> + if (!opt_mysqlbackup_data_chunks &&
> + !opt_mysqlbackup_data_totals &&
> + (!opt_mysqlbackup_summary || (hdr->flags &
BSTREAM_FLAG_INLINE_SUMMARY)))
> + goto end;
> +
> + /*
> + Read table data.
> + This can take some time. So flush what we have so far.
> + */
> + fflush(stdout);
> + if (opt_mysqlbackup_data_chunks)
> + {
> + printf("\n");
> + printf("Data chunks:\n");
> + }
> + /*
> + Even if data chunks are not requested, we have to read them up, to
> + get at the summary. Also we accumulate table sizes.
> + */
> + do
> + {
> + struct st_bstream_data_chunk data_chunk;
> + struct st_backup_table *bup_table;
> + struct st_blob *tbl_name;
> + struct st_blob *db_name;
> + ulonglong chunk_cnt= 0;
> +
> + brc= backup_read_snapshot(image_handle, bup_catalog, &data_chunk);
> + if (brc != BSTREAM_OK)
> + break;
> + chunk_cnt++;
> +
> + /*
> + There can be a special "table" with number 0: "common data".
> + Normal tables start at number 1.
> + */
> + if (data_chunk.table_num)
> + {
> + bup_table= backup_locate_table(bup_catalog, data_chunk.snap_num,
> + data_chunk.table_num - 1);
> + /* Accumulate data size. */
> + DBUG_ASSERT(!bup_table->tbl_data_size ||
> + (bup_table->tbl_item.snap_num ==
data_chunk.snap_num));
> + bup_table->tbl_data_size+= BBL(&data_chunk.data);
> + /* Get pointers to the names. */
> + tbl_name= &bup_table->tbl_item.base.base.name;
> + db_name= &bup_table->tbl_item.base.db->base.name;
> +
> + if (opt_mysqlbackup_data_chunks)
> + {
> + if (opt_mysqlbackup_snapshots)
> + printf(" Chunk %s has %lu bytes for table '%.*s'.'%.*s' "
> + "from snapshot %d\n",
> + llstr(chunk_cnt, llbuff), BBL(&data_chunk.data),
> + BBLS(db_name), BBLS(tbl_name), data_chunk.snap_num);
> + else
> + printf(" Chunk %s has %lu bytes for table '%.*s'.'%.*s'\n",
> + llstr(chunk_cnt, llbuff), BBL(&data_chunk.data),
> + BBLS(db_name), BBLS(tbl_name));
> + }
> + }
> + else
> + {
> + /* purecov: begin inspected */
> + if (opt_mysqlbackup_data_chunks)
> + {
> + if (opt_mysqlbackup_snapshots)
> + printf(" Chunk %s has %lu bytes for common data "
> + "from snapshot %d\n",
> + llstr(chunk_cnt, llbuff), BBL(&data_chunk.data),
> + data_chunk.snap_num);
> + else
> + printf(" Chunk %s has %lu bytes for common data\n",
> + llstr(chunk_cnt, llbuff), BBL(&data_chunk.data));
> + }
> + /* purecov: end */
> + }
> + } while (1);
> +
> + if (opt_mysqlbackup_data_totals)
> + {
> + struct st_backup_database *bup_database;
> + struct st_backup_table *bup_table;
> + struct st_blob *db_name;
> + struct st_blob *tbl_name;
> + uint idx;
> + uint jdx;
> +
> + printf("\n");
> + printf("Data totals:\n");
> + printf("\n");
> + for (idx= 0; idx < bup_catalog->cat_databases.elements; idx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_database= *((struct st_backup_database**)
> + dynamic_array_ptr(&bup_catalog->cat_databases,
idx));
> + db_name= &bup_database->db_item.base.name;
> +
> + for (jdx= 0; jdx < bup_database->db_tables.elements; jdx++)
> + {
> + /* Note that the array contains pointers only. */
> + bup_table= *((struct st_backup_table**)
> + dynamic_array_ptr(&bup_database->db_tables, jdx));
> + tbl_name= &bup_table->tbl_item.base.base.name;
> + if (opt_mysqlbackup_snapshots)
> + printf(" Backup has %s for table '%.*s'.'%.*s' "
> + "in snapshot %d\n",
> + llstr_human(bup_table->tbl_data_size, llbuff),
> + BBLS(db_name), BBLS(tbl_name),
> + bup_table->tbl_item.snap_num);
> + else
> + printf(" Backup has %s for table '%.*s'.'%.*s'\n",
> + llstr_human(bup_table->tbl_data_size, llbuff),
> + BBLS(db_name), BBLS(tbl_name));
> + }
> + }
> + }
> +
> + /* Reading of snapshots must end with BSTREAM_EOC. */
> + if (brc != BSTREAM_EOC)
> + {
> + rc= 2;
> + goto end;
> + }
> +
> + if (!(hdr->flags & BSTREAM_FLAG_INLINE_SUMMARY))
> + {
> + brc= read_and_print_summary(image_handle, bup_catalog);
> + if (brc != BSTREAM_OK)
> + {
> + rc= 2;
> + goto end;
> + }
> + }
> +
> + goto end;
> +
> + use_err:
> + usage();
> + rc= 1;
> +
Instead of a particular use_err target, why to just go to end and use
the value of rc to determine whether usage should be printed.
> + end:
> + /* Flush stdout to get output in right order in case of error
messages. */
> + fflush(stdout);
> + /* Cleanup. */
> + switch (errpos) {
> + default:
> + case 40:
> + brc= backup_image_close(image_handle);
> + if (brc != BSTREAM_OK)
> + rc= 2;
> + case 30:
> + backup_catalog_free(bup_catalog);
> + case 25:
> + my_free(search_object_name, MYF(MY_ALLOW_ZERO_PTR));
> + my_free(search_database_name, MYF(MY_ALLOW_ZERO_PTR));
> + case 20:
> +#ifdef MYSQL_CLIENT
> + my_free((char*) opt_database, MYF(MY_ALLOW_ZERO_PTR));
> + my_free((char*) opt_host, MYF(MY_ALLOW_ZERO_PTR));
> + my_free((char*) opt_pass, MYF(MY_ALLOW_ZERO_PTR));
> + my_free((char*) opt_user, MYF(MY_ALLOW_ZERO_PTR));
> +#endif
Are you sure that this clean-up is not needed if handle_option() fail?
Or to state it differently, are you sure that handle_option() does
the necessary cleanup should it fail? Anyway, why bother with errpos
when doing it in MY_ALLOW_ZERO_PTR mode. Would it not be OK to call
free for all pointers regardless of when it failed?
> + case 10:
> + free_defaults(defaults_argv);
> + }
> + my_free_open_file_info();
> + /* We cannot free DBUG, it is used in global destructors after
exit(). */
> + my_end(MY_DONT_FREE_DBUG);
> + DBUG_RETURN(rc);
> +}
> +
--
Øystein