[Asterisk-code-review] res http websocket.c: prevent avoidable disconnections cause... (asterisk[11])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: res_http_websocket.c: prevent avoidable disconnections caused by write errors
......................................................................


Patch Set 1: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/1863/1/res/res_http_websocket.c
File res/res_http_websocket.c:

Line 249: 	frame_size = header_size+payload_size;
Nitpick - coding guidelines mandate spaces around operators:

frame_size = header_size + payload_size;


Line 251: 	frame = ast_alloca(frame_size+1);
        : 	memset(frame, 0, frame_size+1);
Operator spacing here as well.


Line 257: 
I would remove this extra blank line. We generally use a single line of vertical space to separate code, not two.


Line 275: 		ast_debug(0, "Closing WS session with 1011 because there's no session->f\n");
I wouldn't use ast_debug(0). That causes the statement to be evaluated even when debug is not enabled, which uses more cycles than necessary.

I would use ast_debug(1, ... ) here.


Line 273: 	if (!session->f) {
        : 		ao2_unlock(session);
        : 		ast_debug(0, "Closing WS session with 1011 because there's no session->f\n");
        : 		ast_websocket_close(session, 1011);
        : 		return -1;
        : 	}
This call to ast_websocket_close will potentially cause us to pass a NULL FILE * to ast_careful_fwrite and/or fclose. ast_websocket_close assumes that we still have the FILE stream available.

I would either not call ast_websocket_close here, or else modify ast_websocket_close to be tolerant to having a NULL FILE *.


-- 
To view, visit https://gerrit.asterisk.org/1863
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4db7a586af1c7a57184c31d3d55bf146f1a40598
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Dade Brandon <dade at xencall.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Dade Brandon <dade at xencall.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes