[PATCH btbridge v4 4/6] Initial set of test.
Andrew Jeffery
andrew at aj.id.au
Thu May 19 15:25:46 AEST 2016
Hey Cyril,
The main queries I had are near the bottom, regarding the system bus
and what alternatives we might have.
A few typos: 'test' in the subject should be 'tests'? Probably drop the
full-stop as well.
On Tue, 2016-05-03 at 20:10 -0500, OpenBMC Patches wrote:
> From: Cyril Bur <cyril.bur at au1.ibm.com>
> Very simple tests which can hopefully be extended in the future.
>
> The main purpose of this is to be able to use travis-ci to automatate
'automate'
> the
> running of the tests and being able to fake /dev/bt-host.
>
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
> Makefile | 7 +
> bt-host.c | 235 ++++++++++++++++++++++++++++++++++
> ipmi-bouncer.c | 131 +++++++++++++++++++
> travis/build.sh | 9 ++
> travis/org.openbmc.HostIpmi.conf.test | 20 +++
> travis/run_tests.sh | 15 +++
> 6 files changed, 417 insertions(+)
> create mode 100644 bt-host.c
> create mode 100644 ipmi-bouncer.c
> create mode 100644 travis/org.openbmc.HostIpmi.conf.test
> create mode 100755 travis/run_tests.sh
>
> diff --git a/Makefile b/Makefile
> index 7ffbc01..1cf1a21 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -9,5 +9,12 @@ EXE = btbridged
>
> all: $(EXE)
>
> +.PHONY += test
> +test: $(EXE) ipmi-bouncer bt-host
> +
> +bt-host: bt-host.c
> + gcc -shared -fPIC -ldl $(CFLAGS) $^ -o $@.so
> +
> clean:
> rm -rf *.o $(EXE)
> + rm -rf bt-host.so ipmi-bouncer
> diff --git a/bt-host.c b/bt-host.c
> new file mode 100644
> index 0000000..65bf6bb
> --- /dev/null
> +++ b/bt-host.c
> @@ -0,0 +1,235 @@
> +#define _GNU_SOURCE
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include /* See NOTES */
> +#include
> +
> +#include
> +
> +struct bttest_data {
> + int status;
> + const char msg[64];
> +};
> +
> +static int bt_host_fd;
> +static int timer_fd;
> +
> +static int stop;
> +static int sent_id = -1;
> +static int recv_id;
> +
> +/*
> + * btbridged doesn't care about the message EXCEPT the first byte must be
> + * correct.
> + * The first byte is the size not including the length byte its self.
> + * A len less than 4 will constitute an invalid message according to the BT
> + * protocol, btbridged will care.
> + */
> +static struct bttest_data data[] = {
> + /*
> + * Note, the 4th byte is cmd, the ipmi-bouncer will put cmd in cc so
> + * in this array always duplicate the command
> + *
> + * Make the first message look like:
> + * seq = 1, netfn = 2, lun = 3 and cmd= 4
> + * (thats how btbridged will print it)
> + */
> + { 0, { 4, 0xb, 1, 4, 4 }},
> + { 0, { 4, 0xff, 0xee, 0xdd, 0xdd, 0xbb }},
> + /*
> + * A bug was found in bt_q_drop(), write a test!
> + * Simply send the same seq number a bunch of times
> + */
> + { 0, { 4, 0xaa, 0xde, 0xaa, 0xaa }},
> + { 0, { 4, 0xab, 0xde, 0xab, 0xab }},
> + { 0, { 4, 0xac, 0xde, 0xac, 0xac }},
> + { 0, { 4, 0xad, 0xde, 0xad, 0xad }},
> + { 0, { 4, 0xae, 0xde, 0xae, 0xae }},
> + { 0, { 4, 0xaf, 0xde, 0xaf, 0xaf }},
> + { 0, { 4, 0xa0, 0xde, 0xa0, 0xa0 }},
> + { 0, { 4, 0xa1, 0xde, 0xa1, 0xa1 }},
> + { 0, { 4, 0xa2, 0xde, 0xa2, 0xa2 }},
> + { 0, { 4, 0xa3, 0xde, 0xa3, 0xa3 }},
> + { 0, { 4, 0xa4, 0xde, 0xa4, 0xa4 }},
> + { 0, { 4, 0xa5, 0xde, 0xa5, 0xa5 }},
> + { 0, { 4, 0xa6, 0xde, 0xa6, 0xa6 }},
> + { 0, { 4, 0xa7, 0xde, 0xa7, 0xa7 }},
> + { 0, { 4, 0xa8, 0xde, 0xa8, 0xa8 }},
> + { 0, { 4, 0xa9, 0xde, 0xa9, 0xa9 }},
> + { 0, { 4, 0xaa, 0x88, 0xaa, 0xaa }},
> + { 0, { 4, 0xab, 0x88, 0xab, 0xab }},
> + { 0, { 4, 0xac, 0x88, 0xac, 0xac }},
> + { 0, { 4, 0xad, 0x88, 0xad, 0xad }},
> + { 0, { 4, 0xae, 0x88, 0xae, 0xae }},
> + { 0, { 4, 0xaf, 0x88, 0xaf, 0xaf }},
> + { 0, { 4, 0xa0, 0x88, 0xa0, 0xa0 }},
> + { 0, { 4, 0xa1, 0x88, 0xa1, 0xa1 }},
> + { 0, { 4, 0xa2, 0x88, 0xa2, 0xa2 }},
> + { 0, { 4, 0xa3, 0x88, 0xa3, 0xa3 }},
> + { 0, { 4, 0xa4, 0x88, 0xa4, 0xa4 }},
> + { 0, { 4, 0xa5, 0x88, 0xa5, 0xa5 }},
> + { 0, { 4, 0xa6, 0x88, 0xa6, 0xa6 }},
> + { 0, { 4, 0xa7, 0x88, 0xa7, 0xa7 }},
> + { 0, { 4, 0xa8, 0x88, 0xa8, 0xa8 }},
> + { 0, { 4, 0xa9, 0x88, 0xa9, 0xa9 }},
> +};
> +#define BTTEST_NUM (sizeof(data)/sizeof(struct bttest_data))
> +#define PREFIX "[BTHOST] "
> +
> +#define MSG_OUT(f_, ...) do { printf(PREFIX); printf((f_), ##__VA_ARGS__); } while(0)
> +#define MSG_ERR(f_, ...) do { fprintf(stderr,PREFIX); fprintf(stderr, (f_), ##__VA_ARGS__); } while(0)
> +
> +typedef int (*orig_open_t)(const char *pathname, int flags);
> +typedef int (*orig_poll_t)(struct pollfd *fds, nfds_t nfds, int timeout);
> +typedef int (*orig_read_t)(int fd, void *buf, size_t count);
> +typedef ssize_t (*orig_write_t)(int fd, const void *buf, size_t count);
> +typedef int (*orig_ioctl_t)(int fd, unsigned long request, char *p);
> +typedef int (*orig_timerfd_create_t)(int clockid, int flags);
> +
> +int ioctl(int fd, unsigned long request, char *p)
> +{
> + if (fd == bt_host_fd) {
> + MSG_OUT("ioctl(%d, %lu, %p)\n", fd, request, p);
> + /* TODO Check the request number */
> + return 0;
> + }
> +
> + orig_ioctl_t orig_ioctl;
> + orig_ioctl = (orig_ioctl_t)dlsym(RTLD_NEXT, "ioctl");
> + return orig_ioctl(fd, request, p);
> +}
> +
> +int poll(struct pollfd *fds, nfds_t nfds, int timeout)
> +{
> + int i, j;
> + int ret = 0;
> + int dropped = 0;
> + struct pollfd *new_fds = calloc(nfds, sizeof(struct pollfd));
> + j = 0;
> + for (i = 0; i < nfds; i++) {
> + if (fds[i].fd == bt_host_fd) {
> + short revents = fds[i].events;
> +
> + MSG_OUT("poll() on bt_host fd\n");
> +
> + if (stop)
> + revents &= ~POLLIN;
> + if (sent_id == -1)
> + revents &= ~POLLOUT;
> + fds[i].revents = revents;
> + ret++;
> + dropped++;
> + } else if(fds[i].fd == timer_fd) {
> + MSG_OUT("poll() on timerfd fd, dropping request\n");
> +
> + fds[i].revents = 0;
> + dropped++;
> + } else {
> + new_fds[j].fd = fds[i].fd;
> + new_fds[j].events = fds[i].events;
> + /* Copy this to be sure */
> + new_fds[j].revents = fds[i].revents;
> + j++;
> + }
> + }
> + orig_poll_t orig_poll;
> + orig_poll = (orig_poll_t)dlsym(RTLD_NEXT, "poll");
> + ret += orig_poll(new_fds, nfds - dropped, timeout);
> + j = 0;
> + for (i = 0; i < nfds; i++) {
> + if (fds[i].fd != bt_host_fd && fds[i].fd != timer_fd) {
> + fds[i].fd = new_fds[j].fd;
> + fds[i].revents = new_fds[j].revents;
> + j++;
> + }
> + }
> + free(new_fds);
> + return ret;
> +}
> +
> +int open(const char *pathname, int flags)
> +{
> + if (strcmp("/dev/bt-host", pathname) == 0) {
> + MSG_OUT("open(%s, %x)\n", pathname, flags);
> + bt_host_fd = socket(AF_UNIX, SOCK_STREAM, 0);
> + return bt_host_fd;
> + }
> + orig_open_t orig_open;
> + orig_open = (orig_open_t)dlsym(RTLD_NEXT, "open");
> + return orig_open(pathname, flags);
> +}
> +
> +int read(int fd, void *buf, size_t count)
> +{
> + if (fd == bt_host_fd) {
> + MSG_OUT("read(%d, %p, %ld)\n", fd, buf, count);
> +
> + if (sent_id == -1)
> + sent_id = 0;
> + else
> + sent_id++;
Why are we treating sent_id == -1 as a special case?
> +
> + MSG_OUT("Send msg id %d\n", sent_id);
> +
> + if (count < data[sent_id].msg[0] + 1) {
> + /*
> + * TODO handle this, not urgent, the real driver also gets it
> + * wrong
> + */
> + MSG_ERR("Read size was too small\n");
> + errno = ENOMEM;
> + return -1;
> + }
> + if (sent_id == BTTEST_NUM - 1)
> + stop = 1;
It's a personal thing so I'm not bothered about changing it, but
conditionally assigning booleans always irks me. We could instead do:
stop = (sent_id == (BTTEST_NUM - 1));
> +
> + memcpy(buf, data[sent_id].msg, data[sent_id].msg[0] + 1);
> + return data[sent_id].msg[0] + 1;
It seems we compute 'data[sent_id].msg[0] + 1' several times. Might be
worth making a local variable of it?
> + }
> +
> + orig_read_t orig_read;
> + orig_read = (orig_read_t)dlsym(RTLD_NEXT, "read");
> + return orig_read(fd, buf, count);
> +}
> +
> +int write(int fd, const void *buf, size_t count)
> +{
> + if (fd == bt_host_fd) {
> + MSG_OUT("write(%d, %p, %ld)\n", fd, buf, count);
> + if (count == 5 && ((char *)buf)[4] == 0xce) {
> + MSG_ERR("CAUGHT A TIMEOUT!!!! 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x\n", ((char *)buf)[0], ((char *)buf)[1], ((char *)buf)[2], ((char *)buf)[3], ((char *)buf)[4]);
> + exit(1);
> + }
> + if (memcmp(buf + 1, data[recv_id].msg + 1, count - 2) != 0) {
> + int j;
> +
> + MSG_ERR("Bad response/inconsistent message index: %d\n", recv_id);
> + for (j = 0; j < count - 2; j++)
> + MSG_ERR("0x%02x vs 0x%02x\n", data[recv_id].msg[j + 1], ((char *)buf)[1 + j]);
> + } else {
> + MSG_OUT("Good response to message index: %d\n", recv_id);
> + data[recv_id].status = 2;
> + }
> + if (recv_id == BTTEST_NUM - 1) {
> + MSG_OUT("recieved a response to all messages, tentative success\n");
Typo: received
> + exit(0);
Is there a nicer way to do this than to exit the process from an
LD_PRELOAD library?
> + }
> + recv_id++;
> + return count;
> + }
> + orig_write_t orig_write;
> + orig_write = (orig_write_t)dlsym(RTLD_NEXT, "write");
> + return orig_write(fd, buf, count);
> +}
> +
> +int timerfd_create(int clockid, int flags)
> +{
> + orig_timerfd_create_t orig_timerfd_create;
> + orig_timerfd_create = (orig_timerfd_create_t)dlsym(RTLD_NEXT, "timerfd_create");
> + timer_fd = orig_timerfd_create(clockid, flags);
> + return timer_fd;
What is the reason for wrapping timerfd_create()?
> +}
Overall the wrapping seems like a lot of effort :/
> diff --git a/ipmi-bouncer.c b/ipmi-bouncer.c
> new file mode 100644
> index 0000000..030cffb
> --- /dev/null
> +++ b/ipmi-bouncer.c
> @@ -0,0 +1,131 @@
> +#include
> +#include
> +
> +#include
> +
> +#define PREFIX "[IPMI] "
> +
> +#define MSG_OUT(f_, ...) do { printf(PREFIX); printf((f_), ##__VA_ARGS__); } while(0)
> +#define MSG_ERR(f_, ...) do { fprintf(stderr,PREFIX); fprintf(stderr, (f_), ##__VA_ARGS__); } while(0)
> +
> +sd_bus *bus;
> +
> +static int bttest_ipmi(sd_bus_message *req,
> + void *user_data, sd_bus_error *ret_error)
> +{
> + sd_bus_error error = SD_BUS_ERROR_NULL;
> + sd_bus_message *reply = NULL, *m=NULL;
> + const char *dest, *path;
> + int r, pty;
> + unsigned char seq, netfn, lun, cmd;
> + uint8_t buf[1];
> +
> + MSG_OUT("Got DBUS message\n");
> +
> + r = sd_bus_message_read(req, "yyyy", &seq, &netfn, &lun, &cmd);
> + if (r < 0) {
> + MSG_ERR("FAIL ");
> + errno = -r;
> + perror("Couldn't read DBUS message");
> + return -1;
> + }
> +
> + dest = sd_bus_message_get_sender(req);
> + path = sd_bus_message_get_path(req);
> +
> + r = sd_bus_message_new_method_call(bus, &m, dest, path,
> + "org.openbmc.HostIpmi", "sendMessage");
> + if (r < 0) {
> + MSG_ERR("FAIL ");
> + errno = -r;
> + perror("Failed to add the method object");
> + return -1;
> + }
> +
> + /* Send CMD twice */
> + r = sd_bus_message_append(m, "yyyyy", seq, netfn, lun, cmd, cmd);
> + if (r < 0) {
> + MSG_ERR("FAIL ");
> + errno = -r;
> + perror("Failed add the netfn and others");
> + return -1;
> + }
> +
> + r = sd_bus_message_append_array(m, 'y', buf, 1);
> + if (r < 0) {
> + MSG_ERR("FAIL ");
> + errno = -r;
> + perror("Failed to add the string of response bytes");
> + return -1;
> + }
> +
> + r = sd_bus_call(bus, m, 0, &error, &reply);
> + if (r < 0) {
> + MSG_ERR("FAIL ");
> + errno = -r;
> + perror("Failed to call the method");
> + return -1;
> + }
> +
> + r = sd_bus_message_read(reply, "x", &pty);
> + if (r < 0) {
> + MSG_ERR("FAIL ");
> + errno = -r;
> + perror("Failed to get a rc from the method");
> + }
> +
> + sd_bus_error_free(&error);
> + sd_bus_message_unref(m);
> +
> + return 0;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + sd_bus_slot *slot;
> + int r;
> +
> + /* Connect to system bus */
> + r = sd_bus_open_system(&bus);
Maybe we can avoid the system bus? See comment dbus-run-
session/sd_bus_new comments below.
> + if (r < 0) {
> + MSG_ERR("FAIL");
> + errno = -r;
> + perror("Failed to connect to system bus");
> + return 1;
> + }
> +
> + r = sd_bus_add_match(bus, &slot, "type='signal',"
> + "interface='org.openbmc.HostIpmi',"
> + "member='ReceivedMessage'", bttest_ipmi, NULL);
> + if (r < 0) {
> + MSG_ERR("FAIL");
> + errno = -r;
> + perror("Failed: sd_bus_add_match");
> + goto finish;
> + }
> +
> +
> + for (;;) {
> + r = sd_bus_process(bus, NULL);
> + if (r < 0) {
> + MSG_ERR("FAIL");
> + errno = -r;
> + perror("Failed to process bus");
> + goto finish;
> + }
> +
> + r = sd_bus_wait(bus, (uint64_t) - 1);
> + if (r < 0) {
> + MSG_ERR("FAIL");
> + errno = -r;
> + perror("Failed to wait on bus");
> + goto finish;
> + }
> + }
> +
> +finish:
> + sd_bus_slot_unref(slot);
> + sd_bus_unref(bus);
> +
> + return 0;
> +}
> diff --git a/travis/build.sh b/travis/build.sh
> index 79b0b5c..e330afd 100755
> --- a/travis/build.sh
> +++ b/travis/build.sh
> @@ -1,9 +1,11 @@
> #!/bin/bash
> +set -evx
>
> Dockerfile=$(cat << EOF
> FROM ubuntu:15.10
> RUN DEBIAN_FRONTEND=noninteractive apt-get update && apt-get upgrade -yy
> RUN DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -yy make gcc libsystemd-dev libc6-dev pkg-config
> +RUN mkdir /var/run/dbus
> RUN groupadd -g ${GROUPS} ${USER} && useradd -d ${HOME} -m -u ${UID} -g ${GROUPS} ${USER}
> USER ${USER}
> ENV HOME ${HOME}
> @@ -14,6 +16,9 @@ EOF
> docker pull ubuntu:15.10
> docker build -t temp - <<< "${Dockerfile}"
>
> +sudo cp ./travis/org.openbmc.HostIpmi.conf.test /etc/dbus-1/system.d/org.openbmc.HostIpmi.conf
> +sudo service dbus restart
Can we instead run under dbus-run-session(1)? Or maybe use
sd_bus_new()/sd_bus_start()? If so we might not have to install the
conf under /etc/dbus-1/system.d/... either?
> +
> gcc --version
>
> mkdir -p linux
> @@ -21,3 +26,7 @@ wget https://raw.githubusercontent.com/openbmc/linux/dev-4.3/include/uapi/linux/
>
> docker run --cap-add=sys_admin --net=host --rm=true --user="${USER}" \
> -w "${PWD}" -v "${HOME}":"${HOME}" -t temp make KERNEL_HEADERS=$PWD
> +
> +docker run --cap-add=sys_admin --net=host -v /var/run/dbus:/var/run/dbus --rm=true --user="${USER}" \
> + -w "${PWD}" -v "${HOME}":"${HOME}" -t temp ./travis/run_tests.sh
> +
> diff --git a/travis/org.openbmc.HostIpmi.conf.test b/travis/org.openbmc.HostIpmi.conf.test
> new file mode 100644
> index 0000000..196945f
> --- /dev/null
> +++ b/travis/org.openbmc.HostIpmi.conf.test
> @@ -0,0 +1,20 @@
> +
> +
> +1.0//EN"
> + "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">;
> +
> +
> + This file is need to run openbmc bt bridge daemon.
> + Place this file in /etc/dbus-1/system.d/
> +-->
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> diff --git a/travis/run_tests.sh b/travis/run_tests.sh
> new file mode 100755
> index 0000000..a391798
> --- /dev/null
> +++ b/travis/run_tests.sh
> @@ -0,0 +1,15 @@
> +#!/bin/bash
> +set -evx
> +make KERNEL_HEADERS=${PWD} test
> +LD_PRELOAD=${PWD}/bt-host.so ./btbridged --vv &
> +bridge_pid=$!
> +
> +./ipmi-bouncer &
> +ipmi_pid=$!
> +
> +wait $bridge_pid
> +exit_status=$?
> +
> +kill -9 $ipmi_pid
If we play our cards right with using a non-system-bus, sd_bus_wait()
looks like it would give us an -ENOTCONN if the bus is closed, at which
point ipmi-bouncer would exit gracefully rather than being SIGKILLed.
Thoughts?
> +
> +exit $exit_status
Cheers,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160519/eafa4431/attachment-0001.sig>
More information about the openbmc
mailing list