Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 18, 2015, 1:29 p.m.) Status -- This change has been discarded. Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15049 --- This is still a nuisance warning that doesn't add much value for the effort. - rmudgett On April 3, 2015, 4:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 4:24 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25728 This change causes bugs. It is supposed to return peer because it increased the ref. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25729 This change is another bug. It is supposed to return the found user. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25730 In this case we have a ref before calling AST_SCHED_DEL and the scheduler item that we just deleted also has a ref. Therefore peer could never be invalid. The scheduler is very old with a flawed API. It doesn't know how to handle callback data that must be released such as ao2 object refs or malloced data. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25732 This is another case of dealing with the scheduler. We are releasing the ref we just tried to give the scheduler that failed to be added. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25733 This NULL check is not needed. peer can never be NULL here without an earlier crash. This function is a scheduler callback. - rmudgett On April 3, 2015, 4:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 4:24 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 11:24 p.m.) Review request for Asterisk Developers. Changes --- Clarify Purpose of this Review entry Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing (updated) --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 11:36 p.m., rmudgett wrote: This is still a nuisance warning that doesn't add much value for the effort. Diederik de Groot wrote: We can drop it no problem. I still think it could be useful in detecting _ref/_unref issues, alas it would quite a bit of work but could be beneficial. If there is consensus on the asterisk-developer side, i will make the required Makefile change to suppress this waning. I would like to make sure that you agree to drop the -Wunused-value warning, before i do so. So please report back. By the way did you check out this 'Posted 5 days, 21 hours ago (March 29, 2015, 1:49 a.m.)' entry on this page. I tried to explain how this warning could be benifial in finding these used after ..._unref issues. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15049 --- On April 3, 2015, 11:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 11:24 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2009-2013 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2009 This change causes bugs. It is supposed to return peer because it increased the ref. Stupid me. If should actually also check if ao2_ref did not return an error (-1). On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 11131-11136 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line11131 This is another case of dealing with the scheduler. We are releasing the ref we just tried to give the scheduler that failed to be added. made it a conscious choice choice to discard the pointer using (void)peer_unref(peer); Maybe time to update Scheduler :-) On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2021-2025 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2021 This change is another bug. It is supposed to return the found user. Guess i should not be doing any of this stuff at 3 Am i guess. Difficult to make a point, whilst making mistakes like these. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- On April 4, 2015, 1:15 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs (updated) - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 4:36 p.m., rmudgett wrote: This is still a nuisance warning that doesn't add much value for the effort. Diederik de Groot wrote: We can drop it no problem. I still think it could be useful in detecting _ref/_unref issues, alas it would quite a bit of work but could be beneficial. If there is consensus on the asterisk-developer side, i will make the required Makefile change to suppress this waning. I would like to make sure that you agree to drop the -Wunused-value warning, before i do so. So please report back. Diederik de Groot wrote: By the way did you check out this 'Posted 5 days, 21 hours ago (March 29, 2015, 1:49 a.m.)' entry on this page. I tried to explain how this warning could be benifial in finding these used after ..._unref issues. Those findings were not real bugs. See my review of those specific findings. - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15049 --- On April 3, 2015, 4:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 4:24 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2009-2013 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2009 This change causes bugs. It is supposed to return peer because it increased the ref. Diederik de Groot wrote: Stupid me. If should actually also check if ao2_ref did not return an error (-1). rmudgett wrote: The return value of ao2_ref is rarely used. When it is it is to get the number of refs the object had before the call. The -1 error return value is practically useless. That value means a programming error because the object is invalid and you get bad magic number messages logged, assertion failures, and very likely crashes. Ok, point taken. So that would mean that ao2_ref will be the biggest cause of -Wunused-value warning, even if we changed everything else, because it's return is rarely used :-) I still think it is valid to NULL a pointer that might/might not vanish in the future (as with ao2_ref(xx, -1)). Same goes for example for ast_free, which does not null the pointer afterwards (or did i overlook something ?. It makes NULL pointer checks after such a free or unref a little bit pointless. I know everything is working and asterisk-11 and asterisk-13 are pretty stable as far as i can tell, so not looking for extra work for any of us, just want to finish this point and move on. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- On April 4, 2015, 1:15 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 5:40 p.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2009-2013 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2009 This change causes bugs. It is supposed to return peer because it increased the ref. Diederik de Groot wrote: Stupid me. If should actually also check if ao2_ref did not return an error (-1). The return value of ao2_ref is rarely used. When it is it is to get the number of refs the object had before the call. The -1 error return value is practically useless. That value means a programming error because the object is invalid and you get bad magic number messages logged, assertion failures, and very likely crashes. - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- On April 3, 2015, 6:15 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 6:15 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 11:36 p.m., rmudgett wrote: This is still a nuisance warning that doesn't add much value for the effort. Diederik de Groot wrote: We can drop it no problem. I still think it could be useful in detecting _ref/_unref issues, alas it would quite a bit of work but could be beneficial. If there is consensus on the asterisk-developer side, i will make the required Makefile change to suppress this waning. I would like to make sure that you agree to drop the -Wunused-value warning, before i do so. So please report back. Diederik de Groot wrote: By the way did you check out this 'Posted 5 days, 21 hours ago (March 29, 2015, 1:49 a.m.)' entry on this page. I tried to explain how this warning could be benifial in finding these used after ..._unref issues. rmudgett wrote: Those findings were not real bugs. See my review of those specific findings. Agreed ! So if anybody else thinks we should drop this warning let me know, and we'll drop it. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15049 --- On April 4, 2015, 1:15 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 1, 2015, 12:30 a.m., rmudgett wrote: I dislike this warning. It is more nuisance than helpful. I once worked with compiler that had a similar warning. The lengths you sometimes have to go to eradicate the the warning can be extream. There are several places in this patch that change code behavior and introduce bugs. Hi rmudgett, I agree this would be the biggest amount of work of the whole 'clang' project. It does not really matter from a performance standpoint i think. It can be helpfull to find refcount issues where a previously used refcounted pointer was unreffed and might vanish at any moment. If we where to always require that you have to use the return value from any function including XXX_unref, then we would be setting that that pointer to NULL and therefor the static compiler would be able to detect the null pointer dereference for any use after this location. There are other ways to deal with this, for example adding __attribute(context)__ and using sparse to find these (like the linux-kernel does) instead of depending on this little warning. Is it worth the work ? I don't know. I leave that decision up to you guys. You might want to have a look at: http://asterisk.chan-sccp.net:443/scanbuild-output/2015-03-29-155615-16015-1/ To see how usefull this static compiler can really be (Don't mind the Cast from non-struct type to struct type for now). FYI : I know my patch/fix was certainly not complete nor quite sure not bug free, it was more a sample case of how this could be handled, sorry that that was not clear from the patch description. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14999 --- On March 29, 2015, 1:44 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 1:44 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14999 --- I dislike this warning. It is more nuisance than helpful. I once worked with compiler that had a similar warning. The lengths you sometimes have to go to eradicate the the warning can be extream. There are several places in this patch that change code behavior and introduce bugs. - rmudgett On March 28, 2015, 7:44 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 28, 2015, 7:44 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 12:46 a.m.) Review request for Asterisk Developers. Changes --- ./configure --enable-dev-mode CC=clang CXX=clang++ ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 make.report Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments (updated) ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On March 29, 2015, 1:49 a.m., Diederik de Groot wrote: The fixes for unused-value to chan_iax2, to compile smoothly should be seen as a showcase that this can be off potential benefit. Would have been nice if ao2_ref(..., -1) would have behaved in the same way, so that ao2_ref(..., -1) would return NULL, maybe this could be a future enhancement. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14952 --- On March 29, 2015, 1:44 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 1:44 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 1:44 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs (updated) - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14952 --- /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25567 If this is the last peer, then releasing it here would mean that peer-pokeexpire would cause a segfault. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25569 using peer = peer_unref(peer), will cause the segfault in the lines below everytime, which should be considered a good thing. Will show use of a released pointer during testing. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25568 potential segfault because of released peer /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25570 function is being called with (peer = 0x0), which showed up after messing with the code above. Either this is a real issue, or caused by fiddling with the code. - Diederik de Groot On March 29, 2015, 1:44 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 1:44 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 28, 2015, 8:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description (updated) --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14951 --- Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Will wait for you guys to respond, before continuing. Will needs some more people to finish the '-Wunused-value' task, would be way to much for one person. - Diederik de Groot On March 28, 2015, 8:32 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 28, 2015, 8:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:-Wunused-value Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
