Ceph » rgw

• obscure error handling and control flow (goto out)
• rely on 'external' state like this, member functions (Schema/Prepare/Bind), int rc for errors, and goto labels (goto out)
• macro substitution duplicates a lot of code, especially in big functions like SQLPutObject::Bind()

i do understand why it was done this way; some ops bind a lot variables, and this hides all of the error handling from the calling code:

int SQLListBucketObjects::Bind(const DoutPrefixProvider *dpp, struct DBOpParams *params)
{
  int index = -1;
  int rc = 0;
  struct DBOpPrepareParams p_params = PrepareParams;

  SQL_BIND_INDEX(dpp, stmt, index, p_params.op.bucket.bucket_name.c_str(), sdb);
  SQL_BIND_TEXT(dpp, stmt, index, params->op.bucket.info.bucket.name.c_str(), sdb);

  SQL_BIND_INDEX(dpp, stmt, index, p_params.op.obj.min_marker.c_str(), sdb);
  SQL_BIND_TEXT(dpp, stmt, index, params->op.obj.min_marker.c_str(), sdb);

  SQL_BIND_INDEX(dpp, stmt, index, p_params.op.list_max_count.c_str(), sdb);
  SQL_BIND_INT(dpp, stmt, index, params->op.list_max_count, sdb);

out:
  return rc;
}

the c++ way to 'hide' error handling is with exceptions. unlike goto (which can only jump to labels in the same function), exceptions can propagate up the call stack through function calls. so each of these macros could be a function call to avoid all of the code duplication, and some higher level function (maybe Execute() or ProcessOp()?) can handle all of the errors

however, you do have to be careful with exceptions when there's cleanup code to run. for example, if a function allocates memory that needs cleanup after 'goto out':

int SQLExampleOp::Bind(const DoutPrefixProvider *dpp, const DBOpParams *params)
{
  int index = -1;
  int rc = 0;
  struct DBOpPrepareParams p_params = PrepareParams;

  auto state = new ExampleState; // example memory allocation

  SQL_BIND_INDEX(dpp, stmt, index, p_params.op.bucket.bucket_name.c_str(), sdb);
  SQL_BIND_TEXT(dpp, stmt, index, params->op.bucket.info.bucket.name.c_str(), sdb);

out:
  delete state; // make sure memory is released on error
  return rc;
}

if one of the macros were to throw an exception instead of 'goto out', then the stack would unwind without running that cleanup code, and this memory would leak. fortunately, when unwinding the stack, all of the destructors of local variables will have a chance to run. so it's important to be strict about RAII when using exceptions. in this example, std::unique_ptr would fix the leak:

  auto state = std::make_uniquie<ExampleState>();