From 770989b5547dfd8a2da88a29567fe4dc62626d8c Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Mon, 17 May 2021 08:51:26 -0700 Subject: [PATCH] Normalize grpc_error_handle usage (#26240) --- .../filters/client_idle/client_idle_filter.cc | 2 +- .../chttp2/transport/chttp2_transport.cc | 8 +-- .../ext/transport/chttp2/transport/parsing.cc | 4 +- .../ext/transport/inproc/inproc_transport.cc | 64 ++++++++++--------- src/core/lib/iomgr/error.cc | 9 +-- src/core/lib/iomgr/error.h | 2 +- src/core/lib/iomgr/tcp_server_custom.cc | 7 +- src/core/lib/transport/error_utils.cc | 4 +- src/core/lib/transport/transport_op_string.cc | 2 +- 9 files changed, 52 insertions(+), 50 deletions(-) diff --git a/src/core/ext/filters/client_idle/client_idle_filter.cc b/src/core/ext/filters/client_idle/client_idle_filter.cc index 01abaab7807..6255d911924 100644 --- a/src/core/ext/filters/client_idle/client_idle_filter.cc +++ b/src/core/ext/filters/client_idle/client_idle_filter.cc @@ -187,7 +187,7 @@ void ChannelData::StartTransportOp(grpc_channel_element* elem, grpc_transport_op* op) { ChannelData* chand = static_cast(elem->channel_data); // Catch the disconnect_with_error transport op. - if (op->disconnect_with_error != nullptr) { + if (op->disconnect_with_error != GRPC_ERROR_NONE) { // IncreaseCallCount() introduces a phony call and prevent the timer from // being reset by other threads. chand->IncreaseCallCount(); diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 1ff64756c92..6a2a8300246 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -564,7 +564,7 @@ static void close_transport_locked(grpc_chttp2_transport* t, GRPC_STATUS_UNAVAILABLE); } if (t->write_state != GRPC_CHTTP2_WRITE_STATE_IDLE) { - if (t->close_transport_on_writes_finished == nullptr) { + if (t->close_transport_on_writes_finished == GRPC_ERROR_NONE) { t->close_transport_on_writes_finished = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "Delayed close due to in-progress write"); @@ -821,9 +821,9 @@ static void set_write_state(grpc_chttp2_transport* t, // from peer while we had some pending writes) if (st == GRPC_CHTTP2_WRITE_STATE_IDLE) { grpc_core::ExecCtx::RunList(DEBUG_LOCATION, &t->run_after_write); - if (t->close_transport_on_writes_finished != nullptr) { + if (t->close_transport_on_writes_finished != GRPC_ERROR_NONE) { grpc_error_handle err = t->close_transport_on_writes_finished; - t->close_transport_on_writes_finished = nullptr; + t->close_transport_on_writes_finished = GRPC_ERROR_NONE; close_transport_locked(t, err); } } @@ -1795,7 +1795,7 @@ static void perform_transport_op_locked(void* stream_op, grpc_chttp2_transport* t = static_cast(op->handler_private.extra_arg); - if (op->goaway_error) { + if (op->goaway_error != GRPC_ERROR_NONE) { send_goaway(t, op->goaway_error); } diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index 68e7c69c65f..d588f554ee7 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -246,10 +246,10 @@ grpc_error_handle grpc_chttp2_perform_read(grpc_chttp2_transport* t, t->incoming_frame_size -= static_cast(end - cur); return GRPC_ERROR_NONE; } - GPR_UNREACHABLE_CODE(return nullptr); + GPR_UNREACHABLE_CODE(return GRPC_ERROR_NONE); } - GPR_UNREACHABLE_CODE(return nullptr); + GPR_UNREACHABLE_CODE(return GRPC_ERROR_NONE); } static grpc_error_handle init_frame_parser(grpc_chttp2_transport* t) { diff --git a/src/core/ext/transport/inproc/inproc_transport.cc b/src/core/ext/transport/inproc/inproc_transport.cc index 0c21db7f3af..1fa256a1751 100644 --- a/src/core/ext/transport/inproc/inproc_transport.cc +++ b/src/core/ext/transport/inproc/inproc_transport.cc @@ -388,7 +388,8 @@ void complete_if_batch_end_locked(inproc_stream* s, grpc_error_handle error, int is_rtm = static_cast(op == s->recv_trailing_md_op); if ((is_sm + is_stm + is_rim + is_rm + is_rtm) == 1) { - INPROC_LOG(GPR_INFO, "%s %p %p %p", msg, s, op, error); + INPROC_LOG(GPR_INFO, "%s %p %p %s", msg, s, op, + grpc_error_std_string(error).c_str()); grpc_core::ExecCtx::Run(DEBUG_LOCATION, op->on_complete, GRPC_ERROR_REF(error)); } @@ -468,8 +469,9 @@ void fail_helper_locked(inproc_stream* s, grpc_error_handle error) { .trailing_metadata_available = true; } INPROC_LOG(GPR_INFO, - "fail_helper %p scheduling initial-metadata-ready %p %p", s, - error, err); + "fail_helper %p scheduling initial-metadata-ready %s %s", s, + grpc_error_std_string(error).c_str(), + grpc_error_std_string(err).c_str()); grpc_core::ExecCtx::Run( DEBUG_LOCATION, s->recv_initial_md_op->payload->recv_initial_metadata @@ -483,8 +485,8 @@ void fail_helper_locked(inproc_stream* s, grpc_error_handle error) { s->recv_initial_md_op = nullptr; } if (s->recv_message_op) { - INPROC_LOG(GPR_INFO, "fail_helper %p scheduling message-ready %p", s, - error); + INPROC_LOG(GPR_INFO, "fail_helper %p scheduling message-ready %s", s, + grpc_error_std_string(error).c_str()); grpc_core::ExecCtx::Run( DEBUG_LOCATION, s->recv_message_op->payload->recv_message.recv_message_ready, @@ -508,15 +510,15 @@ void fail_helper_locked(inproc_stream* s, grpc_error_handle error) { s->send_trailing_md_op = nullptr; } if (s->recv_trailing_md_op) { - INPROC_LOG(GPR_INFO, "fail_helper %p scheduling trailing-metadata-ready %p", - s, error); + INPROC_LOG(GPR_INFO, "fail_helper %p scheduling trailing-metadata-ready %s", + s, grpc_error_std_string(error).c_str()); grpc_core::ExecCtx::Run( DEBUG_LOCATION, s->recv_trailing_md_op->payload->recv_trailing_metadata .recv_trailing_metadata_ready, GRPC_ERROR_REF(error)); - INPROC_LOG(GPR_INFO, "fail_helper %p scheduling trailing-md-on-complete %p", - s, error); + INPROC_LOG(GPR_INFO, "fail_helper %p scheduling trailing-md-on-complete %s", + s, grpc_error_std_string(error).c_str()); complete_if_batch_end_locked( s, error, s->recv_trailing_md_op, "fail_helper scheduling recv-trailing-metadata-on-complete"); @@ -685,8 +687,8 @@ void op_state_machine_locked(inproc_stream* s, grpc_error_handle error) { INPROC_LOG( GPR_INFO, "op_state_machine %p scheduling on_complete errors for already " - "recvd initial md %p", - s, new_err); + "recvd initial md %s", + s, grpc_error_std_string(new_err).c_str()); fail_helper_locked(s, GRPC_ERROR_REF(new_err)); goto done; } @@ -710,8 +712,8 @@ void op_state_machine_locked(inproc_stream* s, grpc_error_handle error) { grpc_metadata_batch_clear(&s->to_read_initial_md); s->to_read_initial_md_filled = false; INPROC_LOG(GPR_INFO, - "op_state_machine %p scheduling initial-metadata-ready %p", s, - new_err); + "op_state_machine %p scheduling initial-metadata-ready %s", s, + grpc_error_std_string(new_err).c_str()); grpc_core::ExecCtx::Run( DEBUG_LOCATION, s->recv_initial_md_op->payload->recv_initial_metadata @@ -724,8 +726,8 @@ void op_state_machine_locked(inproc_stream* s, grpc_error_handle error) { if (new_err != GRPC_ERROR_NONE) { INPROC_LOG(GPR_INFO, - "op_state_machine %p scheduling on_complete errors2 %p", s, - new_err); + "op_state_machine %p scheduling on_complete errors2 %s", s, + grpc_error_std_string(new_err).c_str()); fail_helper_locked(s, GRPC_ERROR_REF(new_err)); goto done; } @@ -753,8 +755,8 @@ void op_state_machine_locked(inproc_stream* s, grpc_error_handle error) { INPROC_LOG( GPR_INFO, "op_state_machine %p scheduling on_complete errors for already " - "recvd trailing md %p", - s, new_err); + "recvd trailing md %s", + s, grpc_error_std_string(new_err).c_str()); fail_helper_locked(s, GRPC_ERROR_REF(new_err)); goto done; } @@ -801,8 +803,8 @@ void op_state_machine_locked(inproc_stream* s, grpc_error_handle error) { // a final status, so don't mark this op complete) if (s->t->is_client || s->trailing_md_sent) { INPROC_LOG(GPR_INFO, - "op_state_machine %p scheduling trailing-md-on-complete %p", - s, new_err); + "op_state_machine %p scheduling trailing-md-on-complete %s", + s, grpc_error_std_string(new_err).c_str()); grpc_core::ExecCtx::Run( DEBUG_LOCATION, s->recv_trailing_md_op->payload->recv_trailing_metadata @@ -816,8 +818,8 @@ void op_state_machine_locked(inproc_stream* s, grpc_error_handle error) { } else { INPROC_LOG(GPR_INFO, "op_state_machine %p server needs to delay handling " - "trailing-md-on-complete %p", - s, new_err); + "trailing-md-on-complete %s", + s, grpc_error_std_string(new_err).c_str()); } } else if (!s->trailing_md_recvd) { INPROC_LOG( @@ -830,8 +832,8 @@ void op_state_machine_locked(inproc_stream* s, grpc_error_handle error) { // In this case, we don't care to receive the write-close from the client // because we have already sent status and the RPC is over as far as we // are concerned. - INPROC_LOG(GPR_INFO, "op_state_machine %p scheduling trailing-md-ready %p", - s, new_err); + INPROC_LOG(GPR_INFO, "op_state_machine %p scheduling trailing-md-ready %s", + s, grpc_error_std_string(new_err).c_str()); grpc_core::ExecCtx::Run( DEBUG_LOCATION, s->recv_trailing_md_op->payload->recv_trailing_metadata @@ -1091,8 +1093,8 @@ void perform_stream_op(grpc_transport* gt, grpc_stream* gs, } INPROC_LOG( GPR_INFO, - "perform_stream_op error %p scheduling initial-metadata-ready %p", - s, error); + "perform_stream_op error %p scheduling initial-metadata-ready %s", + s, grpc_error_std_string(error).c_str()); grpc_core::ExecCtx::Run( DEBUG_LOCATION, op->payload->recv_initial_metadata.recv_initial_metadata_ready, @@ -1101,8 +1103,8 @@ void perform_stream_op(grpc_transport* gt, grpc_stream* gs, if (op->recv_message) { INPROC_LOG( GPR_INFO, - "perform_stream_op error %p scheduling recv message-ready %p", s, - error); + "perform_stream_op error %p scheduling recv message-ready %s", s, + grpc_error_std_string(error).c_str()); grpc_core::ExecCtx::Run(DEBUG_LOCATION, op->payload->recv_message.recv_message_ready, GRPC_ERROR_REF(error)); @@ -1110,16 +1112,16 @@ void perform_stream_op(grpc_transport* gt, grpc_stream* gs, if (op->recv_trailing_metadata) { INPROC_LOG( GPR_INFO, - "perform_stream_op error %p scheduling trailing-metadata-ready %p", - s, error); + "perform_stream_op error %p scheduling trailing-metadata-ready %s", + s, grpc_error_std_string(error).c_str()); grpc_core::ExecCtx::Run( DEBUG_LOCATION, op->payload->recv_trailing_metadata.recv_trailing_metadata_ready, GRPC_ERROR_REF(error)); } } - INPROC_LOG(GPR_INFO, "perform_stream_op %p scheduling on_complete %p", s, - error); + INPROC_LOG(GPR_INFO, "perform_stream_op %p scheduling on_complete %s", s, + grpc_error_std_string(error).c_str()); grpc_core::ExecCtx::Run(DEBUG_LOCATION, on_complete, GRPC_ERROR_REF(error)); } gpr_mu_unlock(mu); diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc index 378bdf72954..c47c7928dd9 100644 --- a/src/core/lib/iomgr/error.cc +++ b/src/core/lib/iomgr/error.cc @@ -59,7 +59,7 @@ absl::Status grpc_status_create(absl::StatusCode code, absl::string_view msg, absl::Status s = StatusCreate(code, msg, location, {}); for (size_t i = 0; i < children_count; ++i) { if (!children[i].ok()) { - StatusAddChild(&s, children[i]); + grpc_core::StatusAddChild(&s, children[i]); } } return s; @@ -73,10 +73,11 @@ absl::Status grpc_os_error(const grpc_core::DebugLocation& location, int err, const char* call_name) { absl::Status s = StatusCreate(absl::StatusCode::kUnknown, "OS Error", location, {}); - grpc_core::StatusSetInt(&s, grpc_core::StatusIntProperty::ERRNO, err); - grpc_core::StatusSetStr(&s, grpc_core::StatusStrProperty::OS_ERROR, + grpc_core::StatusSetInt(&s, grpc_core::StatusIntProperty::kErrorNo, err); + grpc_core::StatusSetStr(&s, grpc_core::StatusStrProperty::kOsError, strerror(err)); - grpc_core::StatusSetStr(&s, grpc_core::StatusStrProperty::SYSCALL, call_name); + grpc_core::StatusSetStr(&s, grpc_core::StatusStrProperty::kSyscall, + call_name); return s; } diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h index aecd1e907cc..e0492b809b4 100644 --- a/src/core/lib/iomgr/error.h +++ b/src/core/lib/iomgr/error.h @@ -166,7 +166,7 @@ void grpc_enable_error_creation(); #define GRPC_ERROR_CREATE_FROM_COPIED_STRING(desc) \ StatusCreate(absl::StatusCode::kUnknown, desc, DEBUG_LOCATION, {}) #define GRPC_ERROR_CREATE_FROM_STRING_VIEW(desc) \ - StatusCreate(ababsl::StatusCode::kUnknown, desc, DEBUG_LOCATION, {}) + StatusCreate(absl::StatusCode::kUnknown, desc, DEBUG_LOCATION, {}) absl::Status grpc_status_create(absl::StatusCode code, absl::string_view msg, const grpc_core::DebugLocation& location, diff --git a/src/core/lib/iomgr/tcp_server_custom.cc b/src/core/lib/iomgr/tcp_server_custom.cc index fff75453f02..8ffd728a98a 100644 --- a/src/core/lib/iomgr/tcp_server_custom.cc +++ b/src/core/lib/iomgr/tcp_server_custom.cc @@ -362,10 +362,9 @@ static grpc_error_handle tcp_server_add_port(grpc_tcp_server* s, for (sp = s->head; sp; sp = sp->next) { socket = sp->socket; sockname_temp.len = GRPC_MAX_SOCKADDR_SIZE; - if (nullptr == grpc_custom_socket_vtable->getsockname( - socket, - reinterpret_cast(&sockname_temp.addr), - reinterpret_cast(&sockname_temp.len))) { + if (grpc_custom_socket_vtable->getsockname( + socket, reinterpret_cast(&sockname_temp.addr), + reinterpret_cast(&sockname_temp.len)) == GRPC_ERROR_NONE) { *port = grpc_sockaddr_get_port(&sockname_temp); if (*port > 0) { allocated_addr = static_cast( diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc index e8f05550c96..2d75a56a6de 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -74,7 +74,7 @@ void grpc_error_get_status(grpc_error_handle error, grpc_millis deadline, // until we find the first one that has a status code. grpc_error_handle found_error = recursively_find_error_with_field(error, GRPC_ERROR_INT_GRPC_STATUS); - if (found_error == nullptr) { + if (found_error == GRPC_ERROR_NONE) { /// If no grpc-status exists, retry through the tree to find a http2 error /// code found_error = @@ -83,7 +83,7 @@ void grpc_error_get_status(grpc_error_handle error, grpc_millis deadline, // If we found an error with a status code above, use that; otherwise, // fall back to using the parent error. - if (found_error == nullptr) found_error = error; + if (found_error == GRPC_ERROR_NONE) found_error = error; grpc_status_code status = GRPC_STATUS_UNKNOWN; intptr_t integer; diff --git a/src/core/lib/transport/transport_op_string.cc b/src/core/lib/transport/transport_op_string.cc index 49dec384ee6..0285ac09902 100644 --- a/src/core/lib/transport/transport_op_string.cc +++ b/src/core/lib/transport/transport_op_string.cc @@ -135,7 +135,7 @@ std::string grpc_transport_op_string(grpc_transport_op* op) { " DISCONNECT:", grpc_error_std_string(op->disconnect_with_error))); } - if (op->goaway_error) { + if (op->goaway_error != GRPC_ERROR_NONE) { out.push_back(absl::StrCat(" SEND_GOAWAY:%s", grpc_error_std_string(op->goaway_error))); }