Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esp8266: Added os.statvfs() to get filesystem status. #2413

Closed
wants to merge 3 commits into from

Conversation

@hosaka
Copy link
Contributor

commented Sep 12, 2016

From forum discussion.

Example use case (markxr):

  • Check free fs space
  • If less than some threshold, delete the oldest file
  • Write data to new file....

os.getfree(path) returns the free space available in the VFS. Is the block size assumed to be 512 bit? Not sure about leaving it hardcoded.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

Is the block size assumed to be 512 bit?

I think what you did is correct. The block size is 512 * csize.

Any reason why you didn't copy stmhal's implementation of os.statvfs? That function is provided by CPython as well so is the Pythonic was to query the free space on a filesystem.

@robert-hh

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2016

In my opinion statvfs would be the better choice, since it provides more information. For the coding in vfs_fat.c, the function stat is a good template, since it returns a list too.

@hosaka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

I didn't copy the stmhal version since other parameters that are returned in the tuple go beyond just "free space" info. It does provide more information, but is it useful for a simple function that only needs to return the space available? But I can add the rest certainly.

@hosaka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

If you agree that keeping the function called statvfs that returns all the info is the right way, I will copy the stmhal's version this evening.

@robert-hh

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2016

I think that the other information is useful too, especially if is provided anyhow. Compare it to the stat function for files. Besides the file size, it also returs other information like the file type and timestamps. If you just need the free space on the filesystem, just pick that value from the returned tuple. The initial request was "How so I determine the free space in the file system". That will be met.

@hosaka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

That is a fair point. The format that statvfs returns in stmhal is the same as cpython, as Damien said. I will update the PR later today. Thanks for the advice.

@hosaka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

I would also suggest to add the behaviour similar to listdir() that takes no parameters and sets the path to an empty string.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2016

@dpgeorge: I implemented a similar "get number of free blocks" VFS call in the private tree back in early days. It wasn't merged to master because there were a lot of things to decide: should it be a separate call or statvfs-like, how to name it, what units it returns value in, etc. The reason I went for a separate call for # of free blocks is because I'd consider statvfs heavy-weight.

@hosaka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

This was also the reason for my first version, returning only a number of free blocks in the filesystem and nothing else. Other fields such as blocks free to the unprivileged user seemed less useful.

@robert-hh

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2016

Surely there is data in the list which is not meaningful for Pyboard or ESP8266, but in favour of statvfs is compatibility and especially consistency across the ports.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2016

but in favour of statvfs is compatibility and especially consistency across the ports.

The problem with statvfs is that it requires allocating memory for tuple, and that filling it in may require multiple (expensive?) calls to the underlying VFS. But then public interface will for sure will be os.statvfs(), it will just either call .statvfs() method of the underlying VFS, or few adhoc methods (like .getfree()) of underlying VFS. In this regard, having VFS.statvfs() makes sense. A case that some app may call adhoc methods for extra efficiency can probably be disregarded. There's still issue of "big" tuple allocation on each call to VFS.statvfs().

@dhylands

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2016

Sure - unpriviledged and priviledged users are the same on ESP8266 and pyboard, which is why it returns the same numbers for both. inode information is meaningless for FATFS, so it returns zero )I'm pretty sure linux does the same thing).

The only piece of information that I couldn't easily figure out was the total size, so I used zero for the time being.

@hosaka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2016

Total of fs sectors would be (fatfs->n_fatend - 2) * fatfs->ssize, times the sector size would give us the total size.
Reading the fatfs docs

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

I would strongly urge to have os.statvfs() as the user-facing method to use, for the usual reasons: compatibility with CPython, one less difference for people to learn, one less difference to document, etc.

Also, as said above, statvfs() provides extra info like total size of the FS (if possible to determine), and if we ever support ext2fs (I have a simple read-only driver somewhere...) then free inodes can be useful to know. Oh, and MAX_LFN (size of filename) is also exposed via this function.

In terms of implementing it, we could have the underlying VFS method call take an input buffer/tuple which it just fills (ie does no allocation), something like: vfs_statvfs(mp_obj_t self, mp_obj_t items[10]);

@dhylands

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

I know that the c-runtime library added _r versions of a variety of functions to make them threadsafe. The _r versions take a state variable so that each thread can have its own state.

Perhaps we could do something similar and create _into variants of some functions. So statvfs_into would fill in a previously allocated tuple, and statvfs would allocate and return a buffer (as per existing cpython).

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

Perhaps we could do something similar and create _into variants of some functions. So statvfs_into would fill in a previously allocated tuple, and statvfs would allocate and return a buffer (as per existing cpython).

Yes, this would following the pattern of readinto and friends. But it'd need to take a list; tuples are immutable :) But really I think adding statvfs_into is an over-optimisation at this point.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

But really I think adding statvfs_into is an over-optimisation at this point.

That's the concern I have with additions like that - they're not well-thought out, and may need rework later, which may change interface. And we have too conservative policy regarding changing interfaces (based on #2217 and various previous discussions; and yes, to clarify, I feel that we have not conservative enough policy for additions, and too conservative policy for changes).

And to clarify, I don't talk about os.statvfs_into(). It's definitely not needed now. I'm talking about VFS interface (which is MicroPython's adhoc interface). That really could take a reference to list to populate. Except that of course at current stage, it doesn't make sense to burden @hosaka to implement all code for that. But the point is that if later it's found to be needed, the expected action should be is to remove VFS.statvfs() and add VFS.statvfs_into(). (And for port like esp8266, any extra memory alloc is problematic, I established my mindset around that, and would like others to think about that too.)

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

But the point is that if later it's found to be needed, the expected action should be is to remove VFS.statvfs() and add VFS.statvfs_into()

Sure, no problem, the VFS interface is considered experimental at this stage. But uos.statvfs() will always remain, do you agree?

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2016

Sure.

So, I'm +1 on this patch (except it's worth paying attention to decreased coverage and add a testcase for it).

@hosaka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2016

I can add the testcase/update the docs in a separate patch, or add it to this one. Is it worth adding the total filesystem sectors info as I mentioned above?

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2016

I'd say a testcase (to existing test) should go as a separate commit in this patch, other things could go as separate pull requests for when you have time to implement them. Thanks.

@hosaka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2016

Will it be sufficient to just check the len() of the returned tuple is 10 in the testcase? All other parameters returned will be different for each port.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2016

The test for VFS functionality, https://github.com/micropython/micropython/blob/master/tests/extmod/vfs_fat_ramdisk.py, sets up specific test environment which should be overall stable, so you should check the actual value(s) returned.


mp_obj_tuple_t *t = MP_OBJ_TO_PTR(mp_obj_new_tuple(10, NULL));

t->items[0] = MP_OBJ_NEW_SMALL_INT(fatfs->csize * 512); // f_bsize - block size

This comment has been minimized.

Copy link
@dpgeorge

dpgeorge Sep 16, 2016

Member

I think the 512 should be fatfs->ssize. On stmhal ssize does not exist and 512 should be used instead, but on esp8266 you need to use ssize because it has a larger block size (4096 bytes).

This comment has been minimized.

Copy link
@hosaka

hosaka Sep 21, 2016

Author Contributor

You are right, I have actually tried ssize in my local branch. I will add that to the next commit, together with the total number of blocks value. Thanks for pointing it out!

@hosaka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2016

Testcase has been updated, as well as the total vfs size (total == free blocks on an empty VFS). If this gets merged, I can also update the docs.

@hosaka hosaka changed the title esp8266: Added os.getfree() to get available filesystem space. esp8266: Added os.statvfs() to get filesystem status. Sep 25, 2016
@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

Thank you! Merged in 4fb72fe.

I can also update the docs.

Yes please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.