Introducing read_map_or and readwrite_map_or to AppSlice

[Philip Levis]

Tl dr; As part of the effort to make AppSlice safer for Tock v2, I tried out a map approach and it looks good.

As we’ve been cleaning up the system call API for v2 and making it safer/more robust, one thing that came up is AppSlice and how a capsule should respond if a process fails while it’s using an AppSlice. The general consensus was that AppSlice::ptr() is problematic because it returns [] if the process has failed. The two candidate solutions were using a map-like/enter-like API, or having ptr() return a Result.

After trying both a bit, I think the map-like approach works well. The Result approach requires more up-front code in the caller, to match on the result (or condition on it). In contrast, if AppSlice has a map_or operation, you can specify the default value in the call such that it gives a result that overlaps with potentially a successful call.

This commit has the added methods for AppSlice and an example of using readonly_map_or in spi_controller.rs. From a safety/correctness approach, I very much prefer the new approach: it explicitly acknowledges that a continued send might fail and causes the capsule to deal with this. The major downside is that it adds another level of indentation. For this particular implementation, I actually think that can go away, since spi_controller.rs will no longer require an Option<AppSlice>, due to the new buffer-swapping semantics. It’ll receive AppSlices at initialization and keep those, so you can cut one of the level of map in the method.

https://github.com/tock/tock/commit/1e36aaeaf128151323e5f06b752b6703ca915b5c

Note that an arbitrary call to do_next_read_write could cause a panic if the AppSlice has disappeared. In the current implementation, there are checks such that it’s not invoked if such has happened.

I’m putting the before and after code here so you don’t have to click through if you don’t want to:

Before:

fn do_next_read_write(&self, app: &mut App) {
let start = app.index;
let len = cmp::min(app.len - start, self.kernel_len.get());
let end = start + len;
app.index = end;

self.kernel_write.map(|kwbuf| {
app.app_write.as_mut().map(|src| {
for (i, c) in src.as_ref()[start..end].iter().enumerate() {
kwbuf[i] = *c;
}
});
});

self.spi_master.read_write_bytes(self.kernel_write.take().unwrap(), self.kernel_read.take(), len);
}

After:

fn do_next_read_write(&self, app: &mut App) -> bool {
let count = self.kernel_write.map_or(0, |kwbuf| {
app.app_write.as_mut().map_or(0, |src| {
src.readonly_map_or(0, |buf| {
let mut rval = 0;
for (i, c) in buf[start..end].iter().enumerate() {
kwbuf[i] = *c;
rval = i + 1;
}
rval
})
})
});

if count > 0 {
self.spi_master.read_write_bytes(self.kernel_write.take().unwrap(), self.kernel_read.take(), len);
true
} else {
false
}
}

Code after we remove the Option:

fn do_next_read_write(&self, app: &mut App) -> bool {
let count = self.kernel_write.map_or(0, |kwbuf| {
app.app_write.readonly_map_or(0, |buf| {
let mut rval = 0;
for (i, c) in buf[start..end].iter().enumerate() {
kwbuf[i] = *c;
rval = i + 1;
}
rval
})
});

if count > 0 {
self.spi_master.read_write_bytes(self.kernel_write.take().unwrap(), self.kernel_read.take(), len);
true
} else {
false
}
}

Phil

———————
Philip Levis (he/him)
Associate Professor, Computer Science and Electrical Engineering
Faculty Director, lab64 Maker Space
Stanford University
http://csl.stanford.edu/~pal