From e484a4656d5066d1f3b2107eca7ef1d3c9e76ee9 Mon Sep 17 00:00:00 2001 From: Pavlin Radoslavov Date: Wed, 7 Oct 2015 18:07:48 -0700 Subject: [PATCH] Update the usage of fixed_queue. * Relax non-NULL requirements for fixed_queue by eliminating some of the asserts. Now, when semantically possible, fixed_queue - related function will return the appropriate value even if the queue pointer is NULL. This reduces clutter in the code where we had to do anyway "if (queue != NULL)" checks. * Add non-NULL guards in the few remaining places where fixed_queue_get_list(). For now, we need to use this function, hence the extra check. That function should be eliminated in the future, because all the code where it is used violates the semantics of using a queue. Bug: 24723840 Change-Id: I47632a3515f3d27856d4870e18723d345c040d64 --- osi/include/fixed_queue.h | 16 +++---- osi/src/fixed_queue.c | 20 +++++---- stack/avdt/avdt_scb_act.c | 90 ++++++++++++++++++++------------------- stack/btm/btm_sec.c | 4 +- stack/gatt/gatt_sr.c | 20 +++++---- stack/gatt/gatt_utils.c | 11 ++++- stack/l2cap/l2c_fcr.c | 105 ++++++++++++++++++++++++++-------------------- 7 files changed, 151 insertions(+), 115 deletions(-) diff --git a/osi/include/fixed_queue.h b/osi/include/fixed_queue.h index 6902d6b7b..bb066f812 100644 --- a/osi/include/fixed_queue.h +++ b/osi/include/fixed_queue.h @@ -45,11 +45,12 @@ fixed_queue_t *fixed_queue_new(size_t capacity); // blocked on it) results in undefined behaviour. void fixed_queue_free(fixed_queue_t *queue, fixed_queue_free_cb free_cb); -// Returns a value indicating whether the given |queue| is empty. |queue| may -// not be NULL. +// Returns a value indicating whether the given |queue| is empty. If |queue| +// is NULL, the return value is true. bool fixed_queue_is_empty(fixed_queue_t *queue); -// Returns the length of the |queue|. |queue| may not be NULL. +// Returns the length of the |queue|. If |queue| is NULL, the return value +// is 0. size_t fixed_queue_length(fixed_queue_t *queue); // Returns the maximum number of elements this queue may hold. |queue| may @@ -73,19 +74,18 @@ void *fixed_queue_dequeue(fixed_queue_t *queue); bool fixed_queue_try_enqueue(fixed_queue_t *queue, void *data); // Tries to dequeue an element from |queue|. This function will never block -// the caller. If the queue is empty, this function returns NULL immediately. -// Otherwise, the next element in the queue is returned. |queue| may not be -// NULL. +// the caller. If the queue is empty or NULL, this function returns NULL +// immediately. Otherwise, the next element in the queue is returned. void *fixed_queue_try_dequeue(fixed_queue_t *queue); // Returns the first element from |queue|, if present, without dequeuing it. // This function will never block the caller. Returns NULL if there are no -// elements in the queue. |queue| may not be NULL. +// elements in the queue or |queue| is NULL. void *fixed_queue_try_peek_first(fixed_queue_t *queue); // Returns the last element from |queue|, if present, without dequeuing it. // This function will never block the caller. Returns NULL if there are no -// elements in the queue. |queue| may not be NULL. +// elements in the queue or |queue| is NULL. void *fixed_queue_try_peek_last(fixed_queue_t *queue); // Tries to remove a |data| element from the middle of the |queue|. This diff --git a/osi/src/fixed_queue.c b/osi/src/fixed_queue.c index 03a252cf8..5db43a521 100644 --- a/osi/src/fixed_queue.c +++ b/osi/src/fixed_queue.c @@ -116,7 +116,8 @@ void fixed_queue_free(fixed_queue_t *queue, fixed_queue_free_cb free_cb) { } bool fixed_queue_is_empty(fixed_queue_t *queue) { - assert(queue != NULL); + if (queue == NULL) + return true; pthread_mutex_lock(&queue->lock); bool is_empty = list_is_empty(queue->list); @@ -126,7 +127,8 @@ bool fixed_queue_is_empty(fixed_queue_t *queue) { } size_t fixed_queue_length(fixed_queue_t *queue) { - assert(queue != NULL); + if (queue == NULL) + return 0; pthread_mutex_lock(&queue->lock); size_t length = list_length(queue->list); @@ -185,7 +187,8 @@ bool fixed_queue_try_enqueue(fixed_queue_t *queue, void *data) { } void *fixed_queue_try_dequeue(fixed_queue_t *queue) { - assert(queue != NULL); + if (queue == NULL) + return NULL; if (!semaphore_try_wait(queue->dequeue_sem)) return NULL; @@ -201,7 +204,8 @@ void *fixed_queue_try_dequeue(fixed_queue_t *queue) { } void *fixed_queue_try_peek_first(fixed_queue_t *queue) { - assert(queue != NULL); + if (queue == NULL) + return NULL; pthread_mutex_lock(&queue->lock); void *ret = list_is_empty(queue->list) ? NULL : list_front(queue->list); @@ -211,7 +215,8 @@ void *fixed_queue_try_peek_first(fixed_queue_t *queue) { } void *fixed_queue_try_peek_last(fixed_queue_t *queue) { - assert(queue != NULL); + if (queue == NULL) + return NULL; pthread_mutex_lock(&queue->lock); void *ret = list_is_empty(queue->list) ? NULL : list_back(queue->list); @@ -221,7 +226,8 @@ void *fixed_queue_try_peek_last(fixed_queue_t *queue) { } void *fixed_queue_try_remove_from_queue(fixed_queue_t *queue, void *data) { - assert(queue != NULL); + if (queue == NULL) + return NULL; pthread_mutex_lock(&queue->lock); bool removed = list_remove(queue->list, data); @@ -236,7 +242,7 @@ list_t *fixed_queue_get_list(fixed_queue_t *queue) { assert(queue != NULL); // NOTE: This function is not thread safe, and there is no point for - // callint pthread_mutex_lock() / pthread_mutex_unlock() + // calling pthread_mutex_lock() / pthread_mutex_unlock() return queue->list; } diff --git a/stack/avdt/avdt_scb_act.c b/stack/avdt/avdt_scb_act.c index fd5ba8370..de072b3c4 100644 --- a/stack/avdt/avdt_scb_act.c +++ b/stack/avdt/avdt_scb_act.c @@ -1287,51 +1287,53 @@ void avdt_scb_hdl_write_req_frag(tAVDT_SCB *p_scb, tAVDT_SCB_EVT *p_data) ssrc = avdt_scb_gen_ssrc(p_scb); - list_t *list = fixed_queue_get_list(p_data->apiwrite.frag_q); - const list_node_t *node = list_begin(list); - if (node != list_end(list)) { - p_frag = (BT_HDR *)list_node(node); - node = list_next(node); - - /* get first packet */ - /* posit on Adaptation Layer header */ - p_frag->len += AVDT_AL_HDR_SIZE + AVDT_MEDIA_HDR_SIZE; - p_frag->offset -= AVDT_AL_HDR_SIZE + AVDT_MEDIA_HDR_SIZE; - p = (UINT8 *)(p_frag + 1) + p_frag->offset; - - /* Adaptation Layer header */ - /* TSID, no-fragment bit and coding of length (in 2 length octets - * following) - */ - *p++ = (p_scb->curr_cfg.mux_tsid_media<<3) | AVDT_ALH_LCODE_16BIT; - - /* length of all remaining transport packet */ - UINT16_TO_BE_STREAM(p, p_frag->layer_specific + AVDT_MEDIA_HDR_SIZE ); - /* media header */ - UINT8_TO_BE_STREAM(p, AVDT_MEDIA_OCTET1); - UINT8_TO_BE_STREAM(p, p_data->apiwrite.m_pt); - UINT16_TO_BE_STREAM(p, p_scb->media_seq); - UINT32_TO_BE_STREAM(p, p_data->apiwrite.time_stamp); - UINT32_TO_BE_STREAM(p, ssrc); - p_scb->media_seq++; - } + if (! fixed_queue_is_empty(p_data->apiwrite.frag_q)) { + list_t *list = fixed_queue_get_list(p_data->apiwrite.frag_q); + const list_node_t *node = list_begin(list); + if (node != list_end(list)) { + p_frag = (BT_HDR *)list_node(node); + node = list_next(node); + + /* get first packet */ + /* posit on Adaptation Layer header */ + p_frag->len += AVDT_AL_HDR_SIZE + AVDT_MEDIA_HDR_SIZE; + p_frag->offset -= AVDT_AL_HDR_SIZE + AVDT_MEDIA_HDR_SIZE; + p = (UINT8 *)(p_frag + 1) + p_frag->offset; + + /* Adaptation Layer header */ + /* TSID, no-fragment bit and coding of length (in 2 length octets + * following) + */ + *p++ = (p_scb->curr_cfg.mux_tsid_media<<3) | AVDT_ALH_LCODE_16BIT; + + /* length of all remaining transport packet */ + UINT16_TO_BE_STREAM(p, p_frag->layer_specific + AVDT_MEDIA_HDR_SIZE ); + /* media header */ + UINT8_TO_BE_STREAM(p, AVDT_MEDIA_OCTET1); + UINT8_TO_BE_STREAM(p, p_data->apiwrite.m_pt); + UINT16_TO_BE_STREAM(p, p_scb->media_seq); + UINT32_TO_BE_STREAM(p, p_data->apiwrite.time_stamp); + UINT32_TO_BE_STREAM(p, ssrc); + p_scb->media_seq++; + } - for ( ; node != list_end(list); node = list_next(node)) { - p_frag = (BT_HDR *)list_node(node); - - /* posit on Adaptation Layer header */ - p_frag->len += AVDT_AL_HDR_SIZE; - p_frag->offset -= AVDT_AL_HDR_SIZE; - p = (UINT8 *)(p_frag + 1) + p_frag->offset; - /* Adaptation Layer header */ - /* TSID, fragment bit and coding of length (in 2 length octets - * following) - */ - *p++ = (p_scb->curr_cfg.mux_tsid_media << 3) | - (AVDT_ALH_FRAG_MASK | AVDT_ALH_LCODE_16BIT); - - /* length of all remaining transport packet */ - UINT16_TO_BE_STREAM(p, p_frag->layer_specific); + for ( ; node != list_end(list); node = list_next(node)) { + p_frag = (BT_HDR *)list_node(node); + + /* posit on Adaptation Layer header */ + p_frag->len += AVDT_AL_HDR_SIZE; + p_frag->offset -= AVDT_AL_HDR_SIZE; + p = (UINT8 *)(p_frag + 1) + p_frag->offset; + /* Adaptation Layer header */ + /* TSID, fragment bit and coding of length (in 2 length octets + * following) + */ + *p++ = (p_scb->curr_cfg.mux_tsid_media << 3) | + (AVDT_ALH_FRAG_MASK | AVDT_ALH_LCODE_16BIT); + + /* length of all remaining transport packet */ + UINT16_TO_BE_STREAM(p, p_frag->layer_specific); + } } /* store it */ diff --git a/stack/btm/btm_sec.c b/stack/btm/btm_sec.c index fa29465f5..a31d60353 100644 --- a/stack/btm/btm_sec.c +++ b/stack/btm/btm_sec.c @@ -6231,8 +6231,10 @@ static BOOLEAN btm_sec_is_serv_level0(UINT16 psm) static void btm_sec_check_pending_enc_req (tBTM_SEC_DEV_REC *p_dev_rec, tBT_TRANSPORT transport, UINT8 encr_enable) { - UINT8 res = encr_enable ? BTM_SUCCESS : BTM_ERR_PROCESSING; + if (fixed_queue_is_empty(btm_cb.sec_pending_q)) + return; + UINT8 res = encr_enable ? BTM_SUCCESS : BTM_ERR_PROCESSING; list_t *list = fixed_queue_get_list(btm_cb.sec_pending_q); for (const list_node_t *node = list_begin(list); node != list_end(list); ) { tBTM_SEC_QUEUE_ENTRY *p_e = (tBTM_SEC_QUEUE_ENTRY *)list_node(node); diff --git a/stack/gatt/gatt_sr.c b/stack/gatt/gatt_sr.c index b44d82d0c..9be12aa9d 100644 --- a/stack/gatt/gatt_sr.c +++ b/stack/gatt/gatt_sr.c @@ -166,18 +166,22 @@ static BOOLEAN process_read_multi_rsp (tGATT_SR_CMD *p_cmd, tGATT_STATUS status, p_buf->len = 1; /* Now walk through the buffers puting the data into the response in order */ - list_t *list = fixed_queue_get_list(p_cmd->multi_rsp_q); - const list_node_t *node; + list_t *list = NULL; + const list_node_t *node = NULL; + if (! fixed_queue_is_empty(p_cmd->multi_rsp_q)) + list = fixed_queue_get_list(p_cmd->multi_rsp_q); for (ii = 0; ii < p_cmd->multi_req.num_handles; ii++) { tGATTS_RSP *p_rsp = NULL; - if (ii == 0) - node = list_begin(list); - else - node = list_next(node); - if (node != list_end(list)) - p_rsp = (tGATTS_RSP *)list_node(node); + if (list != NULL) { + if (ii == 0) + node = list_begin(list); + else + node = list_next(node); + if (node != list_end(list)) + p_rsp = (tGATTS_RSP *)list_node(node); + } if (p_rsp != NULL) { diff --git a/stack/gatt/gatt_utils.c b/stack/gatt/gatt_utils.c index b73b6cea3..fb09704cb 100644 --- a/stack/gatt/gatt_utils.c +++ b/stack/gatt/gatt_utils.c @@ -167,6 +167,9 @@ void gatt_set_srv_chg(void) { GATT_TRACE_DEBUG ("gatt_set_srv_chg"); + if (fixed_queue_is_empty(gatt_cb.srv_chg_clt_q)) + return; + list_t *list = fixed_queue_get_list(gatt_cb.srv_chg_clt_q); for (const list_node_t *node = list_begin(list); node != list_end(list); node = list_next(node)) { @@ -198,6 +201,9 @@ tGATTS_PENDING_NEW_SRV_START *gatt_sr_is_new_srv_chg(tBT_UUID *p_app_uuid128, tB { tGATTS_PENDING_NEW_SRV_START *p_buf = NULL; + if (fixed_queue_is_empty(gatt_cb.pending_new_srv_start_q)) + return NULL; + list_t *list = fixed_queue_get_list(gatt_cb.pending_new_srv_start_q); for (const list_node_t *node = list_begin(list); node != list_end(list); node = list_next(node)) { @@ -771,7 +777,7 @@ BOOLEAN gatt_is_srv_chg_ind_pending (tGATT_TCB *p_tcb) { srv_chg_ind_pending = TRUE; } - else + else if (! fixed_queue_is_empty(p_tcb->pending_ind_q)) { list_t *list = fixed_queue_get_list(p_tcb->pending_ind_q); for (const list_node_t *node = list_begin(list); @@ -807,6 +813,9 @@ tGATTS_SRV_CHG *gatt_is_bda_in_the_srv_chg_clt_list (BD_ADDR bda) GATT_TRACE_DEBUG("gatt_is_bda_in_the_srv_chg_clt_list :%02x-%02x-%02x-%02x-%02x-%02x", bda[0], bda[1], bda[2], bda[3], bda[4], bda[5]); + if (fixed_queue_is_empty(gatt_cb.srv_chg_clt_q)) + return NULL; + list_t *list = fixed_queue_get_list(gatt_cb.srv_chg_clt_q); for (const list_node_t *node = list_begin(list); node != list_end(list); node = list_next(node)) { diff --git a/stack/l2cap/l2c_fcr.c b/stack/l2cap/l2c_fcr.c index 6554d9ae8..d6cfcec6f 100644 --- a/stack/l2cap/l2c_fcr.c +++ b/stack/l2cap/l2c_fcr.c @@ -1511,25 +1511,31 @@ static BOOLEAN retransmit_i_frames (tL2C_CCB *p_ccb, UINT8 tx_seq) } /* tx_seq indicates whether to retransmit a specific sequence or all (if == L2C_FCR_RETX_ALL_PKTS) */ - list_t *list_ack = fixed_queue_get_list(p_ccb->fcrb.waiting_for_ack_q); - const list_node_t *node_ack = list_begin(list_ack); + list_t *list_ack = NULL; + const list_node_t *node_ack = NULL; + if (! fixed_queue_is_empty(p_ccb->fcrb.waiting_for_ack_q)) { + list_ack = fixed_queue_get_list(p_ccb->fcrb.waiting_for_ack_q); + node_ack = list_begin(list_ack); + } if (tx_seq != L2C_FCR_RETX_ALL_PKTS) { /* If sending only one, the sequence number tells us which one. Look for it. */ - for ( ; node_ack != list_end(list_ack); node_ack = list_next(node_ack)) { - p_buf = (BT_HDR *)list_node(node_ack); - /* Get the old control word */ - p = ((UINT8 *) (p_buf+1)) + p_buf->offset + L2CAP_PKT_OVERHEAD; + if (list_ack != NULL) { + for ( ; node_ack != list_end(list_ack); node_ack = list_next(node_ack)) { + p_buf = (BT_HDR *)list_node(node_ack); + /* Get the old control word */ + p = ((UINT8 *) (p_buf+1)) + p_buf->offset + L2CAP_PKT_OVERHEAD; - STREAM_TO_UINT16 (ctrl_word, p); + STREAM_TO_UINT16 (ctrl_word, p); - buf_seq = (ctrl_word & L2CAP_FCR_TX_SEQ_BITS) >> L2CAP_FCR_TX_SEQ_BITS_SHIFT; + buf_seq = (ctrl_word & L2CAP_FCR_TX_SEQ_BITS) >> L2CAP_FCR_TX_SEQ_BITS_SHIFT; - L2CAP_TRACE_DEBUG ("retransmit_i_frames() cur seq: %u looking for: %u", buf_seq, tx_seq); + L2CAP_TRACE_DEBUG ("retransmit_i_frames() cur seq: %u looking for: %u", buf_seq, tx_seq); - if (tx_seq == buf_seq) - break; + if (tx_seq == buf_seq) + break; + } } if (!p_buf) @@ -1560,24 +1566,27 @@ static BOOLEAN retransmit_i_frames (tL2C_CCB *p_ccb, UINT8 tx_seq) while (!fixed_queue_is_empty(p_ccb->fcrb.retrans_q)) osi_freebuf(fixed_queue_try_dequeue(p_ccb->fcrb.retrans_q)); - node_ack = list_begin(list_ack); + if (list_ack != NULL) + node_ack = list_begin(list_ack); } - while (node_ack != list_end(list_ack)) - { - p_buf = (BT_HDR *)list_node(node_ack); - node_ack = list_next(node_ack); - - BT_HDR *p_buf2 = l2c_fcr_clone_buf(p_buf, p_buf->offset, p_buf->len); - if (p_buf2) + if (list_ack != NULL) { + while (node_ack != list_end(list_ack)) { - p_buf2->layer_specific = p_buf->layer_specific; + p_buf = (BT_HDR *)list_node(node_ack); + node_ack = list_next(node_ack); - fixed_queue_enqueue(p_ccb->fcrb.retrans_q, p_buf2); - } + BT_HDR *p_buf2 = l2c_fcr_clone_buf(p_buf, p_buf->offset, p_buf->len); + if (p_buf2) + { + p_buf2->layer_specific = p_buf->layer_specific; + + fixed_queue_enqueue(p_ccb->fcrb.retrans_q, p_buf2); + } - if ( (tx_seq != L2C_FCR_RETX_ALL_PKTS) || (p_buf2 == NULL) ) - break; + if ( (tx_seq != L2C_FCR_RETX_ALL_PKTS) || (p_buf2 == NULL) ) + break; + } } l2c_link_check_send_pkts (p_ccb->p_lcb, NULL, NULL); @@ -2282,31 +2291,35 @@ static void l2c_fcr_collect_ack_delay (tL2C_CCB *p_ccb, UINT8 num_bufs_acked) p_ccb->fcrb.ack_q_count_min[index] = fixed_queue_length(p_ccb->fcrb.waiting_for_ack_q); /* update sum, max and min of round trip delay of acking */ - list_t *list = fixed_queue_get_list(p_ccb->fcrb.waiting_for_ack_q); - for (const list_node_t *node = list_begin(list), xx = 0; - (node != list_end(list)) && (xx < num_bufs_acked); - node = list_next(node), xx++) { - p_buf = list_node(node); - /* adding up length of acked I-frames to get throughput */ - p_ccb->fcrb.throughput[index] += p_buf->len - 8; - - if ( xx == num_bufs_acked - 1 ) - { - /* get timestamp from tx I-frame that receiver is acking */ - p = ((UINT8 *) (p_buf+1)) + p_buf->offset + p_buf->len; - if (p_ccb->bypass_fcs != L2CAP_BYPASS_FCS) + list_t *list = NULL; + if (! fixed_queue_is_empty(p_ccb->fcrb.waiting_for_ack_q)) + list = fixed_queue_get_list(p_ccb->fcrb.waiting_for_ack_q); + if (list != NULL) { + for (const list_node_t *node = list_begin(list), xx = 0; + (node != list_end(list)) && (xx < num_bufs_acked); + node = list_next(node), xx++) { + p_buf = list_node(node); + /* adding up length of acked I-frames to get throughput */ + p_ccb->fcrb.throughput[index] += p_buf->len - 8; + + if ( xx == num_bufs_acked - 1 ) { - p += L2CAP_FCS_LEN; - } + /* get timestamp from tx I-frame that receiver is acking */ + p = ((UINT8 *) (p_buf+1)) + p_buf->offset + p_buf->len; + if (p_ccb->bypass_fcs != L2CAP_BYPASS_FCS) + { + p += L2CAP_FCS_LEN; + } - STREAM_TO_UINT32(timestamp, p); - delay = time_get_os_boottime_ms() - timestamp; + STREAM_TO_UINT32(timestamp, p); + delay = time_get_os_boottime_ms() - timestamp; - p_ccb->fcrb.ack_delay_avg[index] += delay; - if ( delay > p_ccb->fcrb.ack_delay_max[index] ) - p_ccb->fcrb.ack_delay_max[index] = delay; - if ( delay < p_ccb->fcrb.ack_delay_min[index] ) - p_ccb->fcrb.ack_delay_min[index] = delay; + p_ccb->fcrb.ack_delay_avg[index] += delay; + if ( delay > p_ccb->fcrb.ack_delay_max[index] ) + p_ccb->fcrb.ack_delay_max[index] = delay; + if ( delay < p_ccb->fcrb.ack_delay_min[index] ) + p_ccb->fcrb.ack_delay_min[index] = delay; + } } } -- 2.11.0