extmod/vfs_blockdev: Implement common helper for read and write.

- Code size saving as all of these functions are very similar. - Resolves the "TODO" of the plain read and write functions not propagating errors. An error in the underlying block device now causes VFatFs to return EIO, for example. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <[email protected]>
picoscratch · Sep 26, 2024 · f4ab9d9 · f4ab9d9
1 parent 4f6d4b2
commit f4ab9d9
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 51 deletions.
diff --git a/extmod/vfs_blockdev.c b/extmod/vfs_blockdev.c
@@ -32,19 +32,6 @@
 
 #if MICROPY_VFS
 
-// Block device functions are expected to return 0 on success
-// and negative integer on errors. Check for positive integer
-// results as some callers (i.e. littlefs) will produce corrupt
-// results from these.
-static int mp_vfs_check_result(mp_obj_t ret) {
-    if (ret == mp_const_none) {
-        return 0;
-    } else {
-        int i = MP_OBJ_SMALL_INT_VALUE(ret);
-        return i > 0 ? (-MP_EINVAL) : i;
-    }
-}
-
 void mp_vfs_blockdev_init(mp_vfs_blockdev_t *self, mp_obj_t bdev) {
     mp_load_method(bdev, MP_QSTR_readblocks, self->readblocks);
     mp_load_method_maybe(bdev, MP_QSTR_writeblocks, self->writeblocks);
@@ -59,27 +46,38 @@ void mp_vfs_blockdev_init(mp_vfs_blockdev_t *self, mp_obj_t bdev) {
     }
 }
 
+// Helper function to minimise code size of read/write functions
+// note the n_args argument is moved to the end for further code size reduction (args keep same position in caller and callee).
+static int mp_vfs_blockdev_call_rw(mp_obj_t *args, size_t block_num, size_t block_off, size_t len, void *buf, size_t n_args) {
+    mp_obj_array_t ar = {{&mp_type_bytearray}, BYTEARRAY_TYPECODE, 0, len, buf};
+    args[2] = MP_OBJ_NEW_SMALL_INT(block_num);
+    args[3] = MP_OBJ_FROM_PTR(&ar);
+    args[4] = MP_OBJ_NEW_SMALL_INT(block_off); // ignored for n_args == 2
+    mp_obj_t ret = mp_call_method_n_kw(n_args, 0, args);
+
+    if (ret == mp_const_none) {
+        return 0;
+    } else {
+        // Block device functions are expected to return 0 on success
+        // and negative integer on errors. Check for positive integer
+        // results as some callers (i.e. littlefs) will produce corrupt
+        // results from these.
+        int i = MP_OBJ_SMALL_INT_VALUE(ret);
+        return i > 0 ? (-MP_EINVAL) : i;
+    }
+}
+
 int mp_vfs_blockdev_read(mp_vfs_blockdev_t *self, size_t block_num, size_t num_blocks, uint8_t *buf) {
     if (self->flags & MP_BLOCKDEV_FLAG_NATIVE) {
         mp_uint_t (*f)(uint8_t *, uint32_t, uint32_t) = (void *)(uintptr_t)self->readblocks[2];
         return f(buf, block_num, num_blocks);
     } else {
-        mp_obj_array_t ar = {{&mp_type_bytearray}, BYTEARRAY_TYPECODE, 0, num_blocks *self->block_size, buf};
-        self->readblocks[2] = MP_OBJ_NEW_SMALL_INT(block_num);
-        self->readblocks[3] = MP_OBJ_FROM_PTR(&ar);
-        mp_call_method_n_kw(2, 0, self->readblocks);
-        // TODO handle error return
-        return 0;
+        return mp_vfs_blockdev_call_rw(self->readblocks, block_num, 0, num_blocks * self->block_size, buf, 2);
     }
 }
 
 int mp_vfs_blockdev_read_ext(mp_vfs_blockdev_t *self, size_t block_num, size_t block_off, size_t len, uint8_t *buf) {
-    mp_obj_array_t ar = {{&mp_type_bytearray}, BYTEARRAY_TYPECODE, 0, len, buf};
-    self->readblocks[2] = MP_OBJ_NEW_SMALL_INT(block_num);
-    self->readblocks[3] = MP_OBJ_FROM_PTR(&ar);
-    self->readblocks[4] = MP_OBJ_NEW_SMALL_INT(block_off);
-    mp_obj_t ret = mp_call_method_n_kw(3, 0, self->readblocks);
-    return mp_vfs_check_result(ret);
+    return mp_vfs_blockdev_call_rw(self->readblocks, block_num, block_off, len, buf, 3);
 }
 
 int mp_vfs_blockdev_write(mp_vfs_blockdev_t *self, size_t block_num, size_t num_blocks, const uint8_t *buf) {
@@ -92,12 +90,7 @@ int mp_vfs_blockdev_write(mp_vfs_blockdev_t *self, size_t block_num, size_t num_
         mp_uint_t (*f)(const uint8_t *, uint32_t, uint32_t) = (void *)(uintptr_t)self->writeblocks[2];
         return f(buf, block_num, num_blocks);
     } else {
-        mp_obj_array_t ar = {{&mp_type_bytearray}, BYTEARRAY_TYPECODE, 0, num_blocks *self->block_size, (void *)buf};
-        self->writeblocks[2] = MP_OBJ_NEW_SMALL_INT(block_num);
-        self->writeblocks[3] = MP_OBJ_FROM_PTR(&ar);
-        mp_call_method_n_kw(2, 0, self->writeblocks);
-        // TODO handle error return
-        return 0;
+        return mp_vfs_blockdev_call_rw(self->writeblocks, block_num, 0, num_blocks * self->block_size, (void *)buf, 2);
     }
 }
 
@@ -106,13 +99,7 @@ int mp_vfs_blockdev_write_ext(mp_vfs_blockdev_t *self, size_t block_num, size_t
         // read-only block device
         return -MP_EROFS;
     }
-
-    mp_obj_array_t ar = {{&mp_type_bytearray}, BYTEARRAY_TYPECODE, 0, len, (void *)buf};
-    self->writeblocks[2] = MP_OBJ_NEW_SMALL_INT(block_num);
-    self->writeblocks[3] = MP_OBJ_FROM_PTR(&ar);
-    self->writeblocks[4] = MP_OBJ_NEW_SMALL_INT(block_off);
-    mp_obj_t ret = mp_call_method_n_kw(3, 0, self->writeblocks);
-    return mp_vfs_check_result(ret);
+    return mp_vfs_blockdev_call_rw(self->writeblocks, block_num, block_off, len, (void *)buf, 3);
 }
 
 mp_obj_t mp_vfs_blockdev_ioctl(mp_vfs_blockdev_t *self, uintptr_t cmd, uintptr_t arg) {

diff --git a/tests/extmod/vfs_blockdev_invalid.py b/tests/extmod/vfs_blockdev_invalid.py
@@ -52,6 +52,8 @@ def ioctl(self, op, arg):
 
 def test(vfs_class):
     print(vfs_class)
+    bdev.read_res = 0  # reset function results
+    bdev.write_res = 0
 
     vfs_class.mkfs(bdev)
     fs = vfs_class(bdev)
@@ -84,4 +86,4 @@ def test(vfs_class):
 
 
 test(vfs.VfsLfs2)
-test(vfs.VfsFat)  # Looks like most failures of underlying device are ignored by VFAT currently
+test(vfs.VfsFat)
diff --git a/tests/extmod/vfs_blockdev_invalid.py.exp b/tests/extmod/vfs_blockdev_invalid.py.exp
@@ -105,22 +105,22 @@ readblocks
 read 1 a
 read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
 readblocks
-opened
+OSError [Errno 5] EIO
 readblocks
-read 1 a
-read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
 readblocks
-opened
+OSError [Errno 5] EIO
 readblocks
-read 1 a
-read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+OSError [Errno 5] EIO
 readblocks
-opened
 readblocks
-read 1 a
-read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+OSError [Errno 5] EIO
 readblocks
-opened
+OSError [Errno 5] EIO
 readblocks
-read 1 a
-read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+readblocks
+OSError [Errno 5] EIO
+readblocks
+OSError [Errno 5] EIO
+readblocks
+readblocks
+OSError [Errno 5] EIO