extmod/vfs_fat: Checks for remove() and rmdir().

[hosaka]

Please note, I haven't run the tests on this yet.
I noticed the TODOs when I was working on the last PR.

I've split the unlink function to avoid repeating the code. From what I see, there is no fatfs attribute that tells us it's a file, i.e if it's not a directory it's a file (example of readdir()).

I am planning to continue to improve the coverage of vfs_fat.c after this approved/discarded.

[hosaka]

moduerrno.c does have EISDIR but doesn't have ENOTDIR error definition, can this be added to the common list?

[pfalcon]

Not to write a test, sorry ;-). We don't want testsuite to bloat production builds ;-). (So, it can be added - when many (at least few) people report good usecases for having it.)

[hosaka]

Very reasonable! I will keep the "20" and a comment for the time being :)

[hosaka]

I have squashed the commits and changed the asserts to prints, including the existing ones.

[dpgeorge]

It looks like you can combine the f_stat functionality for both remove and rmdir. You just need a flag as an argument to the common functionto check if it is/isn't a dir. Eg:

if ((fno.fattrib & AM_DIR) == wanted_attr) {
    return unlink;
} else {
    raise exception;
}

and wanted_attr would be 0 for remove and AM_DIR for rmdir.

Also, for the changes to the tests that turn assert to print, please split that into a separate patch, thanks.

[dpgeorge]

This can fit on one line (it's less than 100 chars).

[dpgeorge]

Please use MP_EISDIR.

[hosaka]

Thanks for the suggestions Damien! I've combined the f_stat checks in the unlink function. I didn't want to just pass 0 as an attribute, so !AM_DIR will always resolve to 0 instead.

I will submit a separate patch for asserts to prints before I carry on with more vfs_fat test coverage once this PR is closed.

[pfalcon]

I'd find introduction of a function named fat_vfs_unlink() to be confusing. In ANSI C/POSIX, remove() and unlink() are similar, but slightly different, and difference is the opposite of what fat_vfs_remove() vs fat_vfs_unlink() do. We don't implement unlink(), and from all the above, there's no reason to have fat_vfs_unlink() function. If you need to factor internal work function for fat_vfs_remove(), please call it fat_vfs_remove_internal().

I didn't want to just pass 0 as an attribute, so !AM_DIR will always resolve to 0 instead.

And yet "attribute" is numeric, even bitmask value, while !AM_DIR is a boolean. Using the latter instead of the former is quite confusing. If you need zero attribute value, please use just that (feel free to add comment if you'd like to clarify something regarding it).

[hosaka]

Hi Paul, thanks for the suggestions. You're quite right about the attribute, I will change it to a zero and add a comment instead.

Regarding the function name, in some modules, functions like path_equal() in vfs don't follow a naming convention, which is good and bad at the same time, it shows that they don't belong to the module but also have a freeform name. Perhaps having a underscore prefix for local functions only could be a good convention? I will rename the function as you suggested.

[dhylands]

In C, all symbols with leading underscores are reserved for the runtime library.
https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html
so it is best to avoid those.

[hosaka]

Oh yes, I completely forgot about that, seeing it being used in python!

[dpgeorge]

Merged in d02f3a5 and f274561 with some minor changes to reduce code size (mp_obj_str_get_str only called once in remove_internal, instead of in remove and rmdir).

Thanks, please proceed with next parts!

[hosaka]

Thanks Damien, I'll be looking into more vfs coverage from now on.