MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Rafal Somla Date:June 23 2007 9:00pm
Subject:Re: bk commit into 5.1 tree (rafal:1.2537)
View as plain text  
Chuck,

Thank you for the review! I think it will help to produce better 5.2 patches. I 
implemented most of the suggestions and answered other questions below. To make 
it easier for you to see the changes I attach a diff from the previous patch. 
Shortly I'll commit an updated patch as well.

Chuck Bell wrote:
> Rafal,
> 
> I think this patch kicks online backup up a notch. Patches like this get us
> closer to GA code.
> 
> Thanks for the explanation on Skype. That helped a lot. 
> 
> I’ve listed my comments in one group otherwise you’d be digging forever
> to
> find any inline comments. I’ve tried to provide references to the code where
> possible.
>
> I apologize if some of my suggestion seems overly picky. I recent discovered
> a document on how review criteria which is helping me “grow some teeth”
> as
> Lars suggested. ;) See: http://forge.mysql.com/wiki/Code_Review_Process for
> more details.
>

It is very good that you become more critical. The way I see it - better to hear 
criticism from own team member than later from external reviewers. So, feel free 
to become even more picky ;)

> 
> Questions
> ---------
>> added clear() method to Image_info::Tables class – What does this do?
>

It empties the list - added a comment.

>> TEST_ERROR_IF(t.name().ptr()[0]=='b'); - What does ‘b’ mean? Can you
> please document any sentinel values that you are using?
> 

This is a dirty hack to artificially trigger error when name of a table being 
backed-up starts with 'b' (sadly, I didn't get it working yet). I added 
explaining comments end documented the macros in debug.h.

> Why do you not use DBUG_ENTER and DBUG_RETURN in some places? I’m curious
> why you would use the macros in some places but not others.
>

The general idea is to reduce clutter in the debug trace by using 
DBUG_ENTER/RETURN only in functions and not in class methods. I'm probably not 
100% consistent in this but this is the principle I was using.

> What would this code generate for a message?
> +   if (!bad_drivers.is_empty())
> +   info.report_error("Error when shutting down restore drivers no %s",
> +                      bad_drivers.c_ptr());
> 
> It reads a little funny. Should there be something after the %s?
>

The intention was to produce message "Error when shutting down restore drivers 
no 1,3,7" where numbers refer to position of the driver in archive's driver 
list. Now I changed it to list driver names and reformulated message 
(ER_BACKUP_STOP_RESTORE_DRIVERS in errmsg.txt)

In the future I think that perhaps it will be better to list problematic drivers 
with separate messages, and have one summarizing error message. Like this:

[Info] Error when shutting down MyISAM backup driver
[Info] Error when shutting down Default backup driver
[ERROR] Errors detected during backup driver shutdown

> 
> Suggestions
> -----------
> Write the policy for return codes and document their use. I think this will
> be very important to native driver developers and to me for the default and
> snapshot drivders. I’d like to see all of them use the same basic
> format/codes/etc.
>
 > Please consider adding some overall documentation about how error handling
 > works to the worklog or maybe create a new worklog to track all that good
 > information.
 >

I added policy description to LLD of WL#3904. Note that driver methods return 
result_t value and should report errors through it. The ideas about how it 
should look are described in the WL. This need extending result_t type which is 
not done yet. So currently errors are signalled by drivers by simply returning 
ERROR code.

> Reminder: spell check. ;)

Done, although I don't trust Eclipse's spell checker 100%. Let me know if you 
spot something.

> 
> Move “FIXME”’s, “Consider:”’s, and “//
> TODO:”’s to @TODO list – see
> sql_backup.cc.
> 

I moved TODO-s to common list. But as for "FIXME" and "Consider" I prefer to 
leave them inlined in the code so that it is easy to see where is the problem/issue.

> Remove commented out code or if it is needed for later change it to a block
> comment and reference refer to it in the @TODO list.
> 

Done.

> We should to use constants for errors (e.g. error= -3 should be error=
> MYSQL_ERR_SOMETHING_BAD or some such);
> 

See my design in LLD of WL#3904. Basically I want to avoid assigning global 
error codes and I give my arguments there.

> Change comments to use NULL instead of NIL (e.g. see archive.cc). Nil makes
> me think 0 rather than no value. But I know most NULL’s are defined as 0 so
> that just confuses things. ;) NULL just reads better.
> 

These are two different things. I use NIL for a nil string which can be 
written/read to/from a stream. This is a no-string which is even different from 
empty string, i.e. NIL != "". NULL is an empty pointer, as usual.

> 
> Things that need fixing
> -----------------------
> Coding guideline: multi-line comments not in a /* */ block. 
> Coding guideline: single comments using // instead of /* comment */ format. 
>
> http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines

Done.

> 
> Change “restoredriver” to “restore driver”. See
> data_backup.cc.
> 

Done.

>> --- 1.1/sql/backup/meta_backup.h	2007-06-21 00:48:31 +02:00
>> +++ 1.2/sql/backup/meta_backup.h	2007-06-21 00:48:31 +02:00
> ...
>> +  /*
>> +    we give default implementation because an item can not use create
>> +    statements and then it doesn't have to worry about that method
>> +  */
>> +  virtual int build_create_stmt(THD*)
>> +  { return -1; }
> 
> Please try to use punctuation and capitalization in block comments.
>

I tried :)

> 
> Concerns for 5.2 alpha release
> ------------------------------
> I think for the 5.2 release the error messages need to be included the
> errmsg.txt file in at least the basic romantic languages before we release
> it as alpha. Just add the few you have now. I wouldn’t want someone
> complaining about not having error messages internationalized. You would
> think that someone would have a generator for that...but the online
> internals documentation doesn’t give any clue to one.
> 

You are right. I moved all current messages to errmsg.txt. I'm not very 
confident in the quality of my English but what are our dear reviewers for ;) As 
for other languages, I think this is a job for docs team. If I get my editor to 
use ISO-latin-2 encoding, I can add Polish translations.

> 
> My Experiences with the patch
> -----------------------------
> I could not get the patch to work on Windows. It complained about several
> files even though I have a freshly cloned mysql-5.1-backup-prototype tree. I
> had the same problem on Linux. After some wrestling with some previous
> patches and some manual resolution, I got it to work on Linux but not
> Windows. 
> 

There was a preceding patch which I announced in an earlier email and this one 
is building over it. I'm sorry for not being clear enough about the structure of 
the patches. When I'm reviewing patches, even if I apply them, I usually don't 
even try to compile. This is why I didn't think about this.

Now I added progress reports to WL which describe patches which I produce. Hope 
that will help in the future.

> Once the code compiled, the error handling worked well. I especially like
> that the server doesn't crash when someone enters an invalid filename (I do
> it often).
> 

I also like that :)

Rafal

> 

===== api_types.h 1.5 vs edited =====
--- 1.5/sql/backup/api_types.h	2007-06-23 22:56:36 +02:00
+++ edited/api_types.h	2007-06-23 21:58:24 +02:00
@@ -129,8 +129,14 @@ class Table_ref
   bool operator!=(const Table_ref &db) const
   { return ! this->operator==(db); }
 
-  // Produce string identifying the table (e.g. for error reporting)
-  //operator String() const;
+  typedef char describe_buf[512];
+
+  /// Produce string identifying the table (e.g. for error reporting)
+  const char* describe(char *buf, size_t len) const
+  {
+    my_snprintf(buf,len,"%s.%s",db().name().ptr(),name().ptr());
+    return buf;
+  }
 
  protected:
 
===== archive.cc 1.7 vs edited =====
--- 1.7/sql/backup/archive.cc	2007-06-23 22:56:36 +02:00
+++ edited/archive.cc	2007-06-23 22:04:10 +02:00
@@ -14,9 +14,11 @@
   - Make reading code resistant to unknown image formats or meta-data types
     (or, assume it is handled by format version number).
   - Improve Image_info::Tables implementation (use some existing data structure).
-  - Add more information to backup archive header.
-  - Decide how to report errors in Archive_info methods and friends.
-
+  - Add more information to backup archive header , for example server's version
+    string.
+  - Handle backward compatibility (new code reads archive with earlier version
+    number)
+  - Add to Archive_info methods for browsing contents of the archive.
  */
 
 #include "backup_engine.h"
@@ -77,11 +79,8 @@ Archive_info::~Archive_info()
   @c Image_info::write_description() method.
 
   For list of databases and tables the format in which they are saved is
-  defined by @c StringPool::save() and @c Archive_info::Tbles::save() methods
+  defined by @c StringPool::save() and @c Archive_info::Tables::save() methods
   respectively.
-
-  TODO: Add more info to the header. E.g. version number of the server on
-  which archive was created.
  */
 
 result_t Archive_info::save(OStream &s)
@@ -110,7 +109,7 @@ result_t Archive_info::save(OStream &s)
       DBUG_PRINT("backup",(" %2d: %s image",ino,img->name()));
       if (ERROR == img->write_description(s))
       {
-        DBUG_PRINT("backup",("Can't write descrition of %s image (#%d)",
+        DBUG_PRINT("backup",("Can't write description of %s image (#%d)",
                              img->name(),ino));
         DBUG_RETURN(ERROR);
       }
@@ -126,7 +125,7 @@ result_t Archive_info::save(OStream &s)
 
 
   // write catalogue
-  DBUG_PRINT("backup",(" writing archive catalog"));
+  DBUG_PRINT("backup",(" writing archive catalogue"));
 
   // db names (one chunk)
   if (ERROR == db_names.save(s)) // note: this closes the chunk
@@ -183,7 +182,6 @@ result_t Archive_info::read(IStream &s)
   if (ver != Archive_info::ver)
   {
     DBUG_PRINT("restore",("Backup archive version %d not supported",ver));
-    // TODO: in future handle backward compatibility
     DBUG_RETURN(ERROR);
   }
 
@@ -287,13 +285,13 @@ result_t Archive_info::read(IStream &s)
 
 } // backup namespace
 
+
 /**********************************
 
   Write/read image descriptions
 
  **********************************/
 
-
 namespace backup {
 
 /**
@@ -311,7 +309,7 @@ namespace backup {
 result_t
 Image_info::write_description(OStream &s)
 {
-  // TODO: write description length here
+  // TODO: to handle unknown description formats, write description length here
 
   stream_result::value res= s.writebyte(type());
 
@@ -332,8 +330,6 @@ Image_info::write_description(OStream &s
   @retval OK
   @retval DONE  end of chunk/stream hit
   @retval ERROR
-
-  TODO: Handle unknown image types (always skip correct amount of bytes).
  */
 result_t
 Image_info::create_from_stream(Archive_info &info, IStream &s, Image_info* &ptr)
@@ -343,7 +339,7 @@ Image_info::create_from_stream(Archive_i
 
   stream_result::value res= s.readbyte(t);
 
-  // if we are at end of data chunk or stream, we should inform here
+  // if we are at end of data chunk or stream, we should tell the caller
   if (res != stream_result::OK)
     return (result_t)res;
 
@@ -393,7 +389,7 @@ namespace backup {
   respectively.
 
   @see @c write_meta_data() for information about the format of the meta-data
-  section of bakcup archive.
+  section of backup archive.
 */
 result_t
 Archive_info::Item::save(THD *thd, OStream &s)
@@ -421,8 +417,6 @@ Archive_info::Item::save(THD *thd, OStre
   @retval OK    if new item was created
   @retval DONE  if end of chunk/stream was reached
   @retval ERROR if error has happened
-
-  TODO: handle unknow entry types (so that they can be skipped).
  */
 result_t
 Archive_info::Item::create_from_stream(const Archive_info &info,
@@ -549,6 +543,7 @@ struct Image_info::Tables::node {
   }
 };
 
+/// Empty the list.
 void Image_info::Tables::clear()
 {
   for (node *ptr= m_head; ptr;)
@@ -562,6 +557,11 @@ void Image_info::Tables::clear()
   m_count= 0;
 }
 
+/**
+  Add a table to the list.
+
+  @returns Position of the table or -1 if error
+ */
 int Image_info::Tables::add(const backup::Table_ref &t)
 {
   StringPool::Key k= m_db_names.add(t.db().name());
@@ -572,6 +572,7 @@ int Image_info::Tables::add(const backup
   return add(k,t.name());
 }
 
+/// Add table at given position.
 int Image_info::Tables::add(const StringPool::Key &k, const String &name)
 {
   node *n= new node(*this,k,name);
@@ -594,6 +595,11 @@ int Image_info::Tables::add(const String
   return m_count-1;
 }
 
+/**
+  Locate table at given position.
+
+  @returns Pointer to table's list node or NULL if position is not occupied
+ */
 Image_info::Tables::node*
 Image_info::Tables::find_table(uint pos) const
 {
@@ -609,6 +615,7 @@ Image_info::Tables::find_table(uint pos)
   return ptr;
 }
 
+/// Return table at a given position.
 inline
 Table_ref Image_info::Tables::operator[](uint pos) const
 {
@@ -753,7 +760,7 @@ namespace backup {
   by the storage engine whose backup driver created it. Thus we save the name
   of storage engine.
 
-  TODO: add more information here. Eg the version number of the storage engine.
+  TODO: add more information here. E.g. the version number of the storage engine.
  */
 result_t
 Native_image::do_write_description(OStream &s)
@@ -787,7 +794,6 @@ Native_image::create_from_stream(version
   {
     DBUG_PRINT("restore",("Restore diver ver %d can't read image version %d",
                           img->ver,ver));
-    // TODO: deal with different versions
     return ERROR;
   }
 
@@ -797,4 +803,3 @@ Native_image::create_from_stream(version
 }
 
 } // backup namespace
-
===== archive.h 1.12 vs edited =====
--- 1.12/sql/backup/archive.h	2007-06-23 22:56:36 +02:00
+++ edited/archive.h	2007-06-23 22:05:44 +02:00
@@ -4,7 +4,8 @@
 /**
   @file
 
-  Data types used to read, write and manipulate backup archives.
+  Data types used to represent contents of a backup archive and to read/write
+  its description (catalogue)
  */
 
 #include <backup/api_types.h>
@@ -28,7 +29,7 @@ typedef Image_info* Img_list[MAX_IMAGES]
   all items stored in the archive (currently only databases and tables). It also
   determines how to save and read the catalogue to/from a backup stream.
 
-  Only item names are stored in the catalouge. Other item data is stored
+  Only item names are stored in the catalogue. Other item data is stored
   in the meta-data part of an archive and in case of tables, their data is
   stored in images created by backup drivers.
 
@@ -43,8 +44,6 @@ typedef Image_info* Img_list[MAX_IMAGES]
 
   When reading or writing backup archive, statistics about the size of its parts
   is stored in the members of this class for later reporting.
-
-  TODO: Define methods for browsing contents of the archive.
  */
 struct Archive_info
 {
@@ -63,7 +62,9 @@ struct Archive_info
    class Db_item;
    class Table_item;
 
- /*
+  /*
+    Classes which might be used to implement contents browsing.
+
    class Item_iterator;  // for iterating over all meta-data items
    class Db_iterator;    // iterates over databases in archive
    class Ditem_iterator; // iterates over per-db items
@@ -80,8 +81,6 @@ struct Archive_info
 
  protected:
 
-  // Item *m_items;
-
    Archive_info():
      img_count(0), table_count(0),
      total_size(0), header_size(0), meta_size(0), data_size(0)
@@ -104,7 +103,7 @@ struct Archive_info
 
   An instance of this class:
   - informs about the type of image,
-  - stores list of tables whose data is keept in the image,
+  - stores list of tables whose data is kept in the image,
   - provides methods for creating backup and restore drivers to write/read the
     image,
   - determines which tables can be stored in the image,
@@ -126,7 +125,7 @@ struct Image_info
   virtual result_t get_restore_driver(Restore_driver*&) =0;
 
   size_t init_size; ///< Size of the initial data transfer (estimate). This is
-                    ///< meaningfull only after a call to get_backup_driver().
+                    ///< meaningful only after a call to get_backup_driver().
 
   /// Write header entry describing the image.
   result_t write_description(OStream&);
@@ -134,7 +133,7 @@ struct Image_info
   /**
     Create instance of @c Image_info described by an entry in backup stream.
 
-    @retval OK    entry successfuly read
+    @retval OK    entry successfully read
     @retval DONE  end of chunk or stream has been reached.
     @retval ERROR an error was detected
    */
@@ -143,9 +142,13 @@ struct Image_info
   /// Determine if a table stored in given engine can be saved in this image.
   virtual bool accept(const Table_ref&, const ::handlerton*) =0;
 
-  /// Return name identifying the image in debug messages.
+  /** 
+    Return name identifying the image in debug messages.
+   
+    The name should fit into "%s backup/restore driver" pattern.
+   */
   virtual const char* name() const
-  { return "<Unknown Driver>"; }
+  { return "<Unknown>"; }
 
   virtual ~Image_info()
   {}
@@ -234,20 +237,20 @@ class Archive_info::Item
 {
  protected:
 
-  const Archive_info *const m_info; ///< Pointer to an archive info instance to
-                                    ///< which this item belongs.
+  /// Pointer to @c Archive_info instance to which this item belongs.
+  const Archive_info *const m_info;
   Item  *next; ///< Used to create a linked list of all meta-data items.
 
  public:
 
   virtual ~Item() {}
 
-  virtual meta::Item& meta() =0;  ///< Return reference to the @c meta::Item
-                                  ///< instance.
+  /// Returns reference to the corresponding @c meta::Item instance.
+  virtual meta::Item& meta() =0;
 
   result_t save(THD*,OStream&); ///< Write entry describing the item.
 
-  // Create item from a stream entry.
+  // Create item from a saved entry.
   static result_t create_from_stream(const Archive_info&, IStream&, Item*&);
 
   class Iterator;
@@ -411,7 +414,7 @@ class Native_image: public Image_info
   const ::handlerton  *m_hton; ///< Pointer to storage engine.
   Engine   *m_be;  ///< Pointer to the native backup engine.
 
-  char m_name[256];  ///< Used to identify image in debug messages.
+  const char *m_name;  ///< Used to identify image in debug messages.
 
  public:
 
@@ -426,8 +429,7 @@ class Native_image: public Image_info
     if(m_be)
     {
       ver= m_be->version();
-      my_snprintf(m_name,sizeof(m_name),"%s's driver",
-                  ::ha_resolve_storage_engine_name(hton));
+      m_name= ::ha_resolve_storage_engine_name(hton);
     }
   }
 
===== backup_kernel.h 1.8 vs edited =====
--- 1.8/sql/backup/backup_kernel.h	2007-06-23 22:56:36 +02:00
+++ edited/backup_kernel.h	2007-06-23 21:58:24 +02:00
@@ -7,8 +7,10 @@
 #include <backup/logger.h>
 
 
-// Called from the big switch in mysql_execute_command() to execute
-// backup related statement
+/*
+  Called from the big switch in mysql_execute_command() to execute
+  backup related statement
+ */
 int execute_backup_command(THD*, LEX*);
 
 /**
@@ -47,9 +49,10 @@ namespace backup {
 
 struct Location
 {
-  // as we don't use rtti we need these to find out real type
-  // of location object.
-
+  /*
+    As we don't use rtti we need these to find out the real type
+    of a location object.
+   */
   enum enum_type {SERVER_FILE, INVALID};
   bool read;
 
@@ -62,6 +65,7 @@ struct Location
   virtual void free()
   { delete this; }  // use destructor to free resources
 
+  /// Describe location for debug purposes
   virtual const char* describe()
   { return "Invalid location"; }
 
@@ -82,7 +86,7 @@ struct Location
   When Backup_info object is created it is empty and ready for adding items
   to it. Methods @c add_table() @c add_db(), @c add_dbs() and @c add_all_dbs()
   can be used for that purpose (currently only databases and tables are
-  supported). After populationg info object with items it should be "closed"
+  supported). After populating info object with items it should be "closed"
   with a call to @c close() method. After that it is ready for use as a
   description of backup archive to be created.
 
@@ -133,7 +137,6 @@ class Backup_info: public Archive_info, 
   int add_all_dbs();
 
   bool close();
-  //bool close_tables();
 
    class Item_iterator;  // for iterating over all meta-data items
 
@@ -143,13 +146,9 @@ class Backup_info: public Archive_info, 
   enum {INIT,   // structure ready for filling
         READY,  // structure ready for backup (tables opened)
         DONE,   // tables are closed
-	ERROR
+        ERROR
        }   m_state;
 
-  /* Find image to which given table can be added and add it.
-     Creates new images as needed.
-     Returns -1 on error.
-   */
   int find_image(const Table_ref&);
 
   int default_image_no; ///< Position of the default image in @c images list, -1 if not used.
@@ -163,8 +162,6 @@ class Backup_info: public Archive_info, 
   THD    *m_thd;
   TABLE  *i_s_tables;
 
-  //bool open_tables();
-
   Item   *m_items;
   Item   *m_last_item;
   Item   *m_last_db;
@@ -195,7 +192,7 @@ class Backup_info::Item_iterator: public
 class Restore_info: public Archive_info, public Logger
 {
   bool m_valid;
-  
+
  public:
 
   Restore_info(IStream &s): m_valid(TRUE)
@@ -203,7 +200,7 @@ class Restore_info: public Archive_info,
     result_t res= read(s);
     if (res == ERROR)
     {
-      report_error("Can't read backup archive header");
+      report_error(ER_BACKUP_READ_HEADER);
       m_valid= FALSE;
     }
   }
===== data_backup.cc 1.19 vs edited =====
--- 1.19/sql/backup/data_backup.cc	2007-06-23 22:56:36 +02:00
+++ edited/data_backup.cc	2007-06-23 21:58:24 +02:00
@@ -10,6 +10,28 @@
   backed up.
  */
 
+/*
+  TODO
+
+  - Implement better scheduling strategy in Scheduler::step
+
+     The driver with least data read is chosen. If there are several,
+     they are polled in round-robin fashion.
+
+  - For consistency, always use OStream for data serialization (?)
+
+  - In Backup_pump::pump(), handle DONE response from backup driver.
+
+  - In case of backup/restore of empty archive, check that all resources
+    are correctly deallocated.
+
+  - Try to announce block size when initializing restore driver with begin()
+    method (is it possible?).
+
+  - If an error from driver is ignored (and operation retried) leave trace
+    of the error in the log.
+ */
+
 #include "backup_engine.h"
 #include "stream.h"
 #include "backup_kernel.h"
@@ -35,7 +57,7 @@ struct backup_state {
               FINISHING,  ///< Final data transfer (phase 7).
               DONE,       ///< Backup complete.
               SHUT_DOWN,  ///< After @c end() call.
-              CANCELED,   ///< After cancelling backup process.
+              CANCELLED,   ///< After cancelling backup process.
               ERROR,
               MAX };
 
@@ -55,7 +77,7 @@ struct backup_state {
       name[FINISHING]= "FINISHING";
       name[DONE]= "DONE";
       name[SHUT_DOWN]= "SHUT DOWN";
-      name[CANCELED]= "CANCELED";
+      name[CANCELLED]= "CANCELLED";
       name[ERROR]= "ERROR";
     }
   };
@@ -107,7 +129,7 @@ struct Block_writer
   Poll backup driver for backup data and send it to a stream. Monitors stages
   of the backup process, keeps track of closed streams etc.
 
-  Usage: Initialize using begin() method, then call pump() metod repeatedly.
+  Usage: Initialize using begin() method, then call pump() method repeatedly.
   The state member informs about the current state of the backup process. When
   done, call end() method. Methods prepare(), lock() and unlock() are forwarded
   to backup driver to implement multi-engine synchronization.
@@ -171,7 +193,7 @@ struct Backup_pump
    */
   byte          *m_buf_head;
 
-  uint          m_buf_retries; ///< how many times failed to get a buffer from blick writer
+  uint          m_buf_retries; ///< how many times failed to get a buffer from block writer
 
   bool get_buf();
   bool drop_buf();
@@ -234,13 +256,13 @@ class Scheduler
   size_t m_init_left;   ///< how much of init data is left (estimate)
   uint   m_known_count; ///< no. drivers which can estimate init data size
   OStream &m_str;       ///< stream to which we write
-  bool   canceled;      ///< true if backup process was canceled
+  bool   cancelled;      ///< true if backup process was cancelled
 
   Scheduler(OStream &s, Logger *log):
     init_count(0), prepare_count(0), finish_count(0),
     m_log(log), m_pumps(NULL), m_last(NULL),
     m_count(0), m_total(0), m_init_left(0), m_known_count(0),
-    m_str(s), canceled(FALSE)
+    m_str(s), cancelled(FALSE)
   {}
 
   void move_pump_to_end(const Pump_iterator&);
@@ -293,7 +315,7 @@ int write_table_data(THD*, Backup_info &
   Scheduler   sch(s,&info);         // scheduler instance
   List<Scheduler::Pump>  inactive;  // list of images not yet being created
   size_t      max_init_size=0;      // keeps maximal init size for images in inactive list
-  int         res;                  // 0 or error code to return
+  int         res= 0;               // 0 or error code to return
 
   size_t      start_bytes= s.bytes;
 
@@ -313,6 +335,7 @@ int write_table_data(THD*, Backup_info &
     if (!p || !p->is_valid())
     {
       info.report_error(ER_OUT_OF_RESOURCES);
+      res= -1;
       goto error;
     }
 
@@ -320,8 +343,11 @@ int write_table_data(THD*, Backup_info &
 
     if (init_size == Driver::UNKNOWN_SIZE)
     {
-      if ((res= sch.add(p)))
+      if (sch.add(p))
+      {
+        res= -2;
         goto error;
+      }
     }
     else
     {
@@ -349,8 +375,9 @@ int write_table_data(THD*, Backup_info &
   BACKUP_SYNC("data_init");
 
   /*
-   poll "at end" drivers activating inactive ones on the way
-   note: if scheduler is empty and there are images with non-zero
+   Poll "at end" drivers activating inactive ones on the way.
+
+   Note: if scheduler is empty and there are images with non-zero
    init size (max_init_size > 0) then enter the loop as one such image
    will be added to the scheduler inside.
   */
@@ -381,14 +408,20 @@ int write_table_data(THD*, Backup_info &
 
       max_init_size= second_max;
 
-      if ((res= sch.add(p1)))
+      if (sch.add(p1))
+      {
+        return -3;
         goto error;
+      }
     }
 
     // poll drivers
 
-    if ((res= sch.step()))
+    if (sch.step())
+    {
+      res= -4;
       goto error;
+    }
   }
 
   {
@@ -399,41 +432,62 @@ int write_table_data(THD*, Backup_info &
     Scheduler::Pump *p;
 
     while ((p= it1++))
-      if ((res= sch.add(p)))
-        goto error;
+    if (sch.add(p))
+    {
+      res= -5;
+      goto error;
+    };
 
     while (sch.init_count > 0)
-      if ((res= sch.step()))
-        goto error;
+    if (sch.step())
+    {
+      res= -6;
+      goto error;
+    };
 
     // prepare for VP
     DBUG_PRINT("backup/data",("-- PREPARE PHASE --"));
     BACKUP_SYNC("data_prepare");
 
-    if ((res= sch.prepare()))
+    if (sch.prepare())
+    {
+      res= -7;
       goto error;
+    }
 
     while (sch.prepare_count > 0)
-      if ((res= sch.step()))
-        goto error;
+    if (sch.step())
+    {
+      res= -8;
+      goto error;
+    }
 
     // VP creation
     DBUG_PRINT("backup/data",("-- SYNC PHASE --"));
     BACKUP_SYNC("data_lock");
-    if ((res= sch.lock()))
+    if (sch.lock())
+    {
+      res= -9;
       goto error;
+    }
 
     BACKUP_SYNC("data_unlock");
-    if ((res= sch.unlock()))
+    if (sch.unlock())
+    {
+      res= -10;
       goto error;
+    }
 
     // get final data from drivers
     DBUG_PRINT("backup/data",("-- FINISH PHASE --"));
     BACKUP_SYNC("data_finish");
 
     while (sch.finish_count > 0)
-      if ((res= sch.step()))
-        goto error;
+    if (sch.step())
+    {
+      res= -11;
+      goto error;
+    }
 
     DBUG_PRINT("backup/data",("-- DONE --"));
   }
@@ -444,7 +498,7 @@ int write_table_data(THD*, Backup_info &
 
  error:
 
-  DBUG_RETURN(res ? res : -1);
+  DBUG_RETURN(res ? res : -12);
 }
 
 } // backup namespace
@@ -496,18 +550,13 @@ struct Scheduler::Pump_iterator
  */
 int Scheduler::step()
 {
-  /* find pump with least data read.
-
-     Pick next driver to pump data from. The driver with least data read
-     is choosen. If there are several, they are polled in round-robin
-     fashion.
-
-     TODO: implement this
-   */
+  // Pick next driver to pump data from.
 
   Pump_iterator p(*this);
 
 /*
+  An attempt to implement more advanced scheduling strategy (not working).
+
   for (Pump_ptr it(m_pumps); it; ++it)
     if ( !p || it->pos() < p->pos() )
       p= it;
@@ -536,7 +585,7 @@ int Scheduler::step()
 
   // update statistics
 
-  if (howmuch > 0)
+  if (!res && howmuch > 0)
   {
     m_total += howmuch;
 
@@ -583,8 +632,11 @@ int Scheduler::step()
 
     case backup_state::DONE:
       p->end();
+
     case backup_state::ERROR:
       remove_pump(p);
+      if (res)
+        cancel_backup(); // we hit an error - bail out
       break;
 
     default: break;
@@ -594,16 +646,13 @@ int Scheduler::step()
                               m_count, init_count, prepare_count, finish_count));
   }
 
-  if (res) // we hit an error - bail out
-   cancel_backup();
-
-  return res;
+  return res ? -1 : 0;
 }
 
 /**
   Add backup pump to the scheduler.
 
-  In case of error, the pump is deleted.
+  The pump is initialized with begin() call. In case of error, it is deleted.
  */
 int Scheduler::add(Pump *p)
 {
@@ -625,9 +674,7 @@ int Scheduler::add(Pump *p)
   m_count++;
   m_total += avg;
 
-  int res= 0;
-
-  if ((res= p->begin()))
+  if (p->begin())
     goto error;
 
   // in case of error, above call should return non-zero code (and report error)
@@ -666,7 +713,7 @@ int Scheduler::add(Pump *p)
 
   delete p;
   cancel_backup();
-  return res;
+  return -1;
 }
 
 /// Move backup pump to the end of scheduler's list.
@@ -713,7 +760,7 @@ void Scheduler::remove_pump(Pump_iterato
 /// Shut down backup process.
 void Scheduler::cancel_backup()
 {
-  if (canceled)
+  if (cancelled)
     return;
 
   // shutdown any remaining drivers
@@ -724,26 +771,24 @@ void Scheduler::cancel_backup()
     remove_pump(p);
   }
 
-  canceled= TRUE;
+  cancelled= TRUE;
 }
 
 
 /// Start prepare phase for all drivers.
 int Scheduler::prepare()
 {
-  DBUG_ASSERT(!canceled);
+  DBUG_ASSERT(!cancelled);
   // we should start prepare phase only when init phase is finished
   DBUG_ASSERT(init_count==0);
   DBUG_PRINT("backup/data",("calling prepare() for all drivers"));
 
-  int res= 0;
-
   for (Pump_iterator it(*this); it; ++it)
   {
-    if ((res= it->prepare()))
+    if (it->prepare())
     {
       cancel_backup();
-      return res;
+      return -1;
     }
     if (it->state == backup_state::PREPARING)
      prepare_count++;
@@ -757,18 +802,16 @@ int Scheduler::prepare()
 /// Lock all drivers.
 int Scheduler::lock()
 {
-  DBUG_ASSERT(!canceled);
+  DBUG_ASSERT(!cancelled);
   // lock only when init and prepare phases are finished
   DBUG_ASSERT(init_count==0 && prepare_count==0);
   DBUG_PRINT("backup/data",("calling lock() for all drivers"));
 
-  int res= 0;
-
   for (Pump_iterator it(*this); it; ++it)
-   if ((res= it->lock()))
+   if (it->lock())
    {
      cancel_backup();
-     return res;
+     return -1;
    }
 
   DBUG_PRINT("backup/data",("driver counts: total=%u, init=%u, prepare=%u, finish=%u.",
@@ -779,17 +822,15 @@ int Scheduler::lock()
 /// Unlock all drivers.
 int Scheduler::unlock()
 {
-  DBUG_ASSERT(!canceled);
+  DBUG_ASSERT(!cancelled);
   DBUG_PRINT("backup/data",("calling unlock() for all drivers"));
 
-  int res= 0;
-
   for(Pump_iterator it(*this); it; ++it)
   {
-    if ((res= it->unlock()))
+    if (it->unlock())
     {
       cancel_backup();
-      return res;
+      return -1;
     }
     if (it->state == backup_state::FINISHING)
       finish_count++;
@@ -842,7 +883,7 @@ int Backup_pump::begin()
     // We check if logger is always setup. Later the assertion can
     // be replaced with "if (m_log)"
     DBUG_ASSERT(m_log);
-      m_log->report_error("Can't initialize backup driver %s",m_name);
+      m_log->report_error(ER_BACKUP_INIT_BACKUP_DRIVER,m_name);
     return -1;
   }
 
@@ -860,7 +901,7 @@ int Backup_pump::end()
     {
       state= backup_state::ERROR;
       DBUG_ASSERT(m_log);
-        m_log->report_error("Can't close backup driver %s",m_name);
+        m_log->report_error(ER_BACKUP_STOP_BACKUP_DRIVER,m_name);
       return -1;
     }
 
@@ -889,10 +930,8 @@ int Backup_pump::prepare()
   default:
     state= backup_state::ERROR;
     DBUG_ASSERT(m_log);
-      m_log->report_error("Error when preparing backup driver %s for"
-                          " synchronization (result=%d)", m_name, (int)res);
-    return -1;
-
+      m_log->report_error(ER_BACKUP_PREPARE_DRIVER, m_name);
+      return -1;
   }
 
   DBUG_PRINT("backup/data",(" preparing %s, goes to %s state",
@@ -908,7 +947,7 @@ int Backup_pump::lock()
   {
     state= backup_state::ERROR;
     DBUG_ASSERT(m_log);
-      m_log->report_error("Can't create VP of %s image",m_name);
+      m_log->report_error(ER_BACKUP_CREATE_VP,m_name);
     return -1;
   }
 
@@ -924,7 +963,7 @@ int Backup_pump::unlock()
   {
     state= backup_state::ERROR;
     DBUG_ASSERT(m_log);
-      m_log->report_error("Can't unlock backup driver %s",m_name);
+      m_log->report_error(ER_BACKUP_UNLOCK_DRIVER,m_name);
     return -1;
   }
 
@@ -937,10 +976,10 @@ int Backup_pump::cancel()
   {
     state= backup_state::ERROR;
     DBUG_ASSERT(m_log);
-      m_log->report_error("Error when cancelling backup process of %s image",m_name);
+      m_log->report_error(ER_BACKUP_CANCEL_BACKUP,m_name);
     return -1;
   }
-  state= backup_state::CANCELED;
+  state= backup_state::CANCELLED;
   return 0;
 }
 
@@ -967,7 +1006,7 @@ int Backup_pump::pump(size_t *howmuch)
   // pumping not allowed in these states
   DBUG_ASSERT(state != backup_state::INACTIVE);
   DBUG_ASSERT(state != backup_state::SHUT_DOWN);
-  DBUG_ASSERT(state != backup_state::CANCELED);
+  DBUG_ASSERT(state != backup_state::CANCELLED);
 
   // we have detected error before - report it once more
   if (state == backup_state::ERROR)
@@ -1008,8 +1047,10 @@ int Backup_pump::pump(size_t *howmuch)
   {
     if (mode == READING)
     {
-      // If m_buf_head is NULL then a new request to the driver
-      // should be made. We should allocate a new output buffer.
+      /*
+        If m_buf_head is NULL then a new request to the driver
+        should be made. We should allocate a new output buffer.
+       */
 
       if (!m_buf_head)
         switch (m_bw.get_buf(m_buf)) {
@@ -1017,7 +1058,7 @@ int Backup_pump::pump(size_t *howmuch)
         case Block_writer::OK:
           m_buf_retries= 0;
           m_buf_head= m_buf.data;
-          m_buf.data+= 4; // leave space for image and stram numbers
+          m_buf.data+= 4; // leave space for image and stream numbers
           m_buf.size-= 4;
           break;
 
@@ -1028,10 +1069,9 @@ int Backup_pump::pump(size_t *howmuch)
         case Block_writer::ERROR:
         default:
           DBUG_ASSERT(m_log);
-            m_log->report_error("Can't get stream window for writing %s backup"
-                                " image data", m_name);
+            m_log->report_error(ER_BACKUP_GET_BUF);
           state= backup_state::ERROR;
-          return -1;
+          return -2;
         }
 
       DBUG_ASSERT(m_buf_head);
@@ -1070,7 +1110,6 @@ int Backup_pump::pump(size_t *howmuch)
 
         if ( m_buf.size > 0 )
         {
-          // TODO: use OStream interface for consistency
           m_buf.size+= 4;
           int2store(m_buf.data,m_drv_no+1);
           int2store(m_buf.data+2,m_buf.table_no);
@@ -1088,18 +1127,14 @@ int Backup_pump::pump(size_t *howmuch)
       case ERROR:
       default:
         DBUG_ASSERT(m_log);
-          m_log->report_error("Error when polling data from backup driver %s",
-                              m_name);
+          m_log->report_error(ER_BACKUP_GET_DATA,m_name);
         state= backup_state::ERROR;
-        return -1;
+        return -3;
 
       case BUSY:
 
         m_bw.drop_buf(m_buf);
         m_buf_head=NULL;  // thus a new request will be made
-
-      // TODO: case DONE:
-
       }
 
     } // if (mode == READING)
@@ -1121,9 +1156,9 @@ int Backup_pump::pump(size_t *howmuch)
       case Block_writer::ERROR:
 
         DBUG_ASSERT(m_log);
-          m_log->report_error("Error when writing %s backup image data", m_name);
+          m_log->report_error(ER_BACKUP_WRITE_DATA, m_name, m_buf.table_no);
         state= backup_state::ERROR;
-        return -1;
+        return -4;
 
       default:  // retry write
         break;
@@ -1136,12 +1171,12 @@ int Backup_pump::pump(size_t *howmuch)
     DBUG_PRINT("backup/data",(" %s changes state %s->%s",
                               m_name,backup_state::name[before_state],
                                      backup_state::name[state]));
-
   return 0;
 }
 
 } // backup namespace
 
+
 /***********************************************
 
                   DATA RESTORE
@@ -1150,35 +1185,30 @@ int Backup_pump::pump(size_t *howmuch)
 
 namespace backup {
 
+
 /**
   Read backup image data from a backup stream and forward it to restore drivers.
-
-  @returns 0 on success.
  */
-
 int restore_table_data(THD*, Restore_info &info, IStream &s)
 {
   DBUG_ENTER("restore::restore_table_data");
 
   enum { READING, SENDING, DONE, ERROR } state= READING;
 
-  // FIXME: when selective restores are implemented, check that
-  // nothing was *selected* to be restored.
-
   if (info.img_count==0 || info.table_count==0) // nothing to restore
     DBUG_RETURN(0);
 
-  result_t res;
   Restore_driver* drv[MAX_IMAGES];
 
   if (info.img_count > MAX_IMAGES)
   {
-    info.report_error("Too many images in backup archive (%d vs. %d)",
-                      info.img_count,MAX_IMAGES);
+    info.report_error(ER_BACKUP_TOO_MANY_IMAGES, info.img_count, MAX_IMAGES);
     DBUG_RETURN(-1);
   }
 
   // Initializing restore drivers
+  result_t res;
+  int      ret=0;
 
   for (uint no=0; no < info.img_count; ++no)
   {
@@ -1190,16 +1220,19 @@ int restore_table_data(THD*, Restore_inf
     if (!img)
       continue;
 
-    if (backup::ERROR == img->get_restore_driver(drv[no]))
+    res= img->get_restore_driver(drv[no]);
+    if (res == backup::ERROR)
     {
-      info.report_error("Can't create restore driver for %s image",img->name());
+      info.report_error(ER_BACKUP_CREATE_RESTORE_DRIVER,img->name());
+      ret= -2;
       goto error;
     };
 
-    res= drv[no]->begin(0); // TODO: provide correct size
+    res= drv[no]->begin(0);
     if (res == backup::ERROR)
     {
-      info.report_error("Can't initialize restore driver for %s image",img->name());
+      info.report_error(ER_BACKUP_INIT_RESTORE_DRIVER,img->name());
+      ret= -3;
       goto error;
     }
   }
@@ -1214,6 +1247,9 @@ int restore_table_data(THD*, Restore_inf
 
     size_t start_bytes= s.bytes;
 
+    Restore_driver  *drvr= NULL;  // pointer to the current driver
+    Image_info      *img= NULL;   // corresponding restore image object
+
     // main data reading loop
 
     while ( state != DONE && state != ERROR )
@@ -1233,9 +1269,10 @@ int restore_table_data(THD*, Restore_inf
           break;
 
         case stream_result::ERROR:
-          info.report_error("Error reading backup stream");
+          info.report_error(ER_BACKUP_READ_DATA);
         default:
           state= ERROR;
+          ret= -4;
           goto error;
 
         }
@@ -1250,20 +1287,28 @@ int restore_table_data(THD*, Restore_inf
         buf.data += 4;
         buf.size -= 4;
 
-        DBUG_PRINT("restore",("Got %lu bytes of %s image data (for table #%u)",
-                   (unsigned long)buf.size,
-                   info.images[img_no-1]->name(),
-                   buf.table_no));
-
-      case SENDING:
-
-        if( img_no < 1 || img_no > info.img_count || !drv[img_no-1] )
+        if (img_no < 1 || img_no > info.img_count || !(drvr= drv[img_no-1]))
         {
           DBUG_PRINT("restore",("Skipping data for image #%u",img_no));
           state= READING;
           break;
         }
 
+        img= info.images[img_no-1];
+        // Each restore driver should have corresponding Image_info object.
+        DBUG_ASSERT(img);
+
+        DBUG_PRINT("restore",("Got %lu bytes of %s image data (for table #%u)",
+                   (unsigned long)buf.size, img->name(), buf.table_no));
+
+      case SENDING:
+
+        /*
+          If we are here, the img pointer should point at the image for which
+          we have next data block and drvr at its restore driver.
+         */
+        DBUG_ASSERT(img && drvr);
+
         /*
           If the global variable "backup_sleep" has been set, use that value
           to cause the kernel to sleep in seconds.
@@ -1271,13 +1316,15 @@ int restore_table_data(THD*, Restore_inf
         if (backup_sleep)
           sleep(backup_sleep);
 
-        switch( drv[img_no-1]->send_data(buf) ) {
+        switch( drvr->send_data(buf) ) {
 
         case backup::OK:
           switch ((int)s.next_chunk()) {
 
           case stream_result::OK:
             state= READING;
+            img= NULL;
+            drvr= NULL;
             repeats= 0;
             break;
 
@@ -1286,22 +1333,21 @@ int restore_table_data(THD*, Restore_inf
             break;
 
           case stream_result::ERROR:
-            info.report_error("Error when switching to next data chunk in backup stream");
+            info.report_error(ER_BACKUP_NEXT_CHUNK);
           default:
             state= ERROR;
+            ret= -5;
             goto error;
-
           }
 
           break;
 
         case backup::ERROR:
-          // TODO: inform about ignored errors
           if( errors > MAX_ERRORS )
           {
-            info.report_error("Error when sending data to restore driver: %s",
-                               info.images[img_no-1]->name());
+            info.report_error(ER_BACKUP_SEND_DATA, buf.table_no, img->name());
             state= ERROR;
+            ret= -6;
             goto error;
           }
           errors++;
@@ -1312,19 +1358,17 @@ int restore_table_data(THD*, Restore_inf
         default:
           if( repeats > MAX_REPEATS )
           {
-            info.report_error("Can't send data block to restoredriver: %s",
-                              info.images[img_no-1]->name());
+            info.report_error(ER_BACKUP_SEND_DATA_RETRY, repeats, img->name());
             state= ERROR;
+            ret= -7;
             goto error;
           }
           repeats++;
           break;
-
         }
 
       default:
         break;
-
       } // switch(state)
 
     } // main reading loop
@@ -1347,25 +1391,24 @@ int restore_table_data(THD*, Restore_inf
 
       DBUG_PRINT("restore",("Shutting down restore driver %s",
                             info.images[no]->name()));
-      if (drv[no]->end() == backup::ERROR)
+      res= drv[no]->end();
+      if (res == backup::ERROR)
       {
-        char buf[16];
-        my_snprintf(buf,sizeof(buf),"%d",no);
-
+        ret= -8;
         state= ERROR;
+
         if (!bad_drivers.is_empty())
           bad_drivers.append(",");
-        bad_drivers.append(buf);
+        bad_drivers.append(info.images[no]->name());
       }
       drv[no]->free();
     }
 
     if (!bad_drivers.is_empty())
-      info.report_error("Error when shutting down restore drivers no %s",
-                        bad_drivers.c_ptr());
+      info.report_error(ER_BACKUP_STOP_RESTORE_DRIVERS, bad_drivers.c_ptr());
   }
 
-  DBUG_RETURN(state == ERROR ? -1 : 0);
+  DBUG_RETURN(state == ERROR ? -9 : 0);
 
  error:
 
@@ -1380,7 +1423,7 @@ int restore_table_data(THD*, Restore_inf
     drv[no]->free();
   }
 
-  DBUG_RETURN(-2);
+  DBUG_RETURN(ret);
 }
 
 } // backup namespace
@@ -1399,7 +1442,7 @@ namespace backup {
 
   The buffer size is given by @c buf.size member.
 
-  Current implementation tries to allocate the data transfer bufer in the
+  Current implementation tries to allocate the data transfer buffer in the
   stream. It can handle only one buffer at a time.
 
   @returns @c NO_RES if buffer can not be allocated, @c OK otherwise.
@@ -1422,7 +1465,7 @@ Block_writer::get_buf(Buffer &buf)
   Write block of data to stream.
 
   The buffer containing data must be obtained from a previous @c get_buf() call.
-  After this call, bufer is returned to the buffer pool and can be reused for
+  After this call, buffer is returned to the buffer pool and can be reused for
   other transfers.
  */
 Block_writer::result_t
@@ -1450,5 +1493,3 @@ Block_writer::drop_buf(Buffer &buf)
 }
 
 } // backup namespace
-
-
===== debug.h 1.4 vs edited =====
--- 1.4/sql/backup/debug.h	2007-06-23 22:56:36 +02:00
+++ edited/debug.h	2007-06-23 21:58:24 +02:00
@@ -3,7 +3,10 @@
 
 #define BACKUP_SYNC_TIMEOUT 300
 
-// TODO: decide how to configure DeBUG_BACKUP
+/*
+  TODO
+  - decide how to configure DEBUG_BACKUP
+ */
 
 #ifndef DBUG_OFF
 # define DBUG_BACKUP
@@ -11,6 +14,25 @@
 
 #ifdef DBUG_BACKUP
 
+/*
+  Macros for debugging error (or other) conditions. Usage:
+
+  TEST_ERROR_IF(<condition deciding if TEST_ERROR should be true>);
+
+  if (<other conditions> || TEST_ERROR)
+  {
+    <report error>
+  }
+
+  The additional TEST_ERROR condition will be set only if "backup_error_test"
+  error injection is set in the server.
+
+  Notes:
+   - Whenever TEST_ERROR is used in a condition, TEST_ERROR_IF() should
+     be called before - otherwise TEST_ERROR might be unintentionally TRUE.
+   - This mechanism is not thread safe.
+ */
+
 namespace backup {
  extern bool test_error_flag;
 }
@@ -19,10 +41,28 @@ namespace backup {
 // FIXME: DBUG_EXECUTE_IF below doesn't work
 #define TEST_ERROR_IF(X) \
  do { \
+   backup::test_error_flag= FALSE; \
    DBUG_EXECUTE_IF("backup_error_test", backup::test_error_flag= (X);); \
  } while(0)
 
-// TODO: set correct info in thd->proc_info
+/*
+  Macros for creating synchronization points in tests.
+
+  Usage
+
+  In the backup code:
+    BACKUP_SYNC("<synchronization point name>");
+
+  In a client:
+    SELECT get_lock("<synchronization point name>",<timeout>);
+    ...
+    SELECT release_lock("<synchronization point name>");
+
+  If the lock is kept by a client, server code will wait on the corresponding
+  BACKUP_SYNC() until it is released.
+
+  Consider: set thd->proc_info when waiting on lock
+ */
 
 #define BACKUP_SYNC(S) \
  do { \
===== logger.cc 1.2 vs edited =====
--- 1.2/sql/backup/logger.cc	2007-06-23 22:56:36 +02:00
+++ edited/logger.cc	2007-06-23 21:58:24 +02:00
@@ -17,7 +17,7 @@ namespace backup {
                      for other messages set to 0
   @param msg         message text
 
-  @returns 0 on succes.
+  @returns 0 on success.
  */
 int Logger::write_message(log_level::value level, int error_code,
                           const char *msg)
@@ -51,7 +51,7 @@ int Logger::write_message(log_level::val
   @param level       level of the message (INFO,WARNING,ERROR)
   @param error_code  code assigned to the message in errmsg.txt
 
-  If the message contains placeholders, additioal arguments provide
+  If the message contains placeholders, additional arguments provide
   values to be put there.
 
   @returns 0 on success.
@@ -64,7 +64,7 @@ int Logger::v_report_error(log_level::va
 /**
   Output unregistered message.
 
-  Format string is given explicitely as an argument.
+  Format string is given explicitly as an argument.
 
   Note: no localization support.
  */
===== logger.h 1.2 vs edited =====
--- 1.2/sql/backup/logger.h	2007-06-23 22:56:36 +02:00
+++ edited/logger.h	2007-06-23 22:14:12 +02:00
@@ -47,7 +47,7 @@ class Logger
      return res;
    }
 
-   /// Calls @c report_error() with log_level::ERROR.
+   /// Reports error with log_level::ERROR.
    int report_error(int error_code, ...)
    {
      va_list args;
@@ -59,6 +59,7 @@ class Logger
      return res;
    }
 
+   /// Reports error with registered error description string.
    int report_error(log_level::value level, int error_code, ...)
    {
      va_list args;
@@ -70,6 +71,7 @@ class Logger
      return res;
    }
 
+   /// Reports error with given description.
    int report_error(const char *format, ...)
    {
      va_list args;
@@ -81,6 +83,7 @@ class Logger
      return res;
    }
 
+   ///  Request that all reported errors are saved in the logger.
    void save_errors()
    {
      if (m_save_errors)
@@ -89,6 +92,7 @@ class Logger
      m_save_errors= TRUE;
    }
 
+   /// Stop saving errors.
    void stop_save_errors()
    {
      if (!m_save_errors)
@@ -96,16 +100,18 @@ class Logger
      m_save_errors= FALSE;
    }
 
+   /// Delete all saved errors to free resources.
    void clear_saved_errors()
    { errors.delete_elements(); }
 
+   /// Return a pointer to most recent saved error.
    MYSQL_ERROR *last_saved_error()
    { return errors.head(); }
 
  private:
 
-  List<MYSQL_ERROR> errors;
-  bool m_save_errors;
+  List<MYSQL_ERROR> errors;  ///< Used to store saved errors
+  bool m_save_errors;        ///< Flag telling if errors should be saved
 
   int v_report_error(log_level::value,int,va_list);
   int v_write_message(log_level::value,int, const char*,va_list);
===== map.h 1.2 vs edited =====
--- 1.2/sql/backup/map.h	2007-06-23 22:56:36 +02:00
+++ edited/map.h	2007-06-23 21:58:24 +02:00
@@ -26,13 +26,13 @@ class Map
   bool  occupied(const Key&) const;
   size_t count() const { return m_count; };
 
-  Map(): m_count(0) {};
+  Map(): m_count(0) {}
 
 #ifdef MAP_DEBUG
   void print();
 #endif
 
-  ~Map() { clear(); };
+  ~Map() { clear(); }
 
   void clear();
 
@@ -69,16 +69,18 @@ bool Map<D,K>::occupied(const Key &k) co
 }
 
 template<class D, class K>
+inline
 typename Map<D,K>::El &
 Map<D,K>::operator[](const Key &k) const
 {
-  if( !occupied(k) )
+  if (!occupied(k))
     return const_cast<El&>(D::null);
 
   return *(entries[k].el);
 }
 
 template<class D, class K>
+inline
 typename Map<D,K>::Key
 Map<D,K>::add(const El &e)
 {
@@ -87,6 +89,7 @@ Map<D,K>::add(const El &e)
 }
 
 template<class D, class K>
+inline
 typename Map<D,K>::Key
 Map<D,K>::find(const El &e) const
 {
@@ -94,15 +97,16 @@ Map<D,K>::find(const El &e) const
   return const_cast<Map<D,K>*>(this)->find_el(k,e);
 }
 
-// Assumption, k is valid.
+// PRE: k is valid.
 template<class D, class K>
+inline
 typename Map<D,K>::Key
 Map<D,K>::find_el(const Key &k, const El &e, bool insert)
 {
   El *x= entries[k].el;
 
-  if( !x )
-    if( insert )
+  if (!x)
+    if (insert)
     {
       set(k,e);
       return k;
@@ -111,41 +115,43 @@ Map<D,K>::find_el(const Key &k, const El
 
   int res;
 
-  if( (res= D::cmp(e,*x)) == 0 )
+  if ((res= D::cmp(e,*x)) == 0)
     return k;
 
   Key &k1 = res>0 ? entries[k].bigger : entries[k].smaller;
 
-  if( K::valid_key(k1) )
+  if (K::valid_key(k1))
     return find_el(k1,e);
 
   Key k2;
-  if( K::valid_key(k2= find_free_loc()) )
+  if (K::valid_key(k2= find_free_loc()))
   {
     k1= k2;
     set(k2,e);
-  };
+  }
 
   return k2;
 }
 
 
 template<class D, class K>
+inline
 typename Map<D,K>::Key
 Map<D,K>::find_free_loc() const
 {
-  if( m_count >= K::size )
+  if (m_count >= K::size)
     return K::null;
 
-  for(uint k=0 ; k < size ; k++ )
-    if( entries[k].el == NULL )
+  for (uint k=0; k < size; k++)
+    if (entries[k].el == NULL)
       return k;
 
   return K::null;
 }
 
-// Assumption: k is valid
+// PRE k is valid
 template<class D, class K>
+inline
 void Map<D,K>::set(const Key &k, const El &e)
 {
   entries[k]= e;
@@ -153,10 +159,11 @@ void Map<D,K>::set(const Key &k, const E
 }
 
 template<class D, class K>
+inline
 void Map<D,K>::clear()
 {
-  for(uint i=0 ; i < size ; i++)
-   if( entries[i].el )
+  for (uint i=0; i < size; i++)
+   if (entries[i].el)
      delete entries[i].el;
 }
 
@@ -186,7 +193,8 @@ inline
 key8 &key8::operator=(unsigned int x)
 {
   val= x & 0xFF;
-  for( int bits= sizeof(unsigned int) ; bits > 8 ; bits-= 8 )
+  // simple hashing
+  for (int bits= sizeof(unsigned int) ; bits > 8 ; bits-= 8)
   {
     x >>= 8;
     val ^= x &0xFF;
===== meta_backup.cc 1.18 vs edited =====
--- 1.18/sql/backup/meta_backup.cc	2007-06-23 22:56:36 +02:00
+++ edited/meta_backup.cc	2007-06-23 21:58:24 +02:00
@@ -1,33 +1,42 @@
 /**
   @file
 
-  Code to save/read meta-data image.
+  Functions used by kernel to save/restore meta-data
  */
 
 /*
   TODO:
 
   - Handle events, routines, triggers and other item types (use Chuck's code).
+
+  - When saving database info, save its default charset and collation.
+    Alternatively, save a complete "CREATE DATABASE ..." statement with all
+    clauses which should work even when new clauses are added in the future.
+
+  - In silent_exec_query() - reset, collect and report errors from statement
+    execution.
+
+  - In the same function: investigate what happens if query triggers error -
+    there were signals that the code might hang in that case (some clean-up
+    needed?)
  */
 
 #include <mysql_priv.h>
 #include <sql_show.h>
-//#include <event_scheduler.h>
-//#include <sp.h>
-//#include <sql_trigger.h>
-//#include <log.h>
+/*
+  Might be needed for other meta-data items
+
+#include <event_scheduler.h>
+#include <sp.h>
+#include <sql_trigger.h>
+#include <log.h>
+*/
 
 #include "backup_aux.h"
 #include "backup_kernel.h"
 #include "meta_backup.h"
 
 
-/*******************************************************
-
-   Functions used by kernel to save/restore meta-data
-
- *******************************************************/
-
 namespace backup {
 
 /**
@@ -60,7 +69,7 @@ int write_meta_data(THD *thd, Backup_inf
     if (res != OK)
     {
       meta::Item::description_buf buf;
-      info.report_error("Error when saving meta-data of %s",
+      info.report_error(ER_BACKUP_WRITE_META,
                          it->meta().describe(buf,sizeof(buf)));
       DBUG_RETURN(-1);
     }
@@ -79,8 +88,8 @@ int write_meta_data(THD *thd, Backup_inf
   Read meta-data items from a stream and create them if they are selected
   for restore.
 
-  @pre  Stream is at the beginning of the meta-data chunk.
-  @post Stream is at the beginning of the first chunk after the meta-data one.
+  @pre  Stream is at the beginning of a saved meta-data chunk.
+  @post Stream is at the beginning of the next chunk.
  */
 int
 restore_meta_data(THD *thd, Restore_info &info, IStream &s)
@@ -101,9 +110,10 @@ restore_meta_data(THD *thd, Restore_info
     {
      DBUG_PRINT("restore",("  creating it!"));
 
-     // change the current database if we are going to create a per-db item
-     // and we are not already in the correct one.
-
+     /*
+       change the current database if we are going to create a per-db item
+       and we are not already in the correct one.
+      */
      const Db_ref db= it->meta().in_db();
 
      if (db.is_valid() && (!curr_db.is_valid() || db != curr_db))
@@ -116,7 +126,7 @@ restore_meta_data(THD *thd, Restore_info
      if (OK != (res= it->meta().create(thd)))
      {
        meta::Item::description_buf buf;
-       info.report_error("Error when creating %s",
+       info.report_error(ER_BACKUP_CREATE_META,
                           it->meta().describe(buf,sizeof(buf)));
        DBUG_RETURN(-1);
      }
@@ -129,7 +139,7 @@ restore_meta_data(THD *thd, Restore_info
 
   if (res != DONE)
   {
-    info.report_error("Error when reading meta-data item info");
+    info.report_error(ER_BACKUP_READ_META);
     DBUG_RETURN(-2);
   }
 
@@ -137,7 +147,7 @@ restore_meta_data(THD *thd, Restore_info
 
   if (stream_result::ERROR == s.next_chunk())
   {
-    info.report_error("Error when closing chunk after reading meta-data info");
+    info.report_error(ER_BACKUP_NEXT_CHUNK);
     DBUG_RETURN(-3);
   };
 
@@ -229,7 +239,7 @@ meta::Item::create(THD *thd)
   </pre>
   strings @<object type> and @<name> are returned by @c X::sql_object_name()
   and @c X::sql_name() methods of the class @c meta::X representing the item.
-  If neccessary, method @c drop() can be overwriten in a specialized class
+  If necessary, method @c drop() can be overwritten in a specialized class
   corresponding to a given type of meta-data item.
 
   @returns OK or ERROR
@@ -238,8 +248,12 @@ result_t
 meta::Item::drop(THD *thd)
 {
   const char *ob= sql_object_name();
-  DBUG_ASSERT(ob); // an item should define object name for DROP statement
-                   // or redefine drop() method.
+
+  /*
+    An item class should define object name for DROP statement
+    or redefine drop() method.
+   */
+  DBUG_ASSERT(ob);
 
   String drop_stmt;
 
@@ -258,10 +272,6 @@ meta::Item::drop(THD *thd)
 
   Currently we don't save anything. A database is always created using
   "CREATE DATABASE @<name>" statement.
-
- TODO: Save info about db charset and collation. Alternatively, save a complete
- "CREATE DATABASE ..." statement with all clauses which should work even when
- new clauses are added in the future.
  */
 result_t
 meta::Db::save(THD*,OStream&)
@@ -287,7 +297,7 @@ meta::Db::read(IStream&)
 /**** SAVE/RESTORE TABLES ***************************************/
 
 /**
-  Build a CREATE steatement for a table.
+  Build a CREATE statement for a table.
 
   We use @c store_create_info() function defined in the server. For that
   we need to open the table. After building the statement the table is closed to
@@ -337,10 +347,10 @@ namespace backup {
 
 /// Execute SQL query without sending anything to client.
 
-// Change net.vio idea taken from execute_init_command in sql_parse.cc
-// TODO: investigate what happens if query triggers error - there were
-// signals that the code might hang in that case (some clean-up needed?)
-// TODO: reset and collect errors from thread execution.
+/*
+  Note: the change net.vio idea taken from execute_init_command in
+  sql_parse.cc
+ */
 
 int silent_exec_query(THD *thd, String &query)
 {
@@ -380,7 +390,7 @@ int silent_exec_query(THD *thd, String &
 
 /**********************************************************
 
-  Old code to intorporate into above framework: handle
+  Old code to incorporate into above framework: handles
   events, routines and triggers
 
  **********************************************************/
===== meta_backup.h 1.2 vs edited =====
--- 1.2/sql/backup/meta_backup.h	2007-06-23 22:56:36 +02:00
+++ edited/meta_backup.h	2007-06-23 21:58:24 +02:00
@@ -87,8 +87,8 @@ class Item
 
   /// Store in @c create_stmt a DDL statement which will create the item.
   /*
-    we give default implementation because an item can not use create
-    statements and then it doesn't have to worry about that method
+    We give a default implementation because an item can not use create
+    statements and then it doesn't have to worry about this method.
   */
   virtual int build_create_stmt(THD*)
   { return -1; }
===== sql_backup.cc 1.32 vs edited =====
--- 1.32/sql/backup/sql_backup.cc	2007-06-23 22:56:36 +02:00
+++ edited/sql_backup.cc	2007-06-23 21:58:24 +02:00
@@ -10,11 +10,14 @@
   - Handle SHOW BACKUP ARCHIVE command.
   - Add database list to the backup/restore summary.
   - Implement meta-data freeze during backup/restore.
-  - Error feedback to client.
   - Improve logic selecting backup driver for a table.
   - Handle other types of meta-data in Backup_info methods.
-  - Handle item dependencied when adding new items.
+  - Handle item dependencies when adding new items.
+  - Lock I_S tables when reading table list and similar (is it needed?)
+  - When reading table list from I_S tables, use select conditions to
+    limit amount of data read. (check prepare_select_* functions in sql_help.cc)
   - Handle other kinds of backup locations (far future).
+  - Complete feedback given by BACKUP/RESTORE statements (send_summary function).
  */
 
 #include "../mysql_priv.h"
@@ -60,9 +63,7 @@ bool test_error_flag= FALSE;
 
   @param lex  results of parsing the statement.
 
-  This function sends response to the client (ok, result set or error).
-
-  @returns 0 on success.
+  @note This function sends response to the client (ok, result set or error).
  */
 
 int
@@ -81,7 +82,7 @@ execute_backup_command(THD *thd, LEX *le
     DBUG_RETURN(ER_BACKUP_INVALID_LOC);
   }
 
-  int res= -1;
+  int res= 0;
 
   switch (lex->sql_command) {
 
@@ -92,40 +93,49 @@ execute_backup_command(THD *thd, LEX *le
 
     if (!stream)
     {
-      res= ER_BACKUP_READ;
-      my_error(res,MYF(0),loc->describe());
+      res= -1;
+      my_error(ER_BACKUP_READ_LOC,MYF(0),loc->describe());
       goto finish_restore;
     }
     else
     {
       backup::Restore_info info(*stream);
 
-      if ((res= check_info(thd,info)))
+      if (check_info(thd,info))
+      {
+        res= -2;
         goto finish_restore;
-
+      }
 
       if (lex->sql_command == SQLCOM_SHOW_ARCHIVE)
       {
-        // FIXME: implement this
         my_error(ER_NOT_ALLOWED_COMMAND,MYF(0));
-        // res= mysql_show_archive(thd,info);
+        /*
+          Uncomment when mysql_show_archive() is implemented
+
+          res= mysql_show_archive(thd,info);
+         */
       }
       else
       {
         info.report_error(backup::log_level::INFO,ER_BACKUP_RESTORE_START);
 
-        // TODO: freeze all DDL operations
+        // TODO: freeze all DDL operations here
 
         info.save_errors();
         info.restore_all_dbs();
 
-        if ((res= check_info(thd,info)))
+        if (check_info(thd,info))
+        {
+          res= -3;
           goto finish_restore;
+        }
 
         info.clear_saved_errors();
 
-        if ((res= mysql_restore(thd,info,*stream)))
+        if (mysql_restore(thd,info,*stream))
         {
+          res= -4;
           report_errors(thd,ER_BACKUP_RESTORE,info);
         }
         else
@@ -135,7 +145,7 @@ execute_backup_command(THD *thd, LEX *le
           send_summary(thd,info);
         }
 
-        // TODO: unfreeze DDL
+        // TODO: unfreeze DDL here
       }
     } // if (!stream)
 
@@ -153,20 +163,23 @@ execute_backup_command(THD *thd, LEX *le
 
     if (!stream)
     {
-      res= ER_BACKUP_WRITE;
-      my_error(res,MYF(0),loc->describe());
+      res= -5;
+      my_error(ER_BACKUP_WRITE_LOC,MYF(0),loc->describe());
       goto finish_backup;
     }
     else
     {
       backup::Backup_info info(thd);
 
-      if ((res= check_info(thd,info)))
+      if (check_info(thd,info))
+      {
+        res= -6;
         goto finish_backup;
+      }
 
       info.report_error(backup::log_level::INFO,ER_BACKUP_BACKUP_START);
 
-      // TODO: freeze all DDL operations
+      // TODO: freeze all DDL operations here
 
       info.save_errors();
 
@@ -181,18 +194,25 @@ execute_backup_command(THD *thd, LEX *le
         info.add_dbs(lex->db_list); // backup databases specified by user
       }
 
-      if ((res= check_info(thd,info)))
+      if (check_info(thd,info))
+      {
+        res= -7;
         goto finish_backup;
+      }
 
       info.close();
 
-      if ((res= check_info(thd,info)))
+      if (check_info(thd,info))
+      {
+        res= -8;
         goto finish_backup;
+      }
 
       info.clear_saved_errors();
 
-      if ((res= mysql_backup(thd,info,*stream)))
+      if (mysql_backup(thd,info,*stream))
       {
+        res= -9;
         report_errors(thd,ER_BACKUP_BACKUP,info);
       }
       else
@@ -201,7 +221,7 @@ execute_backup_command(THD *thd, LEX *le
         send_summary(thd,info);
       }
 
-      // TODO: unfreeze DDL
+      // TODO: unfreeze DDL here
     } // if (!stream)
 
    finish_backup:
@@ -222,8 +242,10 @@ execute_backup_command(THD *thd, LEX *le
   }
 
    default:
-     // execute_backup_command() should be called with correct command id
-     // from the parser. If not, we fail on this assertion.
+     /*
+       execute_backup_command() should be called with correct command id
+       from the parser. If not, we fail on this assertion.
+      */
      DBUG_ASSERT(FALSE);
   }
 
@@ -231,13 +253,13 @@ execute_backup_command(THD *thd, LEX *le
   DBUG_RETURN(res);
 }
 
+
 /*************************************************
  *
  *                 BACKUP
  *
  *************************************************/
 
-
 // Declarations for functions used in backup operation
 
 namespace backup {
@@ -271,20 +293,29 @@ int mysql_backup(THD *thd,
 
   DBUG_PRINT("backup",("Writing image header"));
 
-  if ((res= info.save(s)))
+  if (info.save(s))
+  {
+    res= -1;
     goto finish;
+  }
 
   DBUG_PRINT("backup",("Writing meta-data"));
 
-  if ((res= backup::write_meta_data(thd,info,s)))
-   goto finish;
+  if (backup::write_meta_data(thd,info,s))
+  {
+    res= -2;
+    goto finish;
+  }
 
   DBUG_PRINT("backup",("Writing table data"));
 
   BACKUP_SYNC("backup_data");
 
-  if ((res= backup::write_table_data(thd,info,s)))
+  if (backup::write_table_data(thd,info,s))
+  {
+    res= -3;
     goto finish;
+  }
 
   DBUG_PRINT("backup",("Backup done."));
   BACKUP_SYNC("backup_done");
@@ -294,14 +325,13 @@ int mysql_backup(THD *thd,
  finish:
 
   DBUG_RETURN(res);
-
 }
 
 /*
    Logic for selecting image format for a given table location:
 
    1. If tables from that location have been already stored in one of the
-      sub-images then choose that subimage.
+      sub-images then choose that sub-image.
 
    2. If location has "native" backup format, put it in a new sub-image with
       that format.
@@ -329,6 +359,13 @@ class Backup_info::Table_ref:
   { return m_hton; }
 };
 
+/**
+  Find image to which given table can be added and add it.
+
+  Creates new images as needed.
+
+  @returns Position of the image found in @c images[] table or -1 on error.
+ */
 int Backup_info::find_image(const Backup_info::Table_ref &tbl)
 {
    DBUG_ENTER("Backup_info::find_image");
@@ -416,8 +453,8 @@ int Backup_info::find_image(const Backup
    if (img->accept(tbl,hton))
     DBUG_RETURN(default_image_no);
 
-   report_error("No backup driver found for table %s.%s",
-                tbl.db().name().ptr(), tbl.name().ptr());
+   Table_ref::describe_buf buf;
+   report_error(ER_BACKUP_NO_BACKUP_DRIVER,tbl.describe(buf,sizeof(buf)));
    DBUG_RETURN(-1);
 }
 
@@ -436,7 +473,7 @@ namespace backup {
 TABLE* get_schema_table(THD *thd, ST_SCHEMA_TABLE *st);
 
 /**
-  Create @c Backup_info structure and prepare it for populationg with meta-data
+  Create @c Backup_info structure and prepare it for populating with meta-data
   items.
 
   When adding a complete database to the archive, all its tables are added.
@@ -451,7 +488,7 @@ Backup_info::Backup_info(THD *thd):
   i_s_tables= get_schema_table(m_thd, ::get_schema_table(SCH_TABLES));
   if (!i_s_tables)
   {
-    report_error("Can't acces server's table information");
+    report_error(ER_BACKUP_LIST_TABLES);
     m_state= ERROR;
   }
 }
@@ -499,7 +536,7 @@ int Backup_info::save(OStream &s)
 {
   if (OK != Archive_info::save(s))
   {
-    report_error("Error writing backup archive header");
+    report_error(ER_BACKUP_WRITE_HEADER);
     return -1;
   }
 
@@ -535,7 +572,7 @@ class Backup_info::Db_ref: public backup
 int Backup_info::add_dbs(List<LEX_STRING> &dbs)
 {
   List_iterator<LEX_STRING> it(dbs);
-  int error= 0;
+  int res= 0;
   LEX_STRING *s;
   String unknown_dbs; // comma separated list of databases which don't exist
 
@@ -546,39 +583,42 @@ int Backup_info::add_dbs(List<LEX_STRING
     if (db_exists(db))
     {
       // In case of error, we just compose unknown_dbs list
-      if (error)
+      if (res)
         continue;
 
       Db_item *it= add_db(db);
 
       if (!it)
       {
-        error= ER_BACKUP_BACKUP_PREPARE;
+        res= -1;
         goto finish;
       }
 
-      if ((error= add_db_items(*it)))
+      if (add_db_items(*it))
+      {
+        res= -2;
         goto finish;
+      }
     }
     else
     {
-      if (error)
+      if (res)
         unknown_dbs.append(",");
       else
-        error= ER_BAD_DB_ERROR;
+        res= -3;
       unknown_dbs.append(db.name());
     }
   }
 
-  if (error==ER_BAD_DB_ERROR)
+  if (res == -3)
     report_error(ER_BAD_DB_ERROR,unknown_dbs.c_ptr());
 
  finish:
 
-  if (error)
+  if (res)
     m_state= ERROR;
 
-  return error;
+  return res;
 }
 
 static const LEX_STRING is_string = { C_STRING_WITH_LEN("information_schema") };
@@ -590,15 +630,13 @@ static const LEX_STRING mysql_string = {
 
 int Backup_info::add_all_dbs()
 {
-  int error= 0;
-
   DBUG_PRINT("backup", ("Reading databases from I_S"));
 
   ::TABLE *db_table = get_schema_table(m_thd, ::get_schema_table(SCH_SCHEMATA));
 
   if (!db_table)
   {
-    report_error("Can't access list of databases");
+    report_error(ER_BACKUP_LIST_DBS);
     return -1;
   }
 
@@ -608,11 +646,12 @@ int Backup_info::add_all_dbs()
 
   const Db_ref is_db(is_string);
   const Db_ref mysql_db(mysql_string);
+  int res= 0;
 
   if (ha->ha_rnd_init(TRUE))
   {
-    error= -2;
-    report_error("Can't read list of databases");
+    res= -2;
+    report_error(ER_BACKUP_LIST_DBS);
     goto finish;
   }
 
@@ -632,12 +671,15 @@ int Backup_info::add_all_dbs()
 
     if (!it)
     {
-      error= -3;
+      res= -3;
       goto finish;
     }
 
-    if ((error= add_db_items(*it)))
+    if (add_db_items(*it))
+    {
+      res= -4;
       goto finish;
+    }
   }
 
   DBUG_PRINT("backup", ("No more databases in I_S"));
@@ -649,10 +691,10 @@ int Backup_info::add_all_dbs()
   if (ha)
     ha->close();
 
-  if (error)
+  if (res)
     m_state= ERROR;
 
-  return error;
+  return res;
 }
 
 /**
@@ -709,32 +751,34 @@ int Backup_info::add_db_items(Db_item &d
   DBUG_ASSERT(is_valid());       // should be valid
   DBUG_ASSERT(i_s_tables->file); // i_s_tables should be opened
 
-  // TODO: add other items (views, stored routines,...)
-
   // add all tables from db.
 
   ::handler *ha= i_s_tables->file;
 
-  // TODO: do it properly: use SELECT conditions
-
+  /*
+    If error debugging is switched on (see debug.h) then I_S.TABLES access
+    error will be triggered when backing up database whose name starts with 'a'.
+   */
   TEST_ERROR_IF(db.name().ptr()[0]=='a');
 
   if (ha->ha_rnd_init(TRUE) || TEST_ERROR)
   {
-    report_error("Can't acces table list when collecting meta-data items"
-                 " of database '%s'",db.name().ptr());
+    report_error(ER_BACKUP_LIST_DB_TABLES,db.name().ptr());
     return -1;
   }
 
+  int res= 0;
+
   while (!ha->rnd_next(i_s_tables->record[0]))
   {
     const Backup_info::Table_ref t(i_s_tables);
 
     if (!t.hton())
     {
-      report_error("Can't locate storage engine of table %s.%s",
-                   t.db().name().ptr(), t.name().ptr());
-      return -2;
+      Table_ref::describe_buf buf;
+      report_error(ER_NO_STORAGE_ENGINE,t.describe(buf,sizeof(buf)));
+      res= -2;
+      break;
     }
 
     if (t.db() == db)
@@ -745,17 +789,22 @@ int Backup_info::add_db_items(Db_item &d
       // add_table method selects/creates sub-image appropriate for storing given table
       Table_item *ti= add_table(db,t);
       if (!ti)
-        return -3;
+      {
+        res= -3;
+        break;
+      }
 
-      int res= add_table_items(*ti);
-      if (res)
-        return res;
+      if (add_table_items(*ti))
+      {
+        res= -4;
+        break;
+      }
     }
   }
 
   ha->ha_rnd_end();
 
-  return 0;
+  return res;
 }
 
 /// Gets table identity from a row of I_S.TABLES table
@@ -785,24 +834,35 @@ Backup_info::Table_item*
 Backup_info::add_table(Db_item &db,
                        const Table_ref &t)
 {
-  int no= find_image(t); // Consider: reporting errors inside find image...
+  int no= find_image(t); // Note: find_image reports errors
 
+  /*
+    If error debugging is switched on (see debug.h) then any table whose
+    name starts with 'a' will trigger "no backup driver" error.
+   */
   TEST_ERROR_IF(t.name().ptr()[0]=='a');
 
-  if (no < 0 || !images[no] || TEST_ERROR)
-  {
-    report_error("Can't find backup driver for table '%s.%s'",
-                  t.db().name().ptr(),t.name().ptr());
+  if (no < 0 || TEST_ERROR)
     return NULL;
-  }
 
+  /*
+    If image was found then images[no] should point at the Image_info
+    object
+   */
+  Image_info *img= images[no];
+  DBUG_ASSERT(img);
+
+  /*
+    If error debugging is switched on (see debug.h) then any table whose
+    name starts with 'b' will trigger error when added to backup image.
+   */
   TEST_ERROR_IF(t.name().ptr()[0]=='b');
 
-  int tno= images[no]->tables.add(t);
+  int tno= img->tables.add(t);
   if (tno < 0 || TEST_ERROR)
   {
-    report_error("Table '%s.%s' can't be added to %s's image",
-                 t.db().name().ptr(),t.name().ptr(),images[no]->name());
+    Table_ref::describe_buf buf;
+    report_error(ER_BACKUP_NOT_ACCEPTED,img->name(),t.describe(buf,sizeof(buf)));
     return NULL;
   }
 
@@ -833,12 +893,13 @@ Backup_info::add_table(Db_item &db,
  */
 int Backup_info::add_table_items(Table_item&)
 {
-  // FIXME: complete this
+  // TODO: Implement when we handle per-table meta-data.
   return 0;
 }
 
 } // backup namespcae
 
+
 /*************************************************
  *
  *                 RESTORE
@@ -849,38 +910,42 @@ int Backup_info::add_table_items(Table_i
 
 namespace backup {
 
+// defined in meta_backup.cc
 bool restore_meta_data(THD*, Restore_info&, IStream&);
+
+// defined in data_backup.cc
 bool restore_table_data(THD*, Restore_info&, IStream&);
 
 } // backup namespace
 
 
-// pre: archive info has been already read and the stream is positioned
-//      at archive meta-data
+/**
+  Restore items saved in backup archive.
 
+  @pre archive info has been already read and the stream is positioned
+        at archive's meta-data
+*/
 int mysql_restore(THD *thd, backup::Restore_info &info, backup::IStream &s)
 {
   DBUG_ENTER("mysql_restore");
 
-  int error= 0;
   size_t start_bytes= s.bytes;
 
   DBUG_PRINT("restore",("Restoring meta-data"));
 
-  if ((error= backup::restore_meta_data(thd, info, s)))
-    DBUG_RETURN(error);
+  if (backup::restore_meta_data(thd, info, s))
+    DBUG_RETURN(-1);
 
   DBUG_PRINT("restore",("Restoring table data"));
 
-  if ((error= backup::restore_table_data(thd,info,s)))
-    DBUG_RETURN(error);
+  if (backup::restore_table_data(thd,info,s))
+    DBUG_RETURN(-2);
 
   DBUG_PRINT("restore",("Done."));
 
   info.total_size= s.bytes - start_bytes;
 
   DBUG_RETURN(0);
-
 }
 
 /*************************************************
@@ -899,7 +964,7 @@ struct File_loc: public Location
   { return SERVER_FILE; }
 
   File_loc(const char *p)
-  { path.append(p); }     // files_charset_info
+  { path.append(p); }
 
   const char* describe()
   { return path.c_ptr(); }
@@ -964,7 +1029,6 @@ Location::find(const LEX_STRING &where)
   return new File_loc(where.str);
 }
 
-
 } // backup namespace
 
 
@@ -1067,32 +1131,35 @@ bool send_summary(THD *thd, const Archiv
     Loop through the catalog and list the contents by
     database.
 
-  List<schema_ref> cat1= *catalog;
-  List_iterator<schema_ref> cat(cat1);
-  schema_ref *tmp;
-
-  for (Archive_info::Db_iterator it(info); it; ++it)
-  while ((tmp= cat++))
-  {
-    uint howmuch= 0;
-
-    for (Archive_info::DItem_iterator dit(it); dit; ++dit)
-      howmuch++;
-
-    my_snprintf(buf,sizeof(buf),"%s %3u %s in database %s.",
-                backup? "Backed up" : "Restored",
-                howmuch, howmuch != 1 ? "tables" : " table",
-                it->name().c_ptr());
+    Enable this code when functions for browsing archive contents are
+    implemented.
+
+    List<schema_ref> cat1= *catalog;
+    List_iterator<schema_ref> cat(cat1);
+    schema_ref *tmp;
+
+    for (Archive_info::Db_iterator it(info); it; ++it)
+    while ((tmp= cat++))
+    {
+      uint howmuch= 0;
+
+      for (Archive_info::DItem_iterator dit(it); dit; ++dit)
+        howmuch++;
+
+      my_snprintf(buf,sizeof(buf),"%s %3u %s in database %s.",
+                  backup? "Backed up" : "Restored",
+                  howmuch, howmuch != 1 ? "tables" : " table",
+                  it->name().c_ptr());
+
+      protocol->prepare_for_resend();
+      protocol->store(buf,system_charset_info);
+      protocol->write();
+    }
 
+    my_snprintf(buf,sizeof(buf)," ");
     protocol->prepare_for_resend();
     protocol->store(buf,system_charset_info);
     protocol->write();
-  }
-
-  my_snprintf(buf,sizeof(buf)," ");
-  protocol->prepare_for_resend();
-  protocol->store(buf,system_charset_info);
-  protocol->write();
   */
 
   my_snprintf(buf,sizeof(buf)," header     = %-8lu bytes",(unsigned long)info.header_size);
@@ -1134,10 +1201,6 @@ bool send_summary(THD *thd, const Restor
 
 
 /// Open given table in @c INFORMATION_SCHEMA database.
-
-// TODO: Use select condition to get only selected rows from a table
-// CHECK: prepare_select_* functions (sql_help.cc)
-
 TABLE* get_schema_table(THD *thd, ST_SCHEMA_TABLE *st)
 {
   TABLE *t;
@@ -1145,7 +1208,7 @@ TABLE* get_schema_table(THD *thd, ST_SCH
 
   bzero( &arg, sizeof(TABLE_LIST) );
 
-  /* set context for create_schema_table call */
+  // set context for create_schema_table call
   arg.schema_table= st;
   arg.alias=        NULL;
   arg.select_lex=   NULL;
@@ -1155,8 +1218,8 @@ TABLE* get_schema_table(THD *thd, ST_SCH
   if( !t ) return NULL; // error!
 
   /*
-  Temporarily set thd->lex->wild to NULL to keep st->fill_table
-  happy :)
+   Temporarily set thd->lex->wild to NULL to keep st->fill_table
+   happy :)
   */
 
   LEX local_lex=    *thd->lex;
@@ -1165,16 +1228,19 @@ TABLE* get_schema_table(THD *thd, ST_SCH
   thd->lex=   &local_lex;
 
   local_lex.wild = NULL;
+  // set thd->lex->sql_command to something neutral (see, make_db_list/get_index_file_values).
   local_lex.sql_command = enum_sql_command(0);
 
-  // Note: set thd->lex->sql_command to something neutral (see, make_db_list/get_index_file_values).
-
   /* context for fill_table */
   arg.table= t;
 
+  /*
+    Question: is it correct to fill I_S table each time we use it or should it
+    be filled only once?
+   */
   st->fill_table(thd,&arg,NULL);  // NULL = no select condition
 
-  /* restore wild field */
+  // restore wild field
   thd->lex= saved_lex;
 
   return t;
@@ -1182,10 +1248,11 @@ TABLE* get_schema_table(THD *thd, ST_SCH
 
 /// Build linked @c TABLE_LIST list from a list stored in @c Table_list object.
 
-// FIXME: build list with the same order as in input
-// Actually, should work fine with reversed list as long as we use the reversed
-// list both in table writing and reading.
-
+/*
+  FIXME: build list with the same order as in input
+  Actually, should work fine with reversed list as long as we use the reversed
+  list both in table writing and reading.
+ */
 TABLE_LIST *build_table_list(const Table_list &tables, thr_lock_type lock)
 {
   TABLE_LIST *tl= NULL;
===== stream.cc 1.8 vs edited =====
--- 1.8/sql/backup/stream.cc	2007-06-23 22:56:36 +02:00
+++ edited/stream.cc	2007-06-23 21:58:24 +02:00
@@ -2,9 +2,17 @@
 
 #include "stream.h"
 
+/*
+  TODO
+
+  - blocking of OStream output when data window is allocated.
+  - use my_read instead of read - need to know how to detect EOF.
+
+ */
 
 namespace backup {
 
+/************** IStream *************************/
 
 Window::Result Window::set_length(const size_t len)
 {
@@ -59,8 +67,6 @@ bool Stream::rewind()
 
 /************** OStream *************************/
 
-// TODO: blocking of other stream operations when window is allocated.
-
 byte* OStream::get_window(const size_t len)
 {
   if (m_blocked || m_end+len > last_byte)
@@ -187,8 +193,7 @@ stream_result::value IStream::next_chunk
 
   size_t len= next_chunk_len+2;
 
-  // TODO: Learn how to read stream with EOF detection using my_read
-  long int howmuch= 0;  // POSIX ssize_t not defined in Win :|
+  long int howmuch= 0;  // POSIX ssize_t not defined on win platform :|
 
   while (len > 0 && (howmuch= ::read(m_fd,m_buf,len)) > 0)
     len-= howmuch;
@@ -219,6 +224,8 @@ stream_result::value IStream::next_chunk
   return howmuch==0 ? stream_result::EOS : stream_result::OK;
 }
 
+#ifdef DBUG_BACKUP
+
 // Show data chunks in a backup stream;
 
 void dump_stream(IStream &s)
@@ -250,6 +257,6 @@ void dump_stream(IStream &s)
    DBUG_PRINT("stream",("== ERROR: %d",(int)res));
 }
 
-} // backup namespace
-
+#endif
 
+} // backup namespace
===== stream.h 1.10 vs edited =====
--- 1.10/sql/backup/stream.h	2007-06-23 22:56:36 +02:00
+++ edited/stream.h	2007-06-23 22:20:28 +02:00
@@ -2,13 +2,13 @@
 #define _BACKUP_STREAM_H_
 
 #include <backup/api_types.h>    // for Buffer definition
+#include <backup/debug.h>        // for definition of DBUG_BACKUP
 
-/***********************************************************
-
- Generic Stream Intrface for serializing basic types
- (ints, strings etc).
+/**
+  @file
 
- ***********************************************************/
+ Generic Stream Interface for serializing basic types (integers, strings etc).
+ */
 
 namespace util
 {
@@ -23,10 +23,11 @@ struct stream_result
 
 /*
   Stream window: an abstract object which can be used to access a stream
-  of bytes through a window in process address space. The methods give pointer to the
-  beginning of the window and its current size. There is a request for enlarging the
-  window. We can also move the window beginning to the right (decreasing window size).
-  If some bytes move outside the window, they can't be accesed any more.
+  of bytes through a window in process address space. The methods give pointer
+  to the beginning of the window and its current size. There is a request for
+  enlarging the window. We can also move the window beginning to the right
+  (decreasing window size). If some bytes move outside the window, they can't
+  be accessed any more.
 
   ================================================> (stream of bytes)
 
@@ -58,15 +59,15 @@ struct stream_result
    byte*  head() const;       // pointer to start of the window
    byte*  end() const;        // pointer to the end of the window
    size_t length() const;     // current length of the window (in bytes)
-   Result set_length(const size_t size);   // extend the window to have (at least) given length
-   Result move(const off_t offset);     // move window header
+   Result set_length(const size_t size); // extend the window to have (at least) given length
+   Result move(const off_t offset);      // move window header
    //void  init();
    //void  done();
 
    operator int(const Result&); // interpret the result: is it error or other
                                    common situation (end of data).
 
-   Note: window can be used for reading or writting.
+   Note: window can be used for reading or writing.
 */
 
 
@@ -76,7 +77,7 @@ struct stream_result
    These classes provide interface for serializing basic data
    (numbers, strings) using host independent format.
 
-   Interface is parametrized by a class implementing stream window.
+   Interface is parametrised by a class implementing stream window.
    Instance of the window class is used to read/write stream data.
  */
 
@@ -97,7 +98,12 @@ class IStream
   Result read4int(ulong &x);
   Result readint(ulong &x);
   Result readint(uint &x)
-  { ulong y; Result res= readint(y); x= y; return res; };
+  {
+    ulong y;
+    Result res= readint(y);
+    x= y;
+    return res;
+  }
 
   Result readstr(String &s);
 
@@ -122,19 +128,19 @@ class OStream
   Result writeint(const ulong x);
   Result writestr(const String &s);
   Result writestr(const char *s)
-  { return writestr(String(s,table_alias_charset)); };
+  { return writestr(String(s,table_alias_charset)); }
 
   Result writenil();
 };
 
-
 template<class SW>
+inline
 typename IStream<SW>::Result
 IStream<SW>::readbyte(byte &x)
 {
   Result res;
 
-  if( (res= m_win.set_length(1)) != stream_result::OK )
+  if ((res= m_win.set_length(1)) != stream_result::OK)
     return res;
 
   x= *m_win.head();
@@ -143,12 +149,13 @@ IStream<SW>::readbyte(byte &x)
 }
 
 template<class SW>
+inline
 typename OStream<SW>::Result
 OStream<SW>::writebyte(const byte x)
 {
   Result res;
 
-  if( (res= m_win.set_length(1)) != stream_result::OK )
+  if ((res= m_win.set_length(1)) != stream_result::OK)
     return res;
 
   (*m_win.head())= x;
@@ -156,14 +163,14 @@ OStream<SW>::writebyte(const byte x)
   return m_win.move(1);
 }
 
-//inline
 template<class SW>
+inline
 typename IStream<SW>::Result
 IStream<SW>::read2int(uint &x)
 {
   Result res;
 
-  if( (res= m_win.set_length(2)) != stream_result::OK )
+  if ((res= m_win.set_length(2)) != stream_result::OK)
    return res;
 
   x= uint2korr(m_win.head());
@@ -171,17 +178,14 @@ IStream<SW>::read2int(uint &x)
   return m_win.move(2);
 }
 
-//inline
 template<class SW>
+inline
 typename OStream<SW>::Result
 OStream<SW>::write2int(const int x)
 {
   Result res;
 
-  if( x >= (1<<16) )
-    return Result(stream_result::ERROR);
-
-  if( (res= m_win.set_length(2)) != stream_result::OK )
+  if ((res= m_win.set_length(2)) != stream_result::OK)
     return res;
 
   int2store(m_win.head(),x);
@@ -190,14 +194,14 @@ OStream<SW>::write2int(const int x)
 }
 
 
-//inline
 template<class SW>
+inline
 typename IStream<SW>::Result
 IStream<SW>::read4int(ulong &x)
 {
   Result res;
 
-  if( (res= m_win.set_length(4)) != stream_result::OK )
+  if ((res= m_win.set_length(4)) != stream_result::OK)
    return res;
 
   x= uint4korr(m_win.head());
@@ -205,17 +209,14 @@ IStream<SW>::read4int(ulong &x)
   return m_win.move(4);
 }
 
-//inline
 template<class SW>
+inline
 typename OStream<SW>::Result
 OStream<SW>::write4int(const ulong x)
 {
   Result res;
 
-//  if( x >= (1<<32) )
-//    return Result(stream_result::ERROR);
-
-  if( (res= m_win.set_length(4)) != stream_result::OK )
+  if ((res= m_win.set_length(4)) != stream_result::OK)
     return res;
 
   int4store(m_win.head(),x);
@@ -224,15 +225,16 @@ OStream<SW>::write4int(const ulong x)
 }
 
 
-// write/read numnber using variable-length encoding
+// write/read number using variable-length encoding
 
 template<class SW>
+inline
 typename IStream<SW>::Result
 IStream<SW>::readint(ulong &x)
 {
   Result res;
 
-  if( (res= m_win.set_length(1)) != stream_result::OK )
+  if ((res= m_win.set_length(1)) != stream_result::OK)
     return res;
 
   x= *m_win.head();
@@ -253,26 +255,27 @@ IStream<SW>::readint(ulong &x)
 
   default:
     return Result(stream_result::OK);
-  };
+  }
 }
 
 template<class SW>
+inline
 typename OStream<SW>::Result
 OStream<SW>::writeint(const ulong x)
 {
   Result res;
 
-  if( (res= m_win.set_length(1)) != stream_result::OK )
+  if ((res= m_win.set_length(1)) != stream_result::OK)
     return res;
 
-  if( x < 251 )
+  if (x < 251)
     return writebyte((byte)x);
 
-  if( x < (1<<16) )
+  if (x < (1UL<<16))
   {
     res= writebyte(252);
     return res == stream_result::OK ? write2int(x) : res;
-  };
+  }
 
   res= writebyte(253);
   return res == stream_result::OK ? write4int(x) : res;
@@ -282,37 +285,37 @@ OStream<SW>::writeint(const ulong x)
 // Write/read string using "length coded string" format
 
 template<class SW>
+inline
 typename IStream<SW>::Result
 IStream<SW>::readstr(String &s)
 {
   Result  res;
   uint    len;
 
-  if( (res= readint(len)) != stream_result::OK )
+  if ((res= readint(len)) != stream_result::OK)
     return res;
 
-  if( (res= m_win.set_length(len)) != stream_result::OK )
+  if ((res= m_win.set_length(len)) != stream_result::OK)
     return res;
 
   s.free();
-  s.copy(reinterpret_cast<const char*>(m_win.head()),
-         len,
-         table_alias_charset);
+  s.copy((const char*)m_win.head(), len, &::my_charset_bin);
 
   return m_win.move(len);
 }
 
 template<class SW>
+inline
 typename OStream<SW>::Result
 OStream<SW>::writestr(const String &s)
 {
   Result  res;
   uint    len= s.length();
 
-  if( (res= writeint(len)) != stream_result::OK )
+  if ((res= writeint(len)) != stream_result::OK)
     return res;
 
-  if( (res= m_win.set_length(len)) != stream_result::OK )
+  if ((res= m_win.set_length(len)) != stream_result::OK)
     return res;
 
   memcpy(m_win.head(), s.ptr(), len);
@@ -321,6 +324,7 @@ OStream<SW>::writestr(const String &s)
 }
 
 template<class SW>
+inline
 typename OStream<SW>::Result
 OStream<SW>::writenil()
 {
@@ -336,9 +340,9 @@ OStream<SW>::writenil()
   Backup Stream Interface
 
   The stream is organized as a sequence of chunks each of which
-  can have different length. When stream is read chunk boundraries
+  can have different length. When stream is read chunk boundaries
   are detected. If this happens, next_chunk() member must be called
-  in order to access data in next chunk. When writting to a stream,
+  in order to access data in next chunk. When writing to a stream,
   data is appended to the current chunk. End_chunk() member closes
   the current chunk and starts a new one.
 
@@ -474,12 +478,12 @@ class Stream
 
   int     m_fd;
   String  m_path;
-  int     m_flags;  ///< flags used when openning the file
+  int     m_flags;  ///< flags used when opening the file
 
 };
 
 /**
-  Implement backup stream which writes data to a file.
+  Implements backup stream which writes data to a file.
 
   This class inherits from util::OStream which defines methods for serialization of
   basic datatypes (strings and integer). It also implements the concept of data chunks. Data
@@ -488,7 +492,7 @@ class Stream
 
   A client of this class can ask an instance for an output buffer with <code>get_window()</code>
   method. After filling the buffer its contents can be written to the stream. This is to avoid
-  double buffering and unneccessary copying of data. However, once an output buffer is allocated,
+  double buffering and unnecessary copying of data. However, once an output buffer is allocated,
   all output to the stream is blocked until the buffer is written with <code>write_window()</code>
   or dropped with <code>drop_window()</code>.
  */
@@ -502,10 +506,10 @@ class Stream
 
   OStream instance uses an output buffer of fixed size inherited from Window class. The size of
   a chunk is limited by the size of this buffer as a whole chunk is stored inside the buffer
-  before writting to the file.
+  before writing to the file.
 
-  Writting to the file happens when the current chunk is closed with <code>end_chunk()</code>
-  method. At the time of writting the output, buffer contents is as follows:
+  writing to the file happens when the current chunk is closed with <code>end_chunk()</code>
+  method. At the time of writing the output, buffer contents is as follows:
 
   ====================== <- m_buf
   2 bytes for chunk size
@@ -580,12 +584,12 @@ class OStream:
 
 
 /**
-  Implement backup stream reading data from a file.
+  Implements backup stream reading data from a file.
 
   This class inherits from util::IStream which defines methods for serialization of
   basic datatypes (strings and integer). It also handles chunk boundaries as created by
   the OStream class. When reading data at the end of a data chunk, <code>stream_result::EOC</code>
-  is returned. To access data in next chunck, <code>next_chunk()</code> must be called.
+  is returned. To access data in next chunk, <code>next_chunk()</code> must be called.
  */
 
 /*
@@ -689,26 +693,13 @@ class IStream:
   friend class stream_instances;
 };
 
+#ifdef DBUG_BACKUP
+
 // Function for debugging backup stream implementation.
 void dump_stream(IStream &);
 
+#endif
 
 } // backup namespace
-
-#define TEST_STR_RES(H,R) \
-  do { \
-    if ( (R) != stream_result::OK ) \
-      DBUG_PRINT(H,("stream op result= %d",(R))); \
-    DBUG_ASSERT( (R) != stream_result::ERROR ); \
-  } while(0)
-#define TEST_STR_OK(H,R,X) \
-    if ( (R) != stream_result::OK ) \
-    { \
-      DBUG_PRINT(H,("stream op result= %d",(R))); \
-      return X; \
-    }
-#define TEST_WR_RES(R,X)  TEST_STR_OK("backup",(R),(X))
-#define TEST_RD_IRES(R)  TEST_STR_OK("restore",(R))
-#define TEST_RD_RES(R,X)  TEST_STR_OK("restore",(R),(X))
 
 #endif /*BACKUP_STREAM_H_*/
===== string_pool.cc 1.4 vs edited =====
--- 1.4/sql/backup/string_pool.cc	2007-06-23 22:56:36 +02:00
+++ edited/string_pool.cc	2007-06-23 21:58:24 +02:00
@@ -27,9 +27,9 @@ namespace backup {
 
   0: "foo"
   1: -
-  3: -
-  4: "bar"
-  5: "baz"
+  2: -
+  3: "bar"
+  4: "baz"
   ...
 
   is saved as
@@ -39,7 +39,7 @@ namespace backup {
   where the (NIL:2) entry describes a "hole" in the pool of width 2.
 
   Empty pool is saved as | NIL : 0 |.
- */
+*/
 
 result_t StringPool::save(OStream &str)
 {
@@ -65,7 +65,7 @@ result_t StringPool::save(OStream &str)
       {
         /*
           If skip > 0 this entry closes a hole of skip entries.
-          Write hole with to finish its description.
+          Write hole width to finish its description.
          */
         if (skip > 0)
         {
@@ -80,7 +80,7 @@ result_t StringPool::save(OStream &str)
         if (i >= count())
           break;
 
-        skip= 0; // the hole is closed now
+        skip= 0; // the hole has ended now
 
         DBUG_PRINT("string_pool",("writing entry %s at position %d",
                                   (entries[i].el)->c_ptr(),i));
@@ -134,7 +134,7 @@ result_t StringPool::read(IStream &str)
 
   if (res != stream_result::OK && res != stream_result::NIL)
   {
-    // In that case thee is an error or we hit end of data
+    // In that case there is an error or we hit end of data
     if (res != stream_result::ERROR)
       DBUG_PRINT("string_pool",("End of data hit"));
     DBUG_RETURN((result_t)res);
@@ -157,8 +157,9 @@ result_t StringPool::read(IStream &str)
 
     DBUG_PRINT("sting_pool",("Starts with hole of width %d",skip));
 
-    /* this is non-empty pool starting with a hole - read the following
-       string
+    /*
+      this is non-empty pool starting with a hole - read the following
+      string
      */
 
     res= str.readstr(buf);
===== string_pool.h 1.3 vs edited =====
--- 1.3/sql/backup/string_pool.h	2007-06-23 22:56:36 +02:00
+++ edited/string_pool.h	2007-06-23 21:58:24 +02:00
@@ -4,14 +4,24 @@
 #include "map.h"
 #include "stream.h"
 
-namespace util {
+/**
+  @file
 
-/************************************************************
+  Implementation of a string pool which is a collection of
+  String objects indexed by 8 bit keys.
+ */
+
+/*
+  TODO
+
+  - change memory allocation scheme
+  - use mysql library functions
+  - simplify by not using Map template
+ */
 
-   StringPool class
-   String pool = map from key8 -> Strings
+namespace util {
 
- ************************************************************/
+/// A domain of String values which can by used by Map template.
 
 struct StringDom
 {
@@ -35,10 +45,9 @@ typedef Map<StringDom>  StringPool;
 
 namespace backup {
 
-// Add serialization to util::StringPool
-
-// FIXME: what happens when saving/reading empty pool?
-
+/**
+  Adds serialization to util::StringPool class
+ */
 class StringPool: public util::StringPool
 {
  public:
===== ../share/errmsg.txt 1.142 vs edited =====
--- 1.142/sql/share/errmsg.txt	2007-06-23 22:58:24 +02:00
+++ edited/../share/errmsg.txt	2007-06-23 22:26:44 +02:00
@@ -6055,6 +6055,19 @@ ER_DUP_ENTRY_WITH_KEY_NAME 23000 S1009
 ER_BINLOG_PURGE_EMFILE
         eng "Too many files opened, please execute the command again"
         ger "Zu viele offene Dateien, bitte fER_NO_STORAGE_ENGINE
+        eng "Can't access storage engine of table %-.64s"
+
+ER_BACKUP_BACKUP_START
+        eng "Starting backup process"
+ER_BACKUP_BACKUP_DONE
+        eng "Backup completed"
+ER_BACKUP_RESTORE_START
+        eng "Starting restore process"
+ER_BACKUP_RESTORE_DONE
+        eng "Restore completed"
+
 ER_BACKUP_BACKUP
         eng "Error during backup operation - server's error log contains more information about the error"
 ER_BACKUP_RESTORE
@@ -6062,18 +6075,69 @@ ER_BACKUP_RESTORE
 ER_BACKUP_BACKUP_PREPARE
         eng "Error when preparing for backup operation"
 ER_BACKUP_RESTORE_PREPARE
-        eng "Error when preparing for restore operation"				
+        eng "Error when preparing for restore operation"
 ER_BACKUP_INVALID_LOC
         eng "Invalid backup location '%-.64s'"
-ER_BACKUP_READ
+ER_BACKUP_READ_LOC
         eng "Can't read backup location '%-.64s'"
-ER_BACKUP_WRITE
+ER_BACKUP_WRITE_LOC
         eng "Can't write to backup location '%-.64s'"
-ER_BACKUP_BACKUP_START
-        eng "Starting backup process"
-ER_BACKUP_BACKUP_DONE
-        eng "Backup completed"
-ER_BACKUP_RESTORE_START
-        eng "Starting restore process"
-ER_BACKUP_RESTORE_DONE
-        eng "Restore completed"
+ER_BACKUP_LIST_DBS
+        eng "Can't enumerate server databases"
+ER_BACKUP_LIST_TABLES
+        eng "Can't enumerate server tables"
+ER_BACKUP_LIST_DB_TABLES
+        eng "Can't enumerate tables in database %-.64s"
+ER_BACKUP_READ_HEADER
+        eng "Can't read backup archive preamble"
+ER_BACKUP_WRITE_HEADER
+        eng "Can't write backup archive preamble"
+ER_BACKUP_NO_BACKUP_DRIVER
+        eng "Can't find backup driver for table %-.64s"
+ER_BACKUP_NOT_ACCEPTED
+        eng "%-.64s backup driver was selected for table %-.64s but it rejects to handle this table"
+ER_BACKUP_CREATE_BACKUP_DRIVER
+        eng "Can't create %-.64s backup driver"
+ER_BACKUP_CREATE_RESTORE_DRIVER
+        eng "Can't create %-.64s restore driver"
+ER_BACKUP_TOO_MANY_IMAGES
+        eng "Found %d images in backup archive but maximum %d are supported"
+ER_BACKUP_WRITE_META
+        eng "Error when saving meta-data of %-.64s"
+ER_BACKUP_READ_META
+        eng "Error when reading meta-data list"
+ER_BACKUP_CREATE_META
+        eng "Can't create %-.64s"
+ER_BACKUP_GET_BUF
+        eng "Can't allocate buffer for image data transfer"
+ER_BACKUP_WRITE_DATA
+        eng "Error when writing %-.64s backup image data (for table #%d)"
+ER_BACKUP_READ_DATA
+        eng "Error when reading data from backup stream"
+ER_BACKUP_NEXT_CHUNK
+        eng "Can't go to the next chunk in backup stream"
+
+ER_BACKUP_INIT_BACKUP_DRIVER
+        eng "Can't initialize %-.64s backup driver"
+ER_BACKUP_INIT_RESTORE_DRIVER
+        eng "Can't initialize %-.64s restore driver"
+ER_BACKUP_STOP_BACKUP_DRIVER
+        eng "Can't shut down %-.64s backup driver"
+ER_BACKUP_STOP_RESTORE_DRIVERS
+        eng "Can't shut down %-.64s backup driver(s)"
+ER_BACKUP_PREPARE_DRIVER
+        eng "%-.64s backup driver can't prepare for synchronization"
+ER_BACKUP_CREATE_VP
+        eng	"%-.64s backup driver can't create its image validity point"
+ER_BACKUP_UNLOCK_DRIVER
+        eng "Can't unlock %-.64s backup driver after creating the validity point"
+ER_BACKUP_CANCEL_BACKUP
+        eng "%-.64s backup driver can't cancel its backup operation"
+ER_BACKUP_CANCEL_RESTORE
+        eng "%-.64s restore driver can't cancel its restore operation"
+ER_BACKUP_GET_DATA
+        eng "Error when polling %-.64s backup driver for its image data"
+ER_BACKUP_SEND_DATA
+        eng "Error when sending image data (for table #%d) to %-.64s restore driver"
+ER_BACKUP_SEND_DATA_RETRY
+        eng "After %d attempts %-.64s restore driver still can't accept next block of data"
Thread
bk commit into 5.1 tree (rafal:1.2537)rsomla20 Jun
  • RE: bk commit into 5.1 tree (rafal:1.2537)Chuck Bell21 Jun
    • Re: bk commit into 5.1 tree (rafal:1.2537)Rafal Somla23 Jun
      • RE: bk commit into 5.1 tree (rafal:1.2537)Chuck Bell25 Jun