Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sd-hwdb: fix matching for characters with an ord > 127 #11672

Merged
merged 1 commit into from Feb 13, 2019

Conversation

5 participants
@whot
Copy link
Contributor

commented Feb 8, 2019

Devices like the "Microsoft Microsoft® 2.4GHz Transceiver v9.0 Mouse" contain
characters higher than 127. That ® is correctly stored in the hwdb and passed
into the search field during query, but the comparison fails.

Our search string is a const char *, trie_string() returns a const char * but
the current character is cast to uint8_t. This causes anything over 127 to
fail the match. Fix this, we're dealing with characters everywhere here after
all.

@whot whot added the sd-hwdb label Feb 8, 2019

@yuwata yuwata added this to the v241 milestone Feb 8, 2019

@keszybz

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Can we please add some test for this? The change charuint8_t looks like something very small to have such a large impact.

@keszybz

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

======================================================================
ERROR: test_resolved_domain_restricted_dns (__main__.DnsmasqClientTest)
resolved: domain-restricted DNS servers
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/autopkgtest.ZBHnG7/build.TAy/systemd/test/networkd-test.py", line 619, in test_resolved_domain_restricted_dns
    subprocess.check_call(['systemctl', 'restart', 'systemd-resolved'])
  File "/usr/lib/python3.6/subprocess.py", line 291, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['systemctl', 'restart', 'systemd-resolved']' returned non-zero exit status 1.

@xnox ?

@keszybz

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

A start job is running for Testsuite service (2h 15min 53s / no limit)
...
[�[0;32m  OK  �[0m] Started �[0;1;39mFail on restart�[0m.
autopkgtest [02:44:42]: ERROR: timed out on command "su -s /bin/bash root -c set -e; export USER=`id -nu`; . /etc/profile >/dev/null 2>&1 || true;  . ~/.profile >/dev/null 2>&1 || true; buildtree="/tmp/autopkgtest.Z39r1r/build.QBU/systemd"; mkdir -p -m 1777 -- "/tmp/autopkgtest.Z39r1r/upstream-artifacts"; export AUTOPKGTEST_ARTIFACTS="/tmp/autopkgtest.Z39r1r/upstream-artifacts"; export ADT_ARTIFACTS="$AUTOPKGTEST_ARTIFACTS"; mkdir -p -m 755 "/tmp/autopkgtest.Z39r1r/autopkgtest_tmp"; export AUTOPKGTEST_TMP="/tmp/autopkgtest.Z39r1r/autopkgtest_tmp"; export ADTTMP="$AUTOPKGTEST_TMP"; export DEBIAN_FRONTEND=noninteractive; export LANG=C.UTF-8; export DEB_BUILD_OPTIONS=parallel=1; unset LANGUAGE LC_CTYPE LC_NUMERIC LC_TIME LC_COLLATE   LC_MONETARY LC_MESSAGES LC_PAPER LC_NAME LC_ADDRESS   LC_TELEPHONE LC_MEASUREMENT LC_IDENTIFICATION LC_ALL;rm -f /tmp/autopkgtest_script_pid; set -C; echo $$ > /tmp/autopkgtest_script_pid; set +C; trap "rm -f /tmp/autopkgtest_script_pid" EXIT INT QUIT PIPE; cd "$buildtree"; export AUTOPKGTEST_NORMAL_USER=ubuntu; export ADT_NORMAL_USER=ubuntu; export 'CFLAGS=-O0'; export 'DEB_BUILD_PROFILES=noudeb'; export 'TEST_UPSTREAM=1'; export 'UPSTREAM_PULL_REQUEST=11671'; export 'GITHUB_STATUSES_URL=https://api.github.com/repos/systemd/systemd/statuses/94b2f235de8e6b2ef737d99f2c0b11739fda52ae'; chmod +x /tmp/autopkgtest.Z39r1r/build.QBU/systemd/debian/tests/upstream; touch /tmp/autopkgtest.Z39r1r/upstream-stdout /tmp/autopkgtest.Z39r1r/upstream-stderr; /tmp/autopkgtest.Z39r1r/build.QBU/systemd/debian/tests/upstream 2> >(tee -a /tmp/autopkgtest.Z39r1r/upstream-stderr >&2) > >(tee -a /tmp/autopkgtest.Z39r1r/upstream-stdout);" (kind: test)
autopkgtest [02:44:42]: test upstream: -----------------------]
autopkgtest [02:44:43]: test upstream:  - - - - - - - - - - results - - - - - - - - - -
upstream             FAIL timed out

2h is a lot of time...

@xnox

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2019

networkd-test.py     FAIL non-zero exit status 1
root-unittests       FAIL non-zero exit status 1
upstream             FAIL non-zero exit status 1

Note that root-unittests & upstream is the same, as it's the TEST-24-UNIT-TESTS that is failing in "upstream" tests.

networkd-test.py failing is weird.

@xnox

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2019

On i386, TEST-11-ISSUE-3166 appears to hang the upstream tests.

@whot

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Can we please add some test for this?

I'll need a hint on where to start here. I can write a script that runs systemd-hwdb --root=/some/test/dir update and get the hwdb in that path but I cannot then systemd-hwdb query the resulting hwdb. So the only thing tests can run are system hwdb files which presumably we don't want to update during the test runs.

@poettering

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@whot hmm, maybe try to rebase and repush? it could be just that the CI is flaky.

@whot whot force-pushed the whot:wip/hwdb-matching-fix branch from 96c4761 to 4adea57 Feb 11, 2019

sd-hwdb: fix matching for characters with an ord > 127
Devices like the "Microsoft Microsoft® 2.4GHz Transceiver v9.0 Mouse" contain
characters higher than 127. That ® is correctly stored in the hwdb and passed
into the search field during query, but the comparison fails.

Our search string is a const char *, trie_string() returns a const char * but
the current character is cast to uint8_t. This causes anything over 127 to
fail the match. Fix this, we're dealing with characters everywhere here after
all.

@whot whot force-pushed the whot:wip/hwdb-matching-fix branch from 4adea57 to a361b7c Feb 11, 2019

@whot

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

re-pushed, test suite passes now. Still have no idea how to write a test case for this bit though :)

@keszybz

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

bionic-amd64 has UPSTREAM_PULL_REQUEST=11672, bionic-i386 has UPSTREAM_PULL_REQUEST=11690, and bionic-s390x has UPSTREAM_PULL_REQUEST=11591. I'll restart the tests.

@keszybz

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Once the tests pass (FWIW), I think we should merge this, and think about adding the tests later.

@keszybz keszybz merged commit dc4b6f8 into systemd:master Feb 13, 2019

10 checks passed

CentOS CI Build finished.
Details
CentOS CI (Vagrant) Build finished.
Details
LGTM analysis: C/C++ No alert changes
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
bionic-amd64 autopkgtest finished (success)
Details
bionic-i386 autopkgtest finished (success)
Details
bionic-s390x autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.