fix: spd_set_notification must not always send NOTIFICATION ALL - #1116
Merged
Conversation
NOTIFICATION_SET tests a single bit, but SPD_ALL is 0x3f, the union of the six event bits (include/speechd_types.h:76-83). So "SET SELF NOTIFICATION ALL" was emitted for any non-zero argument, and last, and set_notification_self() (src/server/set.c:502-508) applies "all" to all six events. spd_set_notification_on(conn, SPD_END) therefore enabled every event, and spd_set_notification_off(conn, SPD_BEGIN) disabled every event, including the END and CANCEL a client may be waiting on. Test every bit of val instead of any. For the six single-bit values the two tests are equivalent, so only the SPD_ALL line changes behaviour.
Collaborator
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTIFICATION_SETatsrc/api/c/libspeechd.c:1429-1434tests one bit (notification & val), but the last of the seven uses at:1463passesSPD_ALL, whichinclude/speechd_types.h:83defines as0x3f— the union of the six event bits, not a bit of its own. SoSET SELF NOTIFICATION ALLis emitted for any non-zero argument, and it is emitted last, andset_notification_self(src/server/set.c:502-508) appliesallto all six events. Thenotificationparameter has been a no-op sinced6613243(2010-09-27) added that line;SPD_ALLwas already0x3fat that commit.Measured by compiling the unmodified
src/api/c/libspeechd.cand driving the realspd_set_notification_on/offover a unix socket against a stub SSIP server that modelsset_notification_self. Same harness, same commands, before and after:spd_set_notification_on(conn, SPD_BEGIN)0x3fall six0x01beginspd_set_notification_on(conn, SPD_END)(whatspd-sayasks for)0x3f0x02endspd_set_notification_on(conn, SPD_END|SPD_CANCEL)(the manual's example)0x3f0x0aend, cancelspd_set_notification_on(conn, SPD_ALL)0x3f0x3funchangedspd_set_notification_off(conn, SPD_BEGIN)0x00all six off0x3ebegin offThe last row is the one that bites: a client that turns off a single event loses END and CANCEL too, and the pattern the manual documents at
doc/speech-dispatcher.texi:2380-2400—spd_set_notification_on(conn, SPD_END); spd_set_notification_on(conn, SPD_CANCEL);thensem_waitfor one of them — has nothing left to wake it.Authority:
doc/speech-dispatcher.texidocumentsSPDNotificationas six members with noSPD_ALL, says these functions "set the notification specified by the parameternotification", and notes|combinations are allowed.doc/ssip.texi:1377documentsSET SELF NOTIFICATION ALLas setting all event notifications, so emitting it is never a no-op. The Python binding does the same job correctly —src/api/python/speechd/client.py:613-619sends the six names individually and never sendsall.Why nothing caught it:
src/tests/spd_set_notifications_all.c:77is the only test of this API and it passesSPD_ALL, the single input for which line 1463 is harmless.src/tests/clibrary2.c:95-96usesSPD_END/SPD_CANCELbut never asserts that the other events stayed off, and nothing callsspd_set_notification_off.The fix tests every bit of
valrather than any. For the six single-bit values(n & val) == valandn & valare equivalent, soSPD_ALLis the only line whose behaviour changes — the control row above shows theSPD_ALLwire traffic is byte-for-byte what it is today.Mutants against the same harness: reverting the macro reproduces every row above; the naive
notification == valfixes the single-bit cases but makesSPD_END|SPD_CANCELemit nothing at all (0x00); simply deleting theNOTIFICATION_SET(SPD_ALL, "all")line gives identical results in every row, and I did not choose it only because it stops sendingALLat all, whereas this keeps today's exact wire behaviour forSPD_ALL. Happy to switch if you prefer the deletion.Not tested, stated rather than glossed: I could not build the project here. This is macOS;
./build.shsucceeds but./configurefails ondotconf >= 1.3, which Homebrew does not carry, somake checkand the autotest suite were never run and I have not exercised this against a livespeech-dispatcheror Orca. Everything above is the reallibspeechd.ccompiled standalone against glib, with the server side modelled fromset.crather than executed. No new test is included because the existing test for this API needs a running server; glad to add one tosrc/tests/if you want it, but I would not be able to verify it locally.This change was prepared with AI assistance (Claude Opus 5).