[PATCH btbridge v4 0/6] Tests

[PATCH btbridge v4 0/6] Tests

[OpenBMC Patches]

Initial tests commit. Adding a framework to add more tests

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/openbmc/btbridge/12)
<!-- Reviewable:end -->


https://github.com/openbmc/btbridge/pull/12

Cyril Bur (6):
  Initialise variable to avoid using it uninitialised
  Increase debugging output when receiving a response to message from
    dbus
  Add travis dir to keep all the Travis CI specific files together
  Initial set of test.
  Travis CI: Bump to Ubuntu 16.04
  Add .gitignore

 .build.sh                             |  23 ----
 .gitignore                            |   5 +
 .travis.yml                           |   2 +-
 Makefile                              |   7 +
 bt-host.c                             | 235 ++++++++++++++++++++++++++++++++++
 btbridged.c                           |   4 +-
 ipmi-bouncer.c                        | 131 +++++++++++++++++++
 travis/build.sh                       |  32 +++++
 travis/org.openbmc.HostIpmi.conf.test |  20 +++
 travis/run_tests.sh                   |  15 +++
 10 files changed, 449 insertions(+), 25 deletions(-)
 delete mode 100755 .build.sh
 create mode 100644 .gitignore
 create mode 100644 bt-host.c
 create mode 100644 ipmi-bouncer.c
 create mode 100755 travis/build.sh
 create mode 100644 travis/org.openbmc.HostIpmi.conf.test
 create mode 100755 travis/run_tests.sh

-- 
2.8.1

[PATCH btbridge v4 1/6] Initialise variable to avoid using it uninitialised

[OpenBMC Patches]

From: Cyril Bur <cyril.bur@au1.ibm.com>

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 btbridged.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/btbridged.c b/btbridged.c
index fe692bb..3e261a9 100644
--- a/btbridged.c
+++ b/btbridged.c
@@ -489,7 +489,7 @@ static int dispatch_sd_bus(struct btbridged_context *context)
 static int dispatch_bt(struct btbridged_context *context)
 {
 	int err = 0;
-	int r;
+	int r = 0;
 
 	assert(context);
 
-- 
2.8.1

[PATCH btbridge v4 2/6] Increase debugging output when receiving a response to message from dbus

[OpenBMC Patches]

From: Cyril Bur <cyril.bur@au1.ibm.com>

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 btbridged.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/btbridged.c b/btbridged.c
index 3e261a9..be25eb9 100644
--- a/btbridged.c
+++ b/btbridged.c
@@ -317,6 +317,8 @@ static int method_send_message(sd_bus_message *msg, void *userdata, sd_bus_error
 		goto out;
 	}
 	MSG_OUT("Received a dbus response for msg with seq 0x%02x\n", seq);
+	MSG_OUT("(netfn 0x%02x lun 0x%02x cmd 0x%02x cc 0x%02x with data of size %lu)\n",
+			netfn, lun, cmd, cc, data_sz);
 	bt_msg->call = sd_bus_message_ref(msg);
 	bt_msg->rsp.netfn = netfn;
 	bt_msg->rsp.lun = lun;
-- 
2.8.1

[PATCH btbridge v4 3/6] Add travis dir to keep all the Travis CI specific files together

[OpenBMC Patches]

From: Cyril Bur <cyril.bur@au1.ibm.com>

---
 .build.sh       | 23 -----------------------
 .travis.yml     |  2 +-
 travis/build.sh | 23 +++++++++++++++++++++++
 3 files changed, 24 insertions(+), 24 deletions(-)
 delete mode 100755 .build.sh
 create mode 100755 travis/build.sh

diff --git a/.build.sh b/.build.sh
deleted file mode 100755
index 79b0b5c..0000000
--- a/.build.sh
+++ /dev/null
@@ -1,23 +0,0 @@
-#!/bin/bash
-
-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 groupadd -g ${GROUPS} ${USER} && useradd -d ${HOME} -m -u ${UID} -g ${GROUPS} ${USER}
-USER ${USER}
-ENV HOME ${HOME}
-RUN /bin/bash
-EOF
-)
-
-docker pull ubuntu:15.10
-docker build -t temp - <<< "${Dockerfile}"
-
-gcc --version
-
-mkdir -p linux
-wget https://raw.githubusercontent.com/openbmc/linux/dev-4.3/include/uapi/linux/bt-host.h -O linux/bt-host.h
-
-docker run --cap-add=sys_admin --net=host --rm=true --user="${USER}" \
- -w "${PWD}" -v "${HOME}":"${HOME}" -t temp make KERNEL_HEADERS=$PWD
diff --git a/.travis.yml b/.travis.yml
index faaa918..bfdedab 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -6,4 +6,4 @@ services:
 language: c
 
 script:
-    - ./.build.sh
+    - ./travis/build.sh
diff --git a/travis/build.sh b/travis/build.sh
new file mode 100755
index 0000000..79b0b5c
--- /dev/null
+++ b/travis/build.sh
@@ -0,0 +1,23 @@
+#!/bin/bash
+
+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 groupadd -g ${GROUPS} ${USER} && useradd -d ${HOME} -m -u ${UID} -g ${GROUPS} ${USER}
+USER ${USER}
+ENV HOME ${HOME}
+RUN /bin/bash
+EOF
+)
+
+docker pull ubuntu:15.10
+docker build -t temp - <<< "${Dockerfile}"
+
+gcc --version
+
+mkdir -p linux
+wget https://raw.githubusercontent.com/openbmc/linux/dev-4.3/include/uapi/linux/bt-host.h -O linux/bt-host.h
+
+docker run --cap-add=sys_admin --net=host --rm=true --user="${USER}" \
+ -w "${PWD}" -v "${HOME}":"${HOME}" -t temp make KERNEL_HEADERS=$PWD
-- 
2.8.1

[PATCH btbridge v4 4/6] Initial set of test.

[OpenBMC Patches]

From: Cyril Bur <cyril.bur@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 the
running of the tests and being able to fake /dev/bt-host.

Signed-off-by: Cyril Bur <cyril.bur@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 <dlfcn.h>
+#include <errno.h>
+#include <poll.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/types.h>          /* See NOTES */
+#include <sys/socket.h>
+
+#include <linux/bt-host.h>
+
+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++;
+
+		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;
+
+		memcpy(buf, data[sent_id].msg, data[sent_id].msg[0] + 1);
+		return data[sent_id].msg[0] + 1;
+	}
+
+	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");
+			exit(0);
+		}
+		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;
+}
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 <errno.h>
+#include <stdio.h>
+
+#include <systemd/sd-bus.h>
+
+#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);
+	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
+
 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 @@
+<?xml version="1.0"?> <!--*-nxml-*-->
+<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-BUS Bus Configuration
+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/
+-->
+
+<busconfig>
+
+        <policy context="default">
+                <allow own="org.openbmc.HostIpmi"/>
+                <allow send_destination="org.openbmc.HostIpmi"/>
+                <allow receive_sender="org.openbmc.HostIpmi"/>
+        </policy>
+
+</busconfig>
+
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
+
+exit $exit_status
-- 
2.8.1

[PATCH btbridge v4 5/6] Travis CI: Bump to Ubuntu 16.04

[OpenBMC Patches]

From: Cyril Bur <cyril.bur@au1.ibm.com>

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 travis/build.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/travis/build.sh b/travis/build.sh
index e330afd..6b7f0fb 100755
--- a/travis/build.sh
+++ b/travis/build.sh
@@ -2,7 +2,7 @@
 set -evx
 
 Dockerfile=$(cat << EOF
-FROM ubuntu:15.10
+FROM ubuntu:16.04
 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
@@ -13,7 +13,7 @@ RUN /bin/bash
 EOF
 )
 
-docker pull ubuntu:15.10
+docker pull ubuntu:16.04
 docker build -t temp - <<< "${Dockerfile}"
 
 sudo cp ./travis/org.openbmc.HostIpmi.conf.test /etc/dbus-1/system.d/org.openbmc.HostIpmi.conf
-- 
2.8.1

[PATCH btbridge v4 6/6] Add .gitignore

[OpenBMC Patches]

From: Cyril Bur <cyril.bur@au1.ibm.com>

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 .gitignore | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 .gitignore

diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..d0bbfdb
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,5 @@
+bt-host.so
+btbridged
+ipmi-bouncer
+linux/*
+
-- 
2.8.1

Re: [PATCH btbridge v4 0/6] Tests

[Cyril Bur]

On Tue,  3 May 2016 20:10:02 -0500
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

Sorry about the noise, github is such a mystery to my, no idea how this
happened.

This one looks good.

Cyril

> Initial tests commit. Adding a framework to add more tests
> 
> <!-- Reviewable:start -->
> ---
> This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/openbmc/btbridge/12)
> <!-- Reviewable:end -->
> 
> 
> https://github.com/openbmc/btbridge/pull/12
> 
> Cyril Bur (6):
>   Initialise variable to avoid using it uninitialised
>   Increase debugging output when receiving a response to message from
>     dbus
>   Add travis dir to keep all the Travis CI specific files together
>   Initial set of test.
>   Travis CI: Bump to Ubuntu 16.04
>   Add .gitignore
> 
>  .build.sh                             |  23 ----
>  .gitignore                            |   5 +
>  .travis.yml                           |   2 +-
>  Makefile                              |   7 +
>  bt-host.c                             | 235 ++++++++++++++++++++++++++++++++++
>  btbridged.c                           |   4 +-
>  ipmi-bouncer.c                        | 131 +++++++++++++++++++
>  travis/build.sh                       |  32 +++++
>  travis/org.openbmc.HostIpmi.conf.test |  20 +++
>  travis/run_tests.sh                   |  15 +++
>  10 files changed, 449 insertions(+), 25 deletions(-)
>  delete mode 100755 .build.sh
>  create mode 100644 .gitignore
>  create mode 100644 bt-host.c
>  create mode 100644 ipmi-bouncer.c
>  create mode 100755 travis/build.sh
>  create mode 100644 travis/org.openbmc.HostIpmi.conf.test
>  create mode 100755 travis/run_tests.sh
> 

Re: [PATCH btbridge v4 1/6] Initialise variable to avoid using it uninitialised

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

Hi Cyril,

On Tue, 2016-05-03 at 20:10 -0500, OpenBMC Patches wrote:
> From: Cyril Bur <cyril.bur@au1.ibm.com>
> 
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  btbridged.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/btbridged.c b/btbridged.c
> index fe692bb..3e261a9 100644
> --- a/btbridged.c
> +++ b/btbridged.c
> @@ -489,7 +489,7 @@ static int dispatch_sd_bus(struct btbridged_context *context)
>  static int dispatch_bt(struct btbridged_context *context)
>  {
>  	int err = 0;
> -	int r;
> +	int r = 0;
>  
>  	assert(context);
>  

Building with this patch and native GCC* gives errors:

    $ KERNEL_HEADERS=../../linux/ast2400/include/uapi/ make
    cc  -Wall -O2 -g -I../../linux/ast2400/include/uapi/    btbridged.c  -lsystemd -o btbridged
    btbridged.c: In function ‘main’:
    btbridged.c:590:6: warning: ‘r’ may be used uninitialized in this function [-Wmaybe-uninitialized]
       if (r < 0)
          ^
    btbridged.c:342:6: note: ‘r’ was declared here
      int r, len;
          ^

That's weird, because the note isn't relevant to the function of line
that generated the warning. However, the 'r' defined in bt_host_write()
suffers the same initialisation issue. Initialising it gives me a build
with no warnings so maybe it's worth doing that here also?

Otherwise,

Acked-by: Andrew Jeffery <andrew@aj.id.au>

Cheers,

Andrew

* $ gcc --version
gcc (Ubuntu 5.3.1-14ubuntu2) 5.3.1 20160413

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH btbridge v4 2/6] Increase debugging output when receiving a response to message from dbus

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

On Tue, 2016-05-03 at 20:10 -0500, OpenBMC Patches wrote:
> From: Cyril Bur <cyril.bur@au1.ibm.com>
> 
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  btbridged.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/btbridged.c b/btbridged.c
> index 3e261a9..be25eb9 100644
> --- a/btbridged.c
> +++ b/btbridged.c
> @@ -317,6 +317,8 @@ static int method_send_message(sd_bus_message
> *msg, void *userdata, sd_bus_error
>  		goto out;
>  	}
>  	MSG_OUT("Received a dbus response for msg with seq
> 0x%02x\n", seq);
> +	MSG_OUT("(netfn 0x%02x lun 0x%02x cmd 0x%02x cc 0x%02x with
> data of size %lu)\n",
> +			netfn, lun, cmd, cc, data_sz);
>  	bt_msg->call = sd_bus_message_ref(msg);
>  	bt_msg->rsp.netfn = netfn;
>  	bt_msg->rsp.lun = lun;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH btbridge v4 3/6] Add travis dir to keep all the Travis CI specific files together

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

On Tue, 2016-05-03 at 20:10 -0500, OpenBMC Patches wrote:
> From: Cyril Bur <cyril.bur@au1.ibm.com>
> 

This needs a S-o-B tag.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>


> ---
>  .build.sh       | 23 -----------------------
>  .travis.yml     |  2 +-
>  travis/build.sh | 23 +++++++++++++++++++++++
>  3 files changed, 24 insertions(+), 24 deletions(-)
>  delete mode 100755 .build.sh
>  create mode 100755 travis/build.sh
> 
> diff --git a/.build.sh b/.build.sh
> deleted file mode 100755
> index 79b0b5c..0000000
> --- a/.build.sh
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -#!/bin/bash
> -
> -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 groupadd -g ${GROUPS} ${USER} && useradd -d ${HOME} -m -u ${UID}
> -g ${GROUPS} ${USER}
> -USER ${USER}
> -ENV HOME ${HOME}
> -RUN /bin/bash
> -EOF
> -)
> -
> -docker pull ubuntu:15.10
> -docker build -t temp - <<< "${Dockerfile}"
> -
> -gcc --version
> -
> -mkdir -p linux
> -wget https://raw.githubusercontent.com/openbmc/linux/dev-4.3/include
> /uapi/linux/bt-host.h -O linux/bt-host.h
> -
> -docker run --cap-add=sys_admin --net=host --rm=true --user="${USER}" 
> \
> - -w "${PWD}" -v "${HOME}":"${HOME}" -t temp make KERNEL_HEADERS=$PWD
> diff --git a/.travis.yml b/.travis.yml
> index faaa918..bfdedab 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -6,4 +6,4 @@ services:
>  language: c
>  
>  script:
> -    - ./.build.sh
> +    - ./travis/build.sh
> diff --git a/travis/build.sh b/travis/build.sh
> new file mode 100755
> index 0000000..79b0b5c
> --- /dev/null
> +++ b/travis/build.sh
> @@ -0,0 +1,23 @@
> +#!/bin/bash
> +
> +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 groupadd -g ${GROUPS} ${USER} && useradd -d ${HOME} -m -u ${UID}
> -g ${GROUPS} ${USER}
> +USER ${USER}
> +ENV HOME ${HOME}
> +RUN /bin/bash
> +EOF
> +)
> +
> +docker pull ubuntu:15.10
> +docker build -t temp - <<< "${Dockerfile}"
> +
> +gcc --version
> +
> +mkdir -p linux
> +wget https://raw.githubusercontent.com/openbmc/linux/dev-4.3/include
> /uapi/linux/bt-host.h -O linux/bt-host.h
> +
> +docker run --cap-add=sys_admin --net=host --rm=true --user="${USER}" 
> \
> + -w "${PWD}" -v "${HOME}":"${HOME}" -t temp make KERNEL_HEADERS=$PWD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH btbridge v4 4/6] Initial set of test.

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 16266 bytes --]

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@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@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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH btbridge v4 5/6] Travis CI: Bump to Ubuntu 16.04

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On Tue, 2016-05-03 at 20:10 -0500, OpenBMC Patches wrote:
> From: Cyril Bur <cyril.bur@au1.ibm.com>
> 
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  travis/build.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/travis/build.sh b/travis/build.sh
> index e330afd..6b7f0fb 100755
> --- a/travis/build.sh
> +++ b/travis/build.sh
> @@ -2,7 +2,7 @@
>  set -evx
>  
>  Dockerfile=$(cat << EOF
> -FROM ubuntu:15.10
> +FROM ubuntu:16.04
>  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
> @@ -13,7 +13,7 @@ RUN /bin/bash
>  EOF
>  )
>  
> -docker pull ubuntu:15.10
> +docker pull ubuntu:16.04

Are there reasons for this beyond it being new and shiny? Whether there
are or not, can you please add a description of the motivation to the
commit message?

>  docker build -t temp - <<< "${Dockerfile}"
>  
>  sudo cp ./travis/org.openbmc.HostIpmi.conf.test /etc/dbus-1/system.d/org.openbmc.HostIpmi.conf

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH btbridge v4 6/6] Add .gitignore

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]

On Tue, 2016-05-03 at 20:10 -0500, OpenBMC Patches wrote:
> From: Cyril Bur <cyril.bur@au1.ibm.com>
> 
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  .gitignore | 5 +++++
>  1 file changed, 5 insertions(+)
>  create mode 100644 .gitignore
> 
> diff --git a/.gitignore b/.gitignore
> new file mode 100644
> index 0000000..d0bbfdb
> --- /dev/null
> +++ b/.gitignore
> @@ -0,0 +1,5 @@
> +bt-host.so
> +btbridged
> +ipmi-bouncer
> +linux/*
> +

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH btbridge v4 1/6] Initialise variable to avoid using it uninitialised

[Cyril Bur]

On Thu, 19 May 2016 12:40:49 +0930
Andrew Jeffery <andrew@aj.id.au> wrote:

> Hi Cyril,
> 
> On Tue, 2016-05-03 at 20:10 -0500, OpenBMC Patches wrote:
> > From: Cyril Bur <cyril.bur@au1.ibm.com>
> > 
> > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> > ---
> >  btbridged.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/btbridged.c b/btbridged.c
> > index fe692bb..3e261a9 100644
> > --- a/btbridged.c
> > +++ b/btbridged.c
> > @@ -489,7 +489,7 @@ static int dispatch_sd_bus(struct btbridged_context *context)
> >  static int dispatch_bt(struct btbridged_context *context)
> >  {
> >  	int err = 0;
> > -	int r;
> > +	int r = 0;
> >  
> >  	assert(context);
> >    
> 
> Building with this patch and native GCC* gives errors:
> 
>     $ KERNEL_HEADERS=../../linux/ast2400/include/uapi/ make
>     cc  -Wall -O2 -g -I../../linux/ast2400/include/uapi/    btbridged.c  -lsystemd -o btbridged
>     btbridged.c: In function ‘main’:
>     btbridged.c:590:6: warning: ‘r’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>        if (r < 0)
>           ^
>     btbridged.c:342:6: note: ‘r’ was declared here
>       int r, len;
>           ^
> 
> That's weird, because the note isn't relevant to the function of line
> that generated the warning. However, the 'r' defined in bt_host_write()
> suffers the same initialisation issue. Initialising it gives me a build
> with no warnings so maybe it's worth doing that here also?
> 

Yeah, GCC seems to be getting a bit confused. I'm usually not in favour of
putting in stuff to shut the warnings up but these are just so odd that I'm at
a loss.

Your suggestion works, lets just do that and forget about it.

Thanks

> Otherwise,
> 
> Acked-by: Andrew Jeffery <andrew@aj.id.au>
> 
> Cheers,
> 
> Andrew
> 
> * $ gcc --version
> gcc (Ubuntu 5.3.1-14ubuntu2) 5.3.1 20160413

Re: [PATCH btbridge v4 4/6] Initial set of test.

[Cyril Bur]

On Thu, 19 May 2016 14:55:46 +0930
Andrew Jeffery <andrew@aj.id.au> wrote:

> 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.

Yeah looks better.

> 
> On Tue, 2016-05-03 at 20:10 -0500, OpenBMC Patches wrote:
> > From: Cyril Bur <cyril.bur@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'
> 

Heh thanks.

> >  the
> > running of the tests and being able to fake /dev/bt-host.
> > 
> > Signed-off-by: Cyril Bur <cyril.bur@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?
> 

Honestly your guess is as good as mine at this point, I wrote this so long ago.
Probably artefact of how I was trying to track what I'd sent before I did it
this way. I'll remove.

> > +
> > +		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));
> 

Looks nicer thanks.

> > +
> > +		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?
> 

Sure

> > +	}
> > +
> > +	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?

So, there's always a way. This exit achieves 2 things. 1 reporting success (or
fail in the case of other exits) and to end the btbridge process.

At the moment there isn't really a nice way to end the btbridge process, and if
we did it would probably be signal based, I don't like the sending ourselves a
signal from an LD_PRELOAD library solution either.

A nice solution would be to have another thread that the LD_PRELOAD code could
report to and on pass results to and then it would be in charge of cleaning up
btbridge, that solution is more work though.

I have thought that a kind of test runner thread might be a good idea, maybe
one day.

> 
> > +		}
> > +		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()?

Yeah wasn't thrilled about having to do this. So obviously I've wrapped it
purely to know the FD that is going to get passed back to poll(). In poll I
catch it and drop. Because the travisci environment isn't particularly quick I
was getting timing related problems.

I don't want to claim we've really tuned the timeouts in btbridged but the
tradeoff of upping the timeouts just for tests VS increasing the test
complexity, well, I went with removing those timing problems by not reporting
the time to btbridged.

> 
> > +}  
> 
> Overall the wrapping seems like a lot of effort :/
> 

Maybe rather than do it the way I did it, I could have simply increased the
timeout in the timerfd_create wrapper... just occurred to me now, still, then
we get the problem of how much more... although it would allow us to test
timeout pathes...

> > 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?
> 

So I struggled to get any dbus going and I did initially think that a session
bus was the way to go but it just never worked. I will revisit now that I know
a bit more about it

> > +
> >  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?

ipmi-bouncer knowing to cleanup would be a nice win!

> 
> > +
> > +exit $exit_status  
> 
> Cheers,
> 

Thanks for the review,

Cyril

> Andrew

Re: [PATCH btbridge v4 5/6] Travis CI: Bump to Ubuntu 16.04

[Cyril Bur]

On Thu, 19 May 2016 14:58:46 +0930
Andrew Jeffery <andrew@aj.id.au> wrote:

> On Tue, 2016-05-03 at 20:10 -0500, OpenBMC Patches wrote:
> > From: Cyril Bur <cyril.bur@au1.ibm.com>
> > 
> > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> > ---
> >  travis/build.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/travis/build.sh b/travis/build.sh
> > index e330afd..6b7f0fb 100755
> > --- a/travis/build.sh
> > +++ b/travis/build.sh
> > @@ -2,7 +2,7 @@
> >  set -evx
> >  
> >  Dockerfile=$(cat << EOF
> > -FROM ubuntu:15.10
> > +FROM ubuntu:16.04
> >  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
> > @@ -13,7 +13,7 @@ RUN /bin/bash
> >  EOF
> >  )
> >  
> > -docker pull ubuntu:15.10
> > +docker pull ubuntu:16.04  
> 
> Are there reasons for this beyond it being new and shiny? Whether there
> are or not, can you please add a description of the motivation to the
> commit message?

I like shiny things. Mostly shinyness but 16.04 being LTS is a nice bonus :).
Will do.

> 
> >  docker build -t temp - <<< "${Dockerfile}"
> >  
> >  sudo cp ./travis/org.openbmc.HostIpmi.conf.test /etc/dbus-1/system.d/org.openbmc.HostIpmi.conf  
> 
> Cheers,
> 
> Andrew

Re: [PATCH btbridge v4 4/6] Initial set of test.

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 5260 bytes --]

On Thu, 2016-05-19 at 16:18 +1000, Cyril Bur wrote:
> On Thu, 19 May 2016 14:55:46 +0930
> Andrew Jeffery <andrew@aj.id.au> wrote:
> +
> > > +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?
> So, there's always a way. This exit achieves 2 things. 1 reporting success (or
> fail in the case of other exits) and to end the btbridge process.
> 
> At the moment there isn't really a nice way to end the btbridge process, and if
> we did it would probably be signal based, I don't like the sending ourselves a
> signal from an LD_PRELOAD library solution either.
> 
> A nice solution would be to have another thread that the LD_PRELOAD code could
> report to and on pass results to and then it would be in charge of cleaning up
> btbridge, that solution is more work though.

Adding threads as an alternative doesn't excite me. exit(...) is at
least succinct.

> 
> I have thought that a kind of test runner thread might be a good idea, maybe
> one day.
> 
> > 
> > 
> > > 
> > > +		}
> > > +		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()?
> Yeah wasn't thrilled about having to do this. So obviously I've wrapped it
> purely to know the FD that is going to get passed back to poll().

Hah, I actually overlooked that. I noticed the test against timer_fd in
the poll() override, but for whatever reason it didn't click.


>  In poll I
> catch it and drop. Because the travisci environment isn't particularly quick I
> was getting timing related problems.

Right. I think adding that as a comment would be useful justification.

> 
> I don't want to claim we've really tuned the timeouts in btbridged but the
> tradeoff of upping the timeouts just for tests VS increasing the test
> complexity, well, I went with removing those timing problems by not reporting
> the time to btbridged.

Seems reasonable.

> 
> > 
> > 
> > > 
> > > +}  
> > Overall the wrapping seems like a lot of effort :/
> > 
> Maybe rather than do it the way I did it, I could have simply increased the
> timeout in the timerfd_create wrapper... just occurred to me now, still, then
> we get the problem of how much more... although it would allow us to test
> timeout pathes...

Maybe leave it at adding a TODO comment to that effect?

> 
> > > 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 DEIAN_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?
> > 
> So I struggled to get any dbus going and I did initially think that a session
> bus was the way to go but it just never worked. I will revisit now that I know
> a bit more about it

Okay, hopefully it's not too much messing around.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH btbridge v4 1/6] Initialise variable to avoid using it uninitialised

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 5007 bytes --]

On Thu, 2016-05-19 at 15:44 +1000, Cyril Bur wrote:
> > Building with this patch and native GCC* gives errors:
> > 
> >     $ KERNEL_HEADERS=../../linux/ast2400/include/uapi/ make
> >     cc  -Wall -O2 -g -I../../linux/ast2400/include/uapi/    btbridged.c  -lsystemd -o btbridged
> >     btbridged.c: In function ‘main’:
> >     btbridged.c:590:6: warning: ‘r’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >        if (r < 0)
> >           ^
> >     btbridged.c:342:6: note: ‘r’ was declared here
> >       int r, len;
> >           ^
> > 
> > That's weird, because the note isn't relevant to the function of line
> > that generated the warning. However, the 'r' defined in bt_host_write()
> > suffers the same initialisation issue. Initialising it gives me a build
> > with no warnings so maybe it's worth doing that here also?
> > 
> 
> Yeah, GCC seems to be getting a bit confused. I'm usually not in favour of
> putting in stuff to shut the warnings up but these are just so odd that I'm at
> a loss.
> 
> Your suggestion works, lets just do that and forget about it.

So, just out of interest I ran scan-build over it. Turns out it leads
to a bug. Here's the reasoning:

339	static int bt_host_write(struct btbridged_context *context, struct bt_queue *bt_msg)
340	{
341		struct bt_queue *head;
342		uint8_t data[BT_MAX_MESSAGE] = { 0 };
343		sd_bus_message *msg = NULL;
344		int r, len;
	
1	'r' declared without an initial value	→
345	 
346		assert(context);
347	 
348		if (!bt_msg)
	
2←	Assuming 'bt_msg' is non-null	→
	
3←	Taking false branch	→
349			return -EINVAL;
350	 
351		head = bt_q_get_head(context);
352		if (bt_msg == head) {
	
4←	Assuming 'bt_msg' is not equal to 'head'	→
	
5←	Taking false branch	→
353			struct itimerspec ts;
354			/* Need to adjust the timer */
355			ts.it_interval.tv_sec = 0;
356			ts.it_interval.tv_nsec = 0;
357			if (head->next) {
358				ts.it_value = head->next->start;
359				ts.it_value.tv_sec += BT_HOST_TIMEOUT_SEC;
360				MSG_OUT("Adjusting timer for next element\n");
361			} else {
362				ts.it_value.tv_nsec = 0;
363				ts.it_value.tv_sec = 0;
364				MSG_OUT("Disabling timer, no elements remain in queue\n");
365			}
366			r = timerfd_settime(context->fds[TIMER_FD].fd, TFD_TIMER_ABSTIME, &ts, NULL);
367			if (r == -1)
368				MSG_ERR("Couldn't set timerfd\n");
369		}
370		data[1] = (bt_msg->rsp.netfn << 2) | (bt_msg->rsp.lun & 0x3);
371		data[2] = bt_msg->rsp.seq;
372		data[3] = bt_msg->rsp.cmd;
373		data[4] = bt_msg->rsp.cc;
374		if (bt_msg->rsp.data_len > sizeof(data) - 5) {
	
6←	Taking false branch	→
375			MSG_ERR("Response message size (%zu) too big, truncating\n", bt_msg->rsp.data_len);
376			bt_msg->rsp.data_len = sizeof(data) - 5;
377		}
378		/* netfn/len + seq + cmd + cc = 4 */
379		data[0] = bt_msg->rsp.data_len + 4;
380		if (bt_msg->rsp.data_len)
	
7←	Taking false branch	→
381			memcpy(data + 5, bt_msg->rsp.data, bt_msg->rsp.data_len);
382		/* Count the data[0] byte */
383		len = write(context->fds[BT_FD].fd, data, data[0] + 1);
384		if (len == -1) {
	
8←	Taking false branch     →
385			r = errno;
386			MSG_ERR("Error writing to %s: %s\n", BT_HOST_PATH, strerror(errno));
387			if (bt_msg->call) {
388				r = sd_bus_message_new_method_errno(bt_msg->call, &msg, r, NULL);
389				if (r < 0)
390					MSG_ERR("Couldn't create response error\n");
391			}
392			goto out;
393		} else {
394			if (len != data[0] + 1)
	
9←	Taking false branch	→
395				MSG_ERR("Possible short write to %s, desired len: %d, written len: %d\n", BT_HOST_PATH, data[0] + 1, len);
396			else
397				MSG_OUT("Successfully wrote %d of %d bytes to %s\n", len, data[0] + 1, BT_HOST_PATH);
398	 
399			if (bt_msg->call) {
	
10←	Taking false branch	→
400				r = sd_bus_message_new_method_return(bt_msg->call, &msg);
401				if (r < 0) {
402					MSG_ERR("Couldn't create response message\n");
403					goto out;
404				}
405				r = 0; /* Just to be explicit about what we're sending back */
406				r = sd_bus_message_append(msg, "x", r);
407				if (r < 0) {
408					MSG_ERR("Couldn't append result to response\n");
409					goto out;
410				}
411	 
412			}
413		}
414	 
415	out:
416		if (bt_msg->call) {
	
11←	Taking false branch	→
417			if (sd_bus_send(context->bus, msg, NULL) < 0)
418				MSG_ERR("Couldn't send response message\n");
419			sd_bus_message_unref(msg);
420		}
421		bt_q_drop(context, bt_msg);
422	 
423		/*
424		 * There isn't another message ready to be sent so turn off POLLOUT
425		 */
426		if (!bt_q_get_msg(context)) {
	
12←	Taking false branch	→
427			MSG_OUT("Turning off POLLOUT for the BT in poll()\n");
428			context->fds[BT_FD].events = POLLIN;
429		}
430	 
431		return r;
	
13←	Undefined or garbage value returned to caller
432	}
433

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH btbridge v4 1/6] Initialise variable to avoid using it uninitialised

[Cyril Bur]

On Tue, 24 May 2016 22:33:22 +0930
Andrew Jeffery <andrew@aj.id.au> wrote:

> On Thu, 2016-05-19 at 15:44 +1000, Cyril Bur wrote:
> > > Building with this patch and native GCC* gives errors:
> > > 
> > >     $ KERNEL_HEADERS=../../linux/ast2400/include/uapi/ make
> > >     cc  -Wall -O2 -g -I../../linux/ast2400/include/uapi/    btbridged.c  -lsystemd -o btbridged
> > >     btbridged.c: In function ‘main’:
> > >     btbridged.c:590:6: warning: ‘r’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> > >        if (r < 0)
> > >           ^
> > >     btbridged.c:342:6: note: ‘r’ was declared here
> > >       int r, len;
> > >           ^
> > > 
> > > That's weird, because the note isn't relevant to the function of line
> > > that generated the warning. However, the 'r' defined in bt_host_write()
> > > suffers the same initialisation issue. Initialising it gives me a build
> > > with no warnings so maybe it's worth doing that here also?
> > >   
> > 
> > Yeah, GCC seems to be getting a bit confused. I'm usually not in favour of
> > putting in stuff to shut the warnings up but these are just so odd that I'm at
> > a loss.
> > 
> > Your suggestion works, lets just do that and forget about it.  
> 
> So, just out of interest I ran scan-build over it. Turns out it leads
> to a bug. Here's the reasoning:
> 

Well well well, found it! I'll hurry long the series which solves anyway.

Thanks!!

> 339	static int bt_host_write(struct btbridged_context *context, struct bt_queue *bt_msg)
> 340	{
> 341		struct bt_queue *head;
> 342		uint8_t data[BT_MAX_MESSAGE] = { 0 };
> 343		sd_bus_message *msg = NULL;
> 344		int r, len;
> 	
> 1	'r' declared without an initial value	→
> 345	 
> 346		assert(context);
> 347	 
> 348		if (!bt_msg)
> 	
> 2←	Assuming 'bt_msg' is non-null	→
> 	
> 3←	Taking false branch	→
> 349			return -EINVAL;
> 350	 
> 351		head = bt_q_get_head(context);
> 352		if (bt_msg == head) {
> 	
> 4←	Assuming 'bt_msg' is not equal to 'head'	→
> 	
> 5←	Taking false branch	→
> 353			struct itimerspec ts;
> 354			/* Need to adjust the timer */
> 355			ts.it_interval.tv_sec = 0;
> 356			ts.it_interval.tv_nsec = 0;
> 357			if (head->next) {
> 358				ts.it_value = head->next->start;
> 359				ts.it_value.tv_sec += BT_HOST_TIMEOUT_SEC;
> 360				MSG_OUT("Adjusting timer for next element\n");
> 361			} else {
> 362				ts.it_value.tv_nsec = 0;
> 363				ts.it_value.tv_sec = 0;
> 364				MSG_OUT("Disabling timer, no elements remain in queue\n");
> 365			}
> 366			r = timerfd_settime(context->fds[TIMER_FD].fd, TFD_TIMER_ABSTIME, &ts, NULL);
> 367			if (r == -1)
> 368				MSG_ERR("Couldn't set timerfd\n");
> 369		}
> 370		data[1] = (bt_msg->rsp.netfn << 2) | (bt_msg->rsp.lun & 0x3);
> 371		data[2] = bt_msg->rsp.seq;
> 372		data[3] = bt_msg->rsp.cmd;
> 373		data[4] = bt_msg->rsp.cc;
> 374		if (bt_msg->rsp.data_len > sizeof(data) - 5) {
> 	
> 6←	Taking false branch	→
> 375			MSG_ERR("Response message size (%zu) too big, truncating\n", bt_msg->rsp.data_len);
> 376			bt_msg->rsp.data_len = sizeof(data) - 5;
> 377		}
> 378		/* netfn/len + seq + cmd + cc = 4 */
> 379		data[0] = bt_msg->rsp.data_len + 4;
> 380		if (bt_msg->rsp.data_len)
> 	
> 7←	Taking false branch	→
> 381			memcpy(data + 5, bt_msg->rsp.data, bt_msg->rsp.data_len);
> 382		/* Count the data[0] byte */
> 383		len = write(context->fds[BT_FD].fd, data, data[0] + 1);
> 384		if (len == -1) {
> 	
> 8←	Taking false branch     →
> 385			r = errno;
> 386			MSG_ERR("Error writing to %s: %s\n", BT_HOST_PATH, strerror(errno));
> 387			if (bt_msg->call) {
> 388				r = sd_bus_message_new_method_errno(bt_msg->call, &msg, r, NULL);
> 389				if (r < 0)
> 390					MSG_ERR("Couldn't create response error\n");
> 391			}
> 392			goto out;
> 393		} else {
> 394			if (len != data[0] + 1)
> 	
> 9←	Taking false branch	→
> 395				MSG_ERR("Possible short write to %s, desired len: %d, written len: %d\n", BT_HOST_PATH, data[0] + 1, len);
> 396			else
> 397				MSG_OUT("Successfully wrote %d of %d bytes to %s\n", len, data[0] + 1, BT_HOST_PATH);
> 398	 
> 399			if (bt_msg->call) {
> 	
> 10←	Taking false branch	→
> 400				r = sd_bus_message_new_method_return(bt_msg->call, &msg);
> 401				if (r < 0) {
> 402					MSG_ERR("Couldn't create response message\n");
> 403					goto out;
> 404				}
> 405				r = 0; /* Just to be explicit about what we're sending back */
> 406				r = sd_bus_message_append(msg, "x", r);
> 407				if (r < 0) {
> 408					MSG_ERR("Couldn't append result to response\n");
> 409					goto out;
> 410				}
> 411	 
> 412			}
> 413		}
> 414	 
> 415	out:
> 416		if (bt_msg->call) {
> 	
> 11←	Taking false branch	→
> 417			if (sd_bus_send(context->bus, msg, NULL) < 0)
> 418				MSG_ERR("Couldn't send response message\n");
> 419			sd_bus_message_unref(msg);
> 420		}
> 421		bt_q_drop(context, bt_msg);
> 422	 
> 423		/*
> 424		 * There isn't another message ready to be sent so turn off POLLOUT
> 425		 */
> 426		if (!bt_q_get_msg(context)) {
> 	
> 12←	Taking false branch	→
> 427			MSG_OUT("Turning off POLLOUT for the BT in poll()\n");
> 428			context->fds[BT_FD].events = POLLIN;
> 429		}
> 430	 
> 431		return r;
> 	
> 13←	Undefined or garbage value returned to caller
> 432	}
> 433