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

[Andrew Jeffery]

On Thu, 2016-05-19 at 16:18 +1000, Cyril Bur wrote:
> On Thu, 19 May 2016 14:55:46 +0930
> Andrew Jeffery <andrew at 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
-------------- 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/76140cbd/attachment.sig>