implement review recomendations

components/fatfs/src/diskio.c

@@ -27,15 +27,16 @@ PARTITION VolToPart[] = {
     {1, 0}     /* Logical drive 1 ==> Physical drive 1, auto detection */
 };
 
-BYTE ff_disk_getpdrv()
+esp_err_t ff_diskio_get_drive(BYTE* out_pdrv)
 {
     BYTE i;
     for(i=0; i<_VOLUMES; i++) {
         if (!s_impls[i]) {
-            return i;
+            *out_pdrv = i;
+            return ESP_OK;
         }
     }
-    return 0xFF;
+    return ESP_ERR_NOT_FOUND;
 }
 
 void ff_diskio_register(BYTE pdrv, const ff_diskio_impl_t* discio_impl)

components/fatfs/src/diskio.h

@@ -58,16 +58,19 @@ typedef struct {
 } ff_diskio_impl_t;
 
 /**
- * Register diskio driver for given drive number.
+ * Register or unregister diskio driver for given drive number.
  *
  * When FATFS library calls one of disk_xxx functions for driver number pdrv,
  * corresponding function in discio_impl for given pdrv will be called.
  *
  * @param pdrv drive number
- * @param discio_impl  pointer to ff_diskio_impl_t structure with diskio functions
+ * @param discio_impl   pointer to ff_diskio_impl_t structure with diskio functions
+ *                      or NULL to unregister and free previously registered drive
  */
 void ff_diskio_register(BYTE pdrv, const ff_diskio_impl_t* discio_impl);
 
+#define ff_diskio_unregister(pdrv_) ff_diskio_register(pdrv_, NULL)
+
 /**
  * Register SD/MMC diskio driver
  *
@@ -79,9 +82,12 @@ void ff_diskio_register_sdmmc(BYTE pdrv, sdmmc_card_t* card);
 /**
  * Get next available drive number
  *
- * @return 0xFF on fail, else the drive number
+ * @param   out_pdrv            pointer to the byte to set if successful
+ *
+ * @return  ESP_OK              on success
+ *          ESP_ERR_NOT_FOUND   if all drives are attached
  */
-BYTE ff_disk_getpdrv();
+esp_err_t ff_diskio_get_drive(BYTE* out_pdrv);
 
 /* Disk Status Bits (DSTATUS) */
 

components/fatfs/src/esp_vfs_fat.h

@@ -51,18 +51,24 @@ esp_err_t esp_vfs_fat_register(const char* base_path, const char* fat_drive,
  * @note FATFS structure returned by esp_vfs_fat_register is destroyed after
  *       this call. Make sure to call f_mount function to unmount it before
  *       calling esp_vfs_fat_unregister.
+ *       This function is left for compatibility and will be changed in
+ *       future versions to accept base_path and replace the method below
  * @return
  *      - ESP_OK on success
  *      - ESP_ERR_INVALID_STATE if FATFS is not registered in VFS
  */
-esp_err_t esp_vfs_fat_unregister();
+esp_err_t esp_vfs_fat_unregister() __attribute__((deprecated));
 
 /**
  * @brief Un-register FATFS from VFS
  *
  * @note FATFS structure returned by esp_vfs_fat_register is destroyed after
  *       this call. Make sure to call f_mount function to unmount it before
- *       calling esp_vfs_fat_unregister.
+ *       calling esp_vfs_fat_unregister_ctx.
+ *       Difference between this function and the one above is that this one
+ *       will release the correct drive, while the one above will release
+ *       the last registered one
+ *
  * @param base_path     path prefix where FATFS is registered. This is the same
  *                      used when esp_vfs_fat_register was called
  * @return

components/fatfs/src/vfs_fat.c

@@ -65,12 +65,12 @@ static int vfs_fat_mkdir(void* ctx, const char* name, mode_t mode);
 static int vfs_fat_rmdir(void* ctx, const char* name);
 
 static vfs_fat_ctx_t* s_fat_ctxs[_VOLUMES] = { NULL, NULL };
-//compatibility
+//backwards-compatibility with esp_vfs_fat_unregister()
 static vfs_fat_ctx_t* s_fat_ctx = NULL;
 
-static unsigned char esp_vfs_fat_get_ctx(const char* base_path)
+static size_t find_context_index_by_path(const char* base_path)
 {
-    for(unsigned char i=0; i<_VOLUMES; i++) {
+    for(size_t i=0; i<_VOLUMES; i++) {
         if (s_fat_ctxs[i] && !strcmp(s_fat_ctxs[i]->base_path, base_path)) {
             return i;
         }
@@ -78,9 +78,9 @@ static unsigned char esp_vfs_fat_get_ctx(const char* base_path)
     return _VOLUMES;
 }
 
-static unsigned char esp_vfs_fat_get_empty_ctx()
+static size_t find_unused_context_index()
 {
-    for(unsigned char i=0; i<_VOLUMES; i++) {
+    for(size_t i=0; i<_VOLUMES; i++) {
         if (!s_fat_ctxs[i]) {
             return i;
         }
@@ -90,12 +90,12 @@ static unsigned char esp_vfs_fat_get_empty_ctx()
 
 esp_err_t esp_vfs_fat_register(const char* base_path, const char* fat_drive, size_t max_files, FATFS** out_fs)
 {
-    unsigned char ctx = esp_vfs_fat_get_ctx(base_path);
+    size_t ctx = find_context_index_by_path(base_path);
     if (ctx < _VOLUMES) {
         return ESP_ERR_INVALID_STATE;
     }
 
-    ctx = esp_vfs_fat_get_empty_ctx();
+    ctx = find_unused_context_index();
     if (ctx == _VOLUMES) {
         return ESP_ERR_NO_MEM;
     }
@@ -127,12 +127,8 @@ esp_err_t esp_vfs_fat_register(const char* base_path, const char* fat_drive, siz
         return ESP_ERR_NO_MEM;
     }
     fat_ctx->max_files = max_files;
-
+    strlcpy(fat_ctx->fat_drive, fat_drive, sizeof(fat_ctx->fat_drive) - 1);
-    strncpy(fat_ctx->fat_drive, fat_drive, sizeof(fat_ctx->fat_drive) - 1);
+    strlcpy(fat_ctx->base_path, base_path, sizeof(fat_ctx->base_path) - 1);
-    fat_ctx->fat_drive[sizeof(fat_ctx->fat_drive) - 1] = 0;
-
-    strncpy(fat_ctx->base_path, base_path, sizeof(fat_ctx->base_path) - 1);
-    fat_ctx->base_path[sizeof(fat_ctx->base_path) - 1] = 0;
 
     esp_err_t err = esp_vfs_register(base_path, &vfs, fat_ctx);
     if (err != ESP_OK) {
@@ -153,7 +149,7 @@ esp_err_t esp_vfs_fat_register(const char* base_path, const char* fat_drive, siz
 
 esp_err_t esp_vfs_fat_unregister_ctx(const char* base_path)
 {
-    unsigned char ctx = esp_vfs_fat_get_ctx(base_path);
+    size_t ctx = find_context_index_by_path(base_path);
     if (ctx == _VOLUMES) {
         return ESP_ERR_INVALID_STATE;
     }
@@ -249,14 +245,20 @@ static void file_cleanup(vfs_fat_ctx_t* ctx, int fd)
     memset(&ctx->files[fd], 0, sizeof(FIL));
 }
 
-#define vfs_fat_fix_path(ctx, path) \
+static void prepend_drive_to_path(void * ctx, const char * path, const char * path2){
-    char buf_ ## path[strlen(path)+strlen(((vfs_fat_ctx_t*)ctx)->fat_drive)+1]; \
+    static char buf[FILENAME_MAX+3];
-    sprintf(buf_ ## path,"%s%s", ((vfs_fat_ctx_t*)ctx)->fat_drive, path); \
+    static char buf2[FILENAME_MAX+3];
-    path = (const char *)buf_ ## path;
+    sprintf(buf, "%s%s", ((vfs_fat_ctx_t*)ctx)->fat_drive, path);
+    path = (const char *)buf;
+    if(path2){
+        sprintf(buf2, "%s%s", ((vfs_fat_ctx_t*)ctx)->fat_drive, path2);
+        path2 = (const char *)buf;
+    }
+}
 
 static int vfs_fat_open(void* ctx, const char * path, int flags, int mode)
 {
-    vfs_fat_fix_path(ctx, path);
+    prepend_drive_to_path(ctx, path, NULL);
     ESP_LOGV(TAG, "%s: path=\"%s\", flags=%x, mode=%x", __func__, path, flags, mode);
     vfs_fat_ctx_t* fat_ctx = (vfs_fat_ctx_t*) ctx;
     _lock_acquire(&fat_ctx->lock);
@@ -366,7 +368,7 @@ static int vfs_fat_fstat(void* ctx, int fd, struct stat * st)
 
 static int vfs_fat_stat(void* ctx, const char * path, struct stat * st)
 {
-    vfs_fat_fix_path(ctx, path);
+    prepend_drive_to_path(ctx, path, NULL);
     FILINFO info;
     FRESULT res = f_stat(path, &info);
     if (res != FR_OK) {
@@ -396,7 +398,7 @@ static int vfs_fat_stat(void* ctx, const char * path, struct stat * st)
 
 static int vfs_fat_unlink(void* ctx, const char *path)
 {
-    vfs_fat_fix_path(ctx, path);
+    prepend_drive_to_path(ctx, path, NULL);
     FRESULT res = f_unlink(path);
     if (res != FR_OK) {
         ESP_LOGD(TAG, "%s: fresult=%d", __func__, res);
@@ -408,8 +410,7 @@ static int vfs_fat_unlink(void* ctx, const char *path)
 
 static int vfs_fat_link(void* ctx, const char* n1, const char* n2)
 {
-    vfs_fat_fix_path(ctx, n1);
+    prepend_drive_to_path(ctx, n1, n2);
-    vfs_fat_fix_path(ctx, n2);
     const size_t copy_buf_size = 4096;
     void* buf = malloc(copy_buf_size);
     if (buf == NULL) {
@@ -464,8 +465,7 @@ fail1:
 
 static int vfs_fat_rename(void* ctx, const char *src, const char *dst)
 {
-    vfs_fat_fix_path(ctx, src);
+    prepend_drive_to_path(ctx, src, dst);
-    vfs_fat_fix_path(ctx, dst);
     FRESULT res = f_rename(src, dst);
     if (res != FR_OK) {
         ESP_LOGD(TAG, "%s: fresult=%d", __func__, res);
@@ -477,7 +477,7 @@ static int vfs_fat_rename(void* ctx, const char *src, const char *dst)
 
 static DIR* vfs_fat_opendir(void* ctx, const char* name)
 {
-    vfs_fat_fix_path(ctx, name);
+    prepend_drive_to_path(ctx, name, NULL);
     vfs_fat_dir_t* fat_dir = calloc(1, sizeof(vfs_fat_dir_t));
     if (!fat_dir) {
         errno = ENOMEM;
@@ -582,7 +582,7 @@ static void vfs_fat_seekdir(void* ctx, DIR* pdir, long offset)
 static int vfs_fat_mkdir(void* ctx, const char* name, mode_t mode)
 {
     (void) mode;
-    vfs_fat_fix_path(ctx, name);
+    prepend_drive_to_path(ctx, name, NULL);
     FRESULT res = f_mkdir(name);
     if (res != FR_OK) {
         ESP_LOGD(TAG, "%s: fresult=%d", __func__, res);
@@ -594,7 +594,7 @@ static int vfs_fat_mkdir(void* ctx, const char* name, mode_t mode)
 
 static int vfs_fat_rmdir(void* ctx, const char* name)
 {
-    vfs_fat_fix_path(ctx, name);
+    prepend_drive_to_path(ctx, name, NULL);
     FRESULT res = f_unlink(name);
     if (res != FR_OK) {
         ESP_LOGD(TAG, "%s: fresult=%d", __func__, res);

components/fatfs/src/vfs_fat_sdmmc.c

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include <stdlib.h>
+#include <string.h>
 #include "esp_log.h"
 #include "esp_vfs.h"
 #include "esp_vfs_fat.h"
@@ -23,6 +24,7 @@
 static const char* TAG = "vfs_fat_sdmmc";
 static sdmmc_card_t* s_card = NULL;
 static uint8_t s_pdrv = 0;
+static char * s_base_path = NULL;
 
 esp_err_t esp_vfs_fat_sdmmc_mount(const char* base_path,
     const sdmmc_host_t* host_config,
@@ -36,6 +38,20 @@ esp_err_t esp_vfs_fat_sdmmc_mount(const char* base_path,
     if (s_card != NULL) {
         return ESP_ERR_INVALID_STATE;
     }
+
+    // connect SDMMC driver to FATFS
+    BYTE pdrv = 0xFF;
+    if (ff_diskio_get_drive(&pdrv) != ESP_OK || pdrv == 0xFF) {
+        ESP_LOGD(TAG, "the maximum count of volumes is already mounted");
+        return ESP_ERR_NO_MEM;
+    }
+
+    s_base_path = strdup(base_path);
+    if(!s_base_path){
+        ESP_LOGD(TAG, "could not copy base_path");
+        return ESP_ERR_NO_MEM;
+    }
+
     // enable SDMMC
     sdmmc_host_init();
 
@@ -56,12 +72,6 @@ esp_err_t esp_vfs_fat_sdmmc_mount(const char* base_path,
         *out_card = s_card;
     }
 
-    // connect SDMMC driver to FATFS
-    BYTE pdrv = ff_disk_getpdrv();
-    if (pdrv == 0xFF) {
-        ESP_LOGD(TAG, "the maximum count of volumes is already mounted");
-        goto fail;
-    }
     ff_diskio_register_sdmmc(pdrv, s_card);
     s_pdrv = pdrv;
     char drv[3] = {(char)('0' + pdrv), ':', 0};
@@ -114,6 +124,7 @@ esp_err_t esp_vfs_fat_sdmmc_mount(const char* base_path,
 fail:
     free(workbuf);
     esp_vfs_fat_unregister_ctx(base_path);
+    ff_diskio_unregister(pdrv);
     free(s_card);
     s_card = NULL;
     return err;
@@ -128,8 +139,12 @@ esp_err_t esp_vfs_fat_sdmmc_unmount()
     char drv[3] = {(char)('0' + s_pdrv), ':', 0};
     f_mount(0, drv, 0);
     // release SD driver
+    ff_diskio_unregister(s_pdrv);
     free(s_card);
     s_card = NULL;
     sdmmc_host_deinit();
-    return esp_vfs_fat_unregister();
+    esp_err_t err = esp_vfs_fat_unregister_ctx(s_base_path);
+    free(s_base_path);
+    s_base_path = NULL;
+    return err;
 }