Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2745) WL#4630

[Øystein Grøvlen]

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