MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:oystein.grovlen Date:September 1 2008 12:54pm
Subject:bzr commit into mysql-6.0-backup branch (oystein.grovlen:2688) Bug#39089
WL#4384
View as plain text  
#At file:///home/oysteing/mysql/mysql-6.0-backup-1/

 2688 oystein.grovlen@stripped	2008-09-01
      BUG#39089/WL#4384 Fix missing error handling in backup code.
      Make sure errors returned from functions are reports are handled and/or logged.
modified:
  sql/backup/backup_aux.h
  sql/backup/backup_info.cc
  sql/backup/backup_kernel.h
  sql/backup/data_backup.cc
  sql/backup/image_info.cc
  sql/backup/image_info.h
  sql/backup/kernel.cc
  sql/backup/stream.cc
  sql/backup/stream.h
  sql/mdl.cc
  sql/share/errmsg.txt

=== modified file 'sql/backup/backup_aux.h'
--- a/sql/backup/backup_aux.h	2008-08-21 19:28:49 +0000
+++ b/sql/backup/backup_aux.h	2008-09-01 12:53:37 +0000
@@ -6,11 +6,6 @@
  
   @brief Auxiliary declarations used in online backup code.
 
-  @todo Fix error detection in places marked with "FIXME: detect errors...". 
-  These are places where functions or methods are called and if they can 
-  report errors it should be detected and appropriate action taken. If callee 
-  never reports errors or we want to ignore errors, a comment explaining this
-  should be added.
 */ 
 
 typedef st_plugin_int* storage_engine_ref;
@@ -140,8 +135,8 @@ class String: public ::String
 };
 
 inline
-void set_table_list(TABLE_LIST &tl, const Table_ref &tbl, 
-                    thr_lock_type lock_type, MEM_ROOT *mem)
+int set_table_list(TABLE_LIST &tl, const Table_ref &tbl,
+                   thr_lock_type lock_type, MEM_ROOT *mem)
 {
   DBUG_ASSERT(mem);
 
@@ -149,8 +144,12 @@ void set_table_list(TABLE_LIST &tl, cons
   tl.db= const_cast<char*>(tbl.db().name().ptr());
   tl.lock_type= lock_type;
 
-  // FIXME: detect errors (if NULL returned).
   tl.mdl_lock_data= mdl_alloc_lock(0, tl.db, tl.table_name, mem); 
+  if (!tl.mdl_lock_data)                    // Failed to allocate lock
+  {
+    return 1;
+  }
+  return 0;
 }
 
 inline
@@ -165,7 +164,10 @@ TABLE_LIST* mk_table_list(const Table_re
      return NULL;
 
   bzero(ptr, sizeof(TABLE_LIST));
-  set_table_list(*ptr, tbl, lock_type, mem);
+  if (set_table_list(*ptr, tbl, lock_type, mem)) // Failed to allocate lock
+  {
+    return NULL;
+  }
 
   return ptr;
 }

=== modified file 'sql/backup/backup_info.cc'
--- a/sql/backup/backup_info.cc	2008-08-20 13:23:10 +0000
+++ b/sql/backup/backup_info.cc	2008-09-01 12:53:37 +0000
@@ -5,16 +5,6 @@
   implements algorithm for selecting backup engine used to backup
   given table.
   
-  @todo Fix error detection in places marked with "FIXME: detect errors...". 
-  These are places where functions or methods are called and if they can 
-  report errors it should be detected and appropriate action taken. If callee 
-  never reports errors or we want to ignore errors, a comment explaining this
-  should be added.
-
-  @todo Fix error logging in places marked with "FIXME: error logging...". In 
-  these places it should be decided if and how the error should be shown to the
-  user. If an error message should be logged, it can happen either in the place
-  where error was detected or somewhere up the call stack.
 */
 
 #include "../mysql_priv.h"
@@ -317,14 +307,15 @@ Backup_info::Backup_info(Backup_restore_
 
   bzero(m_snap, sizeof(m_snap));
 
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  hash_init(&ts_hash, &::my_charset_bin, 16, 0, 0,
-            Ts_hash_node::get_key, Ts_hash_node::free, MYF(0));
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  hash_init(&dep_hash, &::my_charset_bin, 16, 0, 0,
-            Dep_node::get_key, Dep_node::free, MYF(0));
+  if (hash_init(&ts_hash, &::my_charset_bin, 16, 0, 0,
+                Ts_hash_node::get_key, Ts_hash_node::free, MYF(0))
+      ||
+      hash_init(&dep_hash, &::my_charset_bin, 16, 0, 0,
+                Dep_node::get_key, Dep_node::free, MYF(0)))
+  {
+    // Allocation failed.  Error has been logged.
+    return;
+  }
 
   /* 
     Create nodata, default, and CS snapshot objects and add them to the 
@@ -332,35 +323,51 @@ Backup_info::Backup_info(Backup_restore_
     element on that list, as a "catch all" entry. 
    */
 
-  snap= new Nodata_snapshot(m_ctx);  // reports errors
-
-  // FIXME: error logging (in case snap could not be allocated).
-  if (!snap || !snap->is_valid())
+  snap= new Nodata_snapshot(m_ctx);             // logs errors
+  if (!snap)
+  {
+    m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
     return;
+  }
+
+  if (!snap->is_valid())
+    return;    // Error has been logged by Nodata_snapshot constructor
 
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  snapshots.push_back(snap);
+  if (snapshots.push_back(snap))
+  {
+    return;                                   // Error has been logged
 
-  snap= new CS_snapshot(m_ctx); // reports errors
+  }
 
-  // FIXME: error logging (in case snap could not be allocated).
-  if (!snap || !snap->is_valid())
+  snap= new CS_snapshot(m_ctx);                 // logs errors
+  if (!snap)
+  {
+    m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
     return;
+  }
 
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  snapshots.push_back(snap);
+  if (!snap->is_valid())
+    return;        // Error has been logged by CS_snapshot constructor
 
-  snap= new Default_snapshot(m_ctx);  // reports errors
+  if (snapshots.push_back(snap))
+  {
+    return;                                   // Error has been logged
+  }
 
-  // FIXME: error logging (in case snap could not be allocated).
-  if (!snap || !snap->is_valid())
+  snap= new Default_snapshot(m_ctx);            // logs errors
+  if (!snap)
+  {
+    m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
     return;
+  }
+
+  if (!snap->is_valid())
+    return;  // Error has been logged by Deafault_snapshot constructor
 
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  snapshots.push_back(snap);
+  if (snapshots.push_back(snap))
+  {
+    return;                                   // Error has been logged
+  }
 
   m_state= CREATED;
 }
@@ -1297,7 +1304,7 @@ class Backup_info::Global_iterator
 
  public:
 
-  Global_iterator(const Image_info&);
+  Global_iterator(const Backup_info&);
 
  private:
 
@@ -1306,11 +1313,15 @@ class Backup_info::Global_iterator
 };
 
 inline
-Backup_info::Global_iterator::Global_iterator(const Image_info &info)
+Backup_info::Global_iterator::Global_iterator(const Backup_info &info)
  :Iterator(info), mode(TABLESPACES), m_it(NULL), m_obj(NULL)
 {
-  // FIXME: detect errors (if NULL returned).
   m_it= m_info.get_tablespaces();
+  if (!m_it)
+  {
+    // Logs error, caller must check that ctx is valid afterwards
+    info.m_ctx.fatal_error(ER_BACKUP_CAT_ENUM);
+  }
   next();       // Note: next() doesn't report errors.
 }
 

=== modified file 'sql/backup/backup_kernel.h'
--- a/sql/backup/backup_kernel.h	2008-08-27 17:30:49 +0000
+++ b/sql/backup/backup_kernel.h	2008-09-01 12:53:37 +0000
@@ -127,7 +127,7 @@ class Backup_restore_ctx: public backup:
   bool m_tables_locked; 
 
   int lock_tables_for_restore();
-  int unlock_tables();
+  void unlock_tables();
   
   friend class Backup_info;
   friend class Restore_info;

=== modified file 'sql/backup/data_backup.cc'
--- a/sql/backup/data_backup.cc	2008-08-27 17:30:49 +0000
+++ b/sql/backup/data_backup.cc	2008-09-01 12:53:37 +0000
@@ -7,17 +7,6 @@
   drivers and protocols to create snapshot of the data stored in the tables being
   backed up.
 
-  @todo Fix error detection in places marked with "FIXME: detect errors...". 
-  These are places where functions or methods are called and if they can 
-  report errors it should be detected and appropriate action taken. If callee 
-  never reports errors or we want to ignore errors, a comment explaining this
-  should be added.
-
-  @todo Fix error logging in places marked with "FIXME: error logging...". In 
-  these places it should be decided if and how the error should be shown to the
-  user. If an error message should be logged, it can happen either in the place
-  where error was detected or somewhere up the call stack.
-
   @todo Implement better scheduling strategy in Scheduler::step
   @todo Add error reporting in the scheduler and elsewhere
   @todo If an error from driver is ignored (and operation retried) leave trace
@@ -104,7 +93,7 @@ class Block_writer
 
   result_t  get_buf(Buffer &);
   result_t  write_buf(const Buffer&);
-  result_t  drop_buf(Buffer&);
+  void      drop_buf(Buffer&);
 
   Block_writer(byte, size_t, Output_stream&);
   ~Block_writer();
@@ -477,9 +466,10 @@ int write_table_data(THD* thd, Backup_in
       if (init_size > max_init_size)
         max_init_size= init_size;
 
-      // FIXME: detect errors if reported.
-      // FIXME: error logging.
-      inactive.push_back(p);
+      if (inactive.push_back(p))
+      {
+        goto error;                           // Error has been logged
+      }
     }
   }
 
@@ -599,9 +589,13 @@ int write_table_data(THD* thd, Backup_in
       Save binlog information for point in time recovery on restore.
     */
     if (mysql_bin_log.is_open())
-      // FIXME: detect errors if reported.
-      // FIXME: error logging.
-      mysql_bin_log.get_current_log(&binlog_pos);
+      if (mysql_bin_log.get_current_log(&binlog_pos))
+      {
+        info.m_ctx.fatal_error(ER_BACKUP_BINLOG,
+                               info.m_ctx.m_type == backup::Logger::BACKUP 
+                               ? "BACKUP" : "RESTORE");
+        goto error;
+      }
 
     /*
       Save VP creation time.
@@ -788,8 +782,7 @@ int Scheduler::step()
       break;
 
     case backup_state::DONE:
-      // FIXME: detect errors if reported.
-      p->end();
+      res= p->end();  // Logs errors, fall-through to error handling below
 
     case backup_state::ERROR:
       remove_pump(p);   // Note: never errors.
@@ -1014,11 +1007,14 @@ Backup_pump::Backup_pump(Snapshot_info &
 {
   DBUG_ASSERT(snap.m_num > 0);
   m_buf.data= NULL;
-  // FIXME: detect errors if reported.
-  bitmap_init(&m_closed_streams,
+  if (bitmap_init(&m_closed_streams,
               NULL,
               1 + snap.table_count(),
-              FALSE); // not thread safe
+              FALSE)) // not thread safe
+  {
+    state= backup_state::ERROR;  // Created object will be invalid
+  }
+
   m_name= snap.name();
   if (ERROR == snap.get_backup_driver(m_drv) || !m_drv)
     state= backup_state::ERROR;
@@ -1253,10 +1249,9 @@ int Backup_pump::pump(size_t *howmuch)
 
         if (m_buf.size > 0)
           mode= WRITING;
-        else
-          // FIXME: detect errors (perhaps ensure that drop_buf never errors).
-          // FIXME: error logging.
-          m_bw.drop_buf(m_buf);
+        else {
+          m_bw.drop_buf(m_buf);              // Never errors
+        }
 
         break;
 
@@ -1274,9 +1269,7 @@ int Backup_pump::pump(size_t *howmuch)
         state= backup_state::DONE;
 
       case BUSY:
-        // FIXME: detect errors (perhaps ensure that drop_buf never errors).
-        // FIXME: error logging.
-        m_bw.drop_buf(m_buf);
+        m_bw.drop_buf(m_buf);                   // Never errors
         m_buf_head=NULL;  // thus a new request will be made
       }
 
@@ -1531,9 +1524,7 @@ int restore_table_data(THD *thd, Restore
           bad_drivers.append(",");
         bad_drivers.append(info.m_snap[n]->name());
       }
-      // FIXME: detect errors.
-      // FIXME: error logging.
-      drv[n]->free();
+      drv[n]->free();                           // Never errors
     }
 
     if (!bad_drivers.is_empty())
@@ -1551,9 +1542,7 @@ int restore_table_data(THD *thd, Restore
     if (!drv[n])
       continue;
 
-    // FIXME: detect errors.
-    // FIXME: error logging (perhaps we don't want to report errors here).
-    drv[n]->free();
+    drv[n]->free();  // Does not report errors
   }
 
   DBUG_RETURN(backup::ERROR);
@@ -1651,13 +1640,12 @@ Block_writer::write_buf(const Buffer &bu
   method can return it to the buffer pool so that it can be reused for other
   transfers.
  */
-Block_writer::result_t
+void
 Block_writer::drop_buf(Buffer &buf)
 {
   buf.data= NULL;
   buf.size= 0;
   taken= FALSE;
-  return OK;
 }
 
 } // backup namespace

=== modified file 'sql/backup/image_info.cc'
--- a/sql/backup/image_info.cc	2008-08-20 13:23:10 +0000
+++ b/sql/backup/image_info.cc	2008-09-01 12:53:37 +0000
@@ -8,16 +8,6 @@
 
   @brief Implements @c Image_info class and friends.
 
-  @todo Fix error detection in places marked with "FIXME: detect errors...". 
-  These are places where functions or methods are called and if they can 
-  report errors it should be detected and appropriate action taken. If callee 
-  never reports errors or we want to ignore errors, a comment explaining this
-  should be added.
-
-  @todo Fix error logging in places marked with "FIXME: error logging...". In 
-  these places it should be decided if and how the error should be shown to the
-  user. If an error message should be logged, it can happen either in the place
-  where error was detected or somewhere up the call stack.
 */
 
 namespace backup {
@@ -25,8 +15,7 @@ namespace backup {
 Image_info::Image_info()
   :data_size(0), m_table_count(0), m_dbs(16, 16), m_ts_map(16,16)
 {
-  // FIXME: detect errors if reported.
-  init_alloc_root(&mem_root, 4 * 1024, 0);
+  init_alloc_root(&mem_root, 4 * 1024, 0);      // Never errors
 
   /* initialize st_bstream_image_header members */
 
@@ -308,10 +297,8 @@ Image_info::add_table(Db &db, const ::St
 
   if (snap.add_table(*t, pos))  // reports errors
     return NULL;
-  
-  // FIXME: error logging.
-  if (db.add_table(*t))
-    return NULL;
+
+  db.add_table(*t);                             // Never errors
 
   if (!snap.m_num)
     snap.m_num= add_snapshot(snap); // reports errors

=== modified file 'sql/backup/image_info.h'
--- a/sql/backup/image_info.h	2008-08-20 13:23:10 +0000
+++ b/sql/backup/image_info.h	2008-09-01 12:53:37 +0000
@@ -4,10 +4,6 @@
 /**
   @file
   
-  @todo Fix error logging in places marked with "FIXME: error logging...". In 
-  these places it should be decided if and how the error should be shown to the
-  user. If an error message should be logged, it can happen either in the place
-  where error was detected or somewhere up the call stack.
 */ 
 
 #include <si_objects.h>
@@ -422,7 +418,7 @@ class Image_info::Db
   obs::Obj* materialize(uint ver, const ::String &sdata);
   result_t add_obj(Dbobj&, ulong pos);
   Dbobj*   get_obj(ulong pos) const;
-  result_t add_table(Table&);
+  void add_table(Table&);
   const char* describe(describe_buf&) const;
 
  private:
@@ -811,24 +807,21 @@ void Image_info::save_binlog_pos(const :
 inline
 Image_info::Db_iterator* Image_info::get_dbs() const
 {
-  // FIXME: error logging (in case allocation fails).
-  return new Db_iterator(*this);
+  return new Db_iterator(*this);  // Let caller handle allocation failures
 }
 
 /// Returns an iterator enumerating all tablespaces stored in backup catalogue.
 inline
 Image_info::Ts_iterator* Image_info::get_tablespaces() const
 {
-  // FIXME: error logging (in case allocation fails).
-  return new Ts_iterator(*this);
+  return new Ts_iterator(*this);  // Let caller handle allocation failures
 }
 
 /// Returns an iterator enumerating all objects in a given database.
 inline
 Image_info::Dbobj_iterator* Image_info::get_db_objects(const Db &db) const
 {
-  // FIXME: error logging (in case allocation fails).
-  return new Dbobj_iterator(*this, db);
+  return new Dbobj_iterator(*this, db); // Let caller handle allocation failures
 }
 
 /********************************************************************
@@ -1036,7 +1029,7 @@ obs::Obj* Image_info::Dbobj::materialize
   The table is appended to database's table list.
  */
 inline
-result_t Image_info::Db::add_table(Table &tbl)
+void Image_info::Db::add_table(Table &tbl)
 {
   tbl.next_table= NULL;
   
@@ -1047,8 +1040,6 @@ result_t Image_info::Db::add_table(Table
     last_table->next_table= &tbl;
     last_table= &tbl;
   }
-  
-  return OK;
 }
 
 /**

=== modified file 'sql/backup/kernel.cc'
--- a/sql/backup/kernel.cc	2008-08-27 17:30:49 +0000
+++ b/sql/backup/kernel.cc	2008-09-01 12:53:37 +0000
@@ -52,17 +52,6 @@
   } // if code jumps here, context destructor will do the clean-up automatically
   @endcode
 
-  @todo Fix error detection in places marked with "FIXME: detect errors...". 
-  These are places where functions or methods are called and if they can 
-  report errors it should be detected and appropriate action taken. If callee 
-  never reports errors or we want to ignore errors, a comment explaining this
-  should be added.
-
-  @todo Fix error logging in places marked with "FIXME: error logging...". In 
-  these places it should be decided if and how the error should be shown to the
-  user. If an error message should be logged, it can happen either in the place
-  where error was detected or somewhere up the call stack.
-
   @todo Use internal table name representation when passing tables to
         backup/restore drivers.
   @todo Handle other types of meta-data in Backup_info methods.
@@ -281,8 +270,10 @@ int send_error(Backup_restore_ctx &log, 
 /**
   Send positive reply after a backup/restore operation.
 
-  Currently the id of the operation is returned. It can be used to select
-  correct entries form the backup progress tables.
+  Currently the id of the operation is returned to the client. It can
+  be used to select correct entries from the backup progress tables.
+
+  @returns 0 on success, error code otherwise.
 */
 int send_reply(Backup_restore_ctx &context)
 {
@@ -295,34 +286,37 @@ int send_reply(Backup_restore_ctx &conte
   /*
     Send field list.
   */
-  // FIXME: detect errors if  reported.
-  // FIXME: error logging.
-  field_list.push_back(new Item_empty_string(STRING_WITH_LEN("backup_id")));
-  // FIXME: detect errors if  reported.
-  // FIXME: error logging.
-  protocol->send_fields(&field_list, Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF);
+  if (field_list.push_back(new Item_empty_string(STRING_WITH_LEN("backup_id"))))
+  {
+    goto err;
+  }
+  if (protocol->send_fields(&field_list,
+                            Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF))
+  {
+    goto err;
+  }
 
   /*
     Send field data.
   */
-  // FIXME: detect errors if  reported.
-  // FIXME: error logging.
-  protocol->prepare_for_resend();
-  // FIXME: detect errors if  reported.
-  // FIXME: error logging.
-  llstr(context.op_id(), buf);
-  // FIXME: detect errors if  reported.
-  // FIXME: error logging.
-  protocol->store(buf, system_charset_info);
-  // FIXME: detect errors if  reported.
-  // FIXME: error logging.
-  protocol->write();
-
-  // FIXME: detect errors if  reported.
-  // FIXME: error logging.
-  my_eof(context.thd());
-  context.report_cleanup();
+  protocol->prepare_for_resend();               // Never errors
+  llstr(context.op_id(), buf);                  // Never errors
+  if (protocol->store(buf, system_charset_info))
+  {
+    goto err;
+  }
+  if (protocol->write())
+  {
+    goto err;
+  }
+  my_eof(context.thd());                        // Never errors
+  context.report_cleanup();                     // Never errors
   DBUG_RETURN(0);
+
+ err:
+  DBUG_RETURN(context.fatal_error(ER_BACKUP_SEND_REPLY,
+                                  context.m_type == backup::Logger::BACKUP
+                                  ? "BACKUP" : "RESTORE"));
 }
 
 
@@ -406,13 +400,10 @@ int Backup_restore_ctx::prepare(LEX_STRI
   
   // Prepare error reporting context.
   
-  // FIXME: detect errors if  reported.
-  // FIXME: error logging.
-  mysql_reset_errors(m_thd, 0);
+  mysql_reset_errors(m_thd, 0);                 // Never errors
   m_thd->no_warnings_for_error= FALSE;
-  // FIXME: detect errors if  reported.
-  // FIXME: error logging.
-  save_errors();  
+
+  save_errors();                                // Never errors
 
 
   /*
@@ -566,7 +557,7 @@ Backup_restore_ctx::prepare_for_backup(S
     Create backup catalogue.
    */
 
-  Backup_info *info= new Backup_info(*this); // reports errors
+  Backup_info *info= new Backup_info(*this);    // Never errors
 
   if (!info)
   {
@@ -575,7 +566,7 @@ Backup_restore_ctx::prepare_for_backup(S
   }
 
   if (!info->is_valid())
-    return NULL;
+    return NULL;    // Error has been logged by Backup_Info contructor
 
   info->save_start_time(when);
   m_catalog= info;
@@ -731,14 +722,14 @@ int Backup_restore_ctx::lock_tables_for_
       backup::Image_info::Table *tbl= snap->get_table(t);
       DBUG_ASSERT(tbl); // All tables should be present in the catalogue.
 
-      // FIXME: detect errors. Don't assert here but report error instead.
-      // FIXME: error logging.
       TABLE_LIST *ptr= backup::mk_table_list(*tbl, TL_WRITE, m_thd->mem_root);
-      DBUG_ASSERT(ptr);
+      if (!ptr)
+      {
+        // Failed to allocate (failure has been logged), return error
+        return ER_OUT_OF_RESOURCES;
+      }
 
-      // FIXME: detect errors if reported.
-      // FIXME: error logging.
-      tables= backup::link_table_list(*ptr, tables);      
+      tables= backup::link_table_list(*ptr, tables); // Never errors
       tbl->m_table= ptr;
     }
   }
@@ -763,18 +754,18 @@ int Backup_restore_ctx::lock_tables_for_
 /**
   Unlock tables which were locked by @c lock_tables_for_restore.
  */ 
-int Backup_restore_ctx::unlock_tables()
+void Backup_restore_ctx::unlock_tables()
 {
   // Do nothing if tables are not locked.
   if (!m_tables_locked)
-    return 0;
+    return;
 
   DBUG_PRINT("restore",("unlocking tables"));
 
-  close_thread_tables(m_thd);
+  close_thread_tables(m_thd);                   // Never errors
   m_tables_locked= FALSE;
 
-  return 0;
+  return;
 }
 
 /**
@@ -791,6 +782,7 @@ int Backup_restore_ctx::unlock_tables()
  */ 
 int Backup_restore_ctx::close()
 {
+  int error= 0;
   if (m_state == CLOSED)
     return 0;
 
@@ -810,15 +802,10 @@ int Backup_restore_ctx::close()
   }
 
   // unlock tables if they are still locked
-
-  // FIXME: detect errors if reported.
-  unlock_tables();
+  unlock_tables();                              // Never errors
 
   // unfreeze meta-data
-
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  obs::ddl_blocker_disable();
+  obs::ddl_blocker_disable();                   // Never errors
 
   // restore thread options
 
@@ -826,10 +813,11 @@ int Backup_restore_ctx::close()
 
   // close stream
 
-  if (m_stream)
-    // FIXME: detect errors if reported.
-    // FIXME: error logging.
-    m_stream->close();
+  if (m_stream && !m_stream->close())
+  {
+    // Note error, but complete clean-up
+    error= ER_BACKUP_CLOSE;
+  }
 
   if (m_catalog)
     m_catalog->save_end_time(when); // Note: no errors.
@@ -861,19 +849,27 @@ int Backup_restore_ctx::close()
      */
     if (res && my_errno != ENOENT)
     {
-      report_error(ER_CANT_DELETE_FILE, m_path, my_errno);
-      if (!m_error)
-        m_error= ER_CANT_DELETE_FILE;
+      error= ER_CANT_DELETE_FILE;
     }
   }
 
-  // We report completion of the operation only if no errors were detected.
-
-  if (!m_error)
-    report_stop(when, TRUE);
+  /* We report completion of the operation only if no errors were detected,
+     and logger has been initialized.
+  */
+  if (!error)
+  {
+    if (backup::Logger::m_state == backup::Logger::RUNNING)
+    {
+      report_stop(when, TRUE);
+    }
+  }
+  else
+  {
+    fatal_error(error);                         // Log error
+  }
 
   m_state= CLOSED;
-  return m_error;
+  return error;
 }
 
 /**
@@ -901,9 +897,7 @@ int Backup_restore_ctx::do_backup()
 
   DEBUG_SYNC(m_thd, "before_backup_meta");
 
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  report_stats_pre(info);
+  report_stats_pre(info);                       // Never errors
 
   DBUG_PRINT("backup",("Writing preamble"));
   DEBUG_SYNC(m_thd, "backup_before_write_preamble");
@@ -918,7 +912,7 @@ int Backup_restore_ctx::do_backup()
 
   DEBUG_SYNC(m_thd, "before_backup_data");
 
-  if (write_table_data(m_thd, info, s)) // reports errors
+  if (write_table_data(m_thd, info, s)) // logs errors
     DBUG_RETURN(send_error(*this, ER_BACKUP_BACKUP));
 
   DBUG_PRINT("backup",("Writing summary"));
@@ -929,9 +923,7 @@ int Backup_restore_ctx::do_backup()
     DBUG_RETURN(m_error);
   }
 
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  report_stats_post(info);
+  report_stats_post(info);                      // Never errors
 
   DBUG_PRINT("backup",("Backup done."));
   DEBUG_SYNC(m_thd, "before_backup_done");
@@ -959,8 +951,11 @@ int Backup_restore_ctx::restore_triggers
 
   DBUG_ASSERT(m_catalog);
 
-  // FIXME: detect errors (when dbit==NULL). Perhaps just assert.
-  Image_info::Iterator *dbit= m_catalog->get_dbs();
+  Image_info::Iterator *dbit=m_catalog->get_dbs();
+  if (!dbit)
+  {
+    return fatal_error(ER_BACKUP_CAT_ENUM);
+  }
   Image_info::Obj *obj;
   List<Image_info::Obj> events;
   Image_info::Obj::describe_buf buf;
@@ -971,18 +966,20 @@ int Backup_restore_ctx::restore_triggers
   
   while ((obj= (*dbit)++)) 
   {
-    // FIXME: detect errors (when it==NULL). Perhaps just assert.
-    Image_info::Iterator *it= 
+    Image_info::Iterator *it=
                     m_catalog->get_db_objects(*static_cast<Image_info::Db*>(obj));
-
+    if (!it) {
+      DBUG_RETURN(fatal_error(ER_BACKUP_LIST_DB_TABLES));
+    }
     while ((obj= (*it)++))
       switch (obj->type()) {
       
       case BSTREAM_IT_EVENT:
         DBUG_ASSERT(obj->m_obj_ptr);
-        // FIXME: detect errors if reported.
-        // FIXME: error logging.
-        events.push_back(obj);
+        if (events.push_back(obj))
+        {
+          DBUG_RETURN(ER_OUT_OF_RESOURCES);     // Error has been logged
+        }
         break;
       
       case BSTREAM_IT_TRIGGER:
@@ -1044,27 +1041,24 @@ int Backup_restore_ctx::do_restore()
   Input_stream &s= *static_cast<Input_stream*>(m_stream);
   Restore_info &info= *static_cast<Restore_info*>(m_catalog);
 
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  report_stats_pre(info);
+  report_stats_pre(info);                       // Never errors
 
   DBUG_PRINT("restore", ("Restoring meta-data"));
 
-  // FIXME: detect errors if reported.
-  disable_fkey_constraints();  // reports errors
+  disable_fkey_constraints();                   // Never errors
 
   if (read_meta_data(info, s))
   {
-    // FIXME: detect errors.
-    // FIXME: error logging.
-    m_thd->main_da.reset_diagnostics_area();
+    m_thd->main_da.reset_diagnostics_area();    // Never errors
 
     fatal_error(ER_BACKUP_READ_META);
     DBUG_RETURN(m_error);
   }
 
-  // FIXME: detect errors.
-  s.next_chunk();
+  if (s.next_chunk() == BSTREAM_ERROR)
+  {
+    DBUG_RETURN(fatal_error(ER_BACKUP_NEXT_CHUNK));
+  }
 
   DBUG_PRINT("restore",("Restoring table data"));
 
@@ -1074,21 +1068,16 @@ int Backup_restore_ctx::do_restore()
     It should be fixed inside object services implementation and then the
     following line should be removed.
    */
-  // FIXME: detect errors.
-  // FIXME: error logging.
-  close_thread_tables(m_thd);
-  // FIXME: detect errors.
-  // FIXME: error logging.
-  m_thd->main_da.reset_diagnostics_area();
+  close_thread_tables(m_thd);                   // Never errors
+  m_thd->main_da.reset_diagnostics_area();      // Never errors  
 
-  if (lock_tables_for_restore()) // reports errors
+  if (lock_tables_for_restore())                // logs errors
     DBUG_RETURN(m_error);
 
   // Here restore drivers are created to restore table data
   err= restore_table_data(m_thd, info, s); // reports errors
 
-  // FIXME: detect errors if reported.
-  unlock_tables();
+  unlock_tables();                              // Never errors
 
   if (err)
     DBUG_RETURN(ER_BACKUP_RESTORE);
@@ -1117,16 +1106,10 @@ int Backup_restore_ctx::do_restore()
     It should be fixed inside object services implementation and then the
     following line should be removed.
    */
-  // FIXME: detect errors.
-  // FIXME: error logging.
-  close_thread_tables(m_thd);
-  // FIXME: detect errors.
-  // FIXME: error logging.
-  m_thd->main_da.reset_diagnostics_area();
-
-  // FIXME: detect errors if reported.
-  // FIXME: error logging.
-  report_stats_post(info);
+  close_thread_tables(m_thd);                   // Never errors
+  m_thd->main_da.reset_diagnostics_area();      // Never errors
+
+  report_stats_post(info);                      // Never errors
 
   DBUG_RETURN(0);
 }
@@ -1530,10 +1513,12 @@ void* bcat_iterator_get(st_bstream_image
   case BSTREAM_IT_GLOBAL:   // all global objects
   {
     Iterator *it= info->get_global();
-  
     if (!it)
     {
       info->m_ctx.fatal_error(ER_BACKUP_CAT_ENUM);
+    }
+    if (!info->m_ctx.is_valid())  // Fatal error has occurred
+    {
       return NULL;
     }
 

=== modified file 'sql/backup/stream.cc'
--- a/sql/backup/stream.cc	2008-08-08 17:21:31 +0000
+++ b/sql/backup/stream.cc	2008-09-01 12:53:37 +0000
@@ -349,18 +349,26 @@ int Stream::prepare_path(::String *backu
 
 bool Stream::open()
 {
-  close();
+  if (!close())
+  {
+    return false;
+  }
   m_fd= my_open(m_path.c_ptr(), m_flags, MYF(0));
   return m_fd >= 0;
 }
 
-void Stream::close()
+bool Stream::close()
 {
+  bool ret= true;
   if (m_fd >= 0)
   {
-    my_close(m_fd, MYF(0));
+    if (my_close(m_fd, MYF(0)))
+    {
+      ret= false;
+    }
     m_fd= -1;
   }
+  return ret;
 }
 
 bool Stream::rewind()
@@ -453,7 +461,10 @@ bool Output_stream::init()
 bool Output_stream::open()
 {
   MY_STAT stat_info;
-  close();
+  if (!close())
+  {
+    return false;
+  }
 
   /* Allow to write to existing named pipe */
   if (my_stat(m_path.c_ptr(), &stat_info, MYF(0)) &&
@@ -504,12 +515,17 @@ bool Output_stream::open()
 
   If @c destroy is TRUE, the stream object is deleted.
 */
-void Output_stream::close()
+bool Output_stream::close()
 {
+  bool ret= true;
   if (m_fd < 0)
-    return;
+    return true;
+
+  if (bstream_close(this) == BSTREAM_ERROR)
+  {
+    ret= false;
+  }
 
-  bstream_close(this);
 #ifdef HAVE_COMPRESS
   if (m_with_compression)
   {
@@ -538,7 +554,8 @@ void Output_stream::close()
     my_free(zbuf, MYF(0));
   }
 #endif
-  Stream::close();
+  ret &= Stream::close();
+  return ret;
 }
 
 /**
@@ -634,7 +651,10 @@ bool Input_stream::init()
 */
 bool Input_stream::open()
 {
-  close();
+  if (!close())
+  {
+    return false;
+  }
 
   bool ret= Stream::open();
 
@@ -685,12 +705,17 @@ bool Input_stream::open()
 
   If @c destroy is TRUE, the stream object is deleted.
 */
-void Input_stream::close()
+bool Input_stream::close()
 {
+  bool ret= true;
   if (m_fd < 0)
-    return;
+    return true;
+
+  if (bstream_close(this) == BSTREAM_ERROR)
+  {
+    ret= false;
+  }
 
-  bstream_close(this);
 #ifdef HAVE_COMPRESS
   if (m_with_compression)
   {
@@ -700,7 +725,8 @@ void Input_stream::close()
     my_free(zbuf, (MYF(0)));
   }
 #endif
-  Stream::close();
+  ret &= Stream::close();
+  return ret;
 }
 
 /**

=== modified file 'sql/backup/stream.h'
--- a/sql/backup/stream.h	2008-08-08 17:21:31 +0000
+++ b/sql/backup/stream.h	2008-09-01 12:53:37 +0000
@@ -80,7 +80,7 @@ class Stream: public fd_stream
  public:
 
   bool open();
-  virtual void close();
+  virtual bool close();
   bool rewind();
 
   /// Check if stream is opened
@@ -121,7 +121,7 @@ class Output_stream:
   Output_stream(Logger&, ::String *, LEX_STRING, bool);
 
   bool open();
-  void close();
+  bool close();
   bool rewind();
 
  private:
@@ -139,7 +139,7 @@ class Input_stream:
   Input_stream(Logger&, ::String *, LEX_STRING);
 
   bool open();
-  void close();
+  bool close();
   bool rewind();
 
   int next_chunk();

=== modified file 'sql/mdl.cc'
--- a/sql/mdl.cc	2008-07-07 15:51:20 +0000
+++ b/sql/mdl.cc	2008-09-01 12:53:37 +0000
@@ -294,7 +294,7 @@ void mdl_init_lock(MDL_LOCK_DATA *lock_d
 
    @note The allocated lock request will have MDL_SHARED type.
 
-   @retval 0      Error
+   @retval 0      Error if out of memory
    @retval non-0  Pointer to an object representing a lock request
 */
 

=== modified file 'sql/share/errmsg.txt'
--- a/sql/share/errmsg.txt	2008-08-27 17:30:49 +0000
+++ b/sql/share/errmsg.txt	2008-09-01 12:53:37 +0000
@@ -6388,7 +6388,20 @@ ER_BACKUP_GRANT_SKIPPED
         eng "The grant '%-.64s' was skipped because the user does not exist."
 ER_BACKUP_GRANT_WRONG_DB
         eng "The grant '%-.64s' failed. Database not included in the backup image."
+ER_BACKUP_SEND_REPLY
+        eng "Failed to send reply to client after successful %-.64s operation"
+        nor "Mislykket sending av svar til klient etter %-.64s"
+        norwegian-ny "Sendinga av svar til klienten etter %-.64s"
+ER_BACKUP_CLOSE
+        eng "Backup/Restore: Error on close of backup stream"
+        nor "Backup/Restore: Feil ved lukning av backup-strøm"
+        norwegian-ny "Backup/Restore:  Feil ved lukking av backup-straum"
+ER_BACKUP_BINLOG
+        eng "Error on accessing binlog during %-.64s"
+        nor "Lesing av binlog feilet under %-.64s"
+        norwegian-ny "Lesing av binlog feila under %-.64s"
 ER_BACKUP_LOGPATHS
   eng "The log names for backup_history and backup_progress must be unique."
 ER_BACKUP_LOGPATH_INVALID
   eng "The path specified for the %-.64s is invalid. ref: %-.64s"
+

Thread
bzr commit into mysql-6.0-backup branch (oystein.grovlen:2688) Bug#39089WL#4384oystein.grovlen1 Sep