'freespace' argument for 'session-get' RPC method

[taem]

Hello,

Please take a look at the proposed implementation of 'freespace' argument for 'session-get' RPC method. This argument returns available disk space (in KiB) for the default download directory for new torrents.

Am I going the right way?

Thanks.

[taem]

Display available disk space in the web interface

[taem]

Add 'freespace' argument for 'session-get' RPC method

[reardon]

This is my current top feature request--I already wrote my own patch but not as clean (via platform.c) as this.

But why not use the POSIX function on Darwin? Then for WIN32 use GetDiskFreeSpaceEx?().

In rpcimpl.c, do a check for illegal value for freespace and only add the response term if you have a legal value (ie not -1).

Suggest you change the dictionary term to "download-dir-freespace" or "download-dir-df". Also recommend you just return the value in bytes and let the local formatter decide what to do. Though since you have to go thru JSON you will be constained to 2⁵³ instead of 2⁶⁴

[reardon]

Sorry I guess this really belongs at #1212. I for one do not need an elaborate solution that includes quota info. Rather I just want available space in the download-dir, as is proposed here.

[charles]

reardon, do you want to revise this ticket's patch to add the WIN32 and POSIX changes you described? Seems like win32 support in this ticket would be a nice-to-have. I'm not sure which function you're referring to for Darwin.

[reardon]

Agree, WIN32 is nice-to-have. I don't have Win32 compilation ability, so I will only write pseudo-code and submit as comments.

By POSIX function, I meant the same statvfs() that is used for Linux et al. Darwin supports it so I don't see a reason to special-case Darwin.

I will modify taem's patch tonight if it hasn't been done already.

[reardon]

Please see revised patch. This includes support in transmission-remote but does not update rpc-spec

[jordan]

adding rb07 the CC list. he's been doing the Qt client builds on Windows and might be able to verify or correct the GetDiskFreeSpaceEx?() part of this patch.

[rb07]

Replying to jordan:

adding rb07 the CC list. he's been doing the Qt client builds on Windows and might be able to verify or correct the GetDiskFreeSpaceEx() part of this patch.

The commented part looks fine, I would only add the missing include and change the variable type to uint64_t (Linux uses unsigned long on struct statvfs).

Answering the questions on the code's comments:

• You don't need to convert to UTF-16, unless path is really a UTF-8 string, but then something else has to be done with the build (i.e. outside the code, to switch Windows API). Currently there is no handling of UTF-8 on Windows (other than what Qt does), so there are no conversions to UTF-16 on libtransmission; this could change when full support for foreign locales is added (Linux doesn't have it either).

• AFAIK the path doesn't need the trailing back slash, that is building with MinGW; I haven't tested using shared drives or other UNC paths.

Since MinGW doesn't have statvfs() your solution is fine.

#ifndef WIN32 /* POSIX */ 
 #include <sys/statvfs.h> 
#else
 #include <windows.h>   /* or specifically <winbase.h> */
#endif

...

static uint64_t 
getFreeSpace( const char * path )
{
#ifdef WIN32

 uint64_t freeBytesAvailable = 0;

 if ( GetDiskFreeSpaceEx(path, &freeBytesAvailable, NULL, NULL) )
 {
     return freeByteAvailable;
 }
 else
 {
     return -1;
 }
#else /* POSIX */

...

[reardon]

<windows.h> is already included in platform.c so I think that would be redundant.

[jordan]

very nice.

rb07: thanks for letting yourself be volunteered :)

[rb07]

There's an unreachable "return -1" at the end of the WIN32 section on the latest patch, and you may get a warning for returning uint64_t when the function is still defined as int64_t.

[reardon]

If we want to maintain inline results then we are stuck with the uint->int conversion. gcc does not complain, fwiw.

[reardon]

wrong diff, re-up

[jordan]

Another revision. Changes this time:

• Added rpc spec documentation
• Instead of omitting the rpc key, send it with a value of -1 when it can't be calculated
• Added autoconf check for statvfs()
• Avoid possible overflow when multiplying buf.f_bavail and buf.f_bsize

Please let me know if I've added any bugs in this patch. If it looks good to everyone I'll check it in.

[reardon]

Small glitch in remote.c, I think you changed all of the other references to 'download-dir-free-space' (with hyphen) except here. Works fine with the change.

[jordan]

04:41:22 < CIA-44> jordan * r11627 /trunk/ (8 files in 4 dirs): (trunk, daemon) #3833 "'freespace' argument for 'session-get' RPC method" -- committing patch from taem, reardon, and rb07

[taem]

Hi all,

Sorry. I'm just returned from the holidays. Thanks for applying.

[Elbandi]

Hi,

I'd like only to indicate, this report wrong info if the quota is set. but quota query is a bit complex :(

[taem]

Replying to Elbandi:

Hi,

I'd like only to indicate, this report wrong info if the quota is set. but quota query is a bit complex :(

Hi,

You are right. I'm currently investigating this issue.

[taem]

Replying to Elbandi:

Hi,

I'd like only to indicate, this report wrong info if the quota is set. but quota query is a bit complex :(

Elbandi, can you please test patch. I've added check for user quota. I'm not sure what to do when quota isn't enabled or there is error on getting quota info:

int64_t
tr_sessionGetDownloadDirFreeSpace( const tr_session * session )
{
    assert( tr_isSession( session ) );

    /* Check user quota first */
    {
        int64_t freespace;

        freespace = tr_getQuotaFreeSpace( session->downloadDir );
        /* If quota isn't enabled or there is error on getting its info */
        /* proceed to check available space by the usual way */
        if (freespace >= 0)
        {
            return freespace;
        }
    }

    return tr_getFreeSpace( session->downloadDir );
}

Also there are some dirty code to determine special block device, which is needed by the quotactl().

[taem]

Tested on Linux with local file system only.

[Elbandi]

i got some semester exams, i cannot test now. I added some tweak to your patch (malloc+strcpy -> tr_strdup): ​http://transmission.pastebin.com/WJzMGXgJ

But ​man getmntent says:

The getmntent() function reads the next line from the file system description file
fp and returns a pointer to a structure containing the broken out fields from
a line in the file. The pointer points to a static area of memory which is
overwritten by subsequent calls to getmntent(). 

so the returned mnt_fsname can used as device: ​http://transmission.pastebin.com/CqMu7jT1

(not tested, it's only a idea)

For quota ​man quotactl:

ESRCH 
No disc quota is found for the indicated user. 
Quotas have not been turned on for this filesystem. 

the second else in getquota is not nessesary.

[taem]

Replying to Elbandi:

i got some semester exams, i cannot test now. I added some tweak to your patch (malloc+strcpy -> tr_strdup): ​http://transmission.pastebin.com/WJzMGXgJ

Cool. Less code. Better code.

But ​man getmntent says:

The getmntent() function reads the next line from the file system description file
fp and returns a pointer to a structure containing the broken out fields from
a line in the file. The pointer points to a static area of memory which is
overwritten by subsequent calls to getmntent(). 

so the returned mnt_fsname can used as device: ​http://transmission.pastebin.com/CqMu7jT1

(not tested, it's only a idea)

It's works. Thanks.

For quota ​man quotactl:

ESRCH 
No disc quota is found for the indicated user. 
Quotas have not been turned on for this filesystem. 

the second else in getquota is not nessesary.

Unfortunaly, both dqb_bsoftlimit=0 and dqb_bhardlimit=0 are valid. I guess it's relates to

The dqb_valid field is a bit mask that is set to indicate the entries in the dqblk structure that are valid. Currently, the kernel fills in all entries of the dqblk structure and marks them as valid in the dqb_valid field.

From the quotactl(2) linux manpage. I suppose, zero = quota is not enabled.

Please find attached the refreshed patch.

[jordan]

getblkdev(), getdev(), and getquota() should all be static functions

Other than that, is everyone who's contributed to this happy with freespace2.diff + freespacequota.patch?

I'd like to get this in before the 2.20 freeze :)

[rb07]

Replying to jordan:

Other than that, is everyone who's contributed to this happy with freespace2.diff + freespacequota.patch?

The Windows part doesn't need anything on the freespacequota.patch, the tr_getFreeSpace() function already returns the amount available to the user.

Quoting from the Microsoft manual:

"A pointer to a variable that receives the total number of free bytes on a disk that are available to the user who is associated with the calling thread.

...

If per-user quotas are being used, this value may be less than the total number of free bytes on a disk."

[taem]

Replying to jordan:

Other than that, is everyone who's contributed to this happy with freespace2.diff + freespacequota.patch?

Yes.

[taem]

Replying to rb07:

The Windows part doesn't need anything on the freespacequota.patch, the tr_getFreeSpace() function already returns the amount available to the user.

Perhaps, '#ifndef WIN32' should be moved to tr_sessionGetDownloadDirFreeSpace() from tr_getQuotaFreeSpace().

[taem]

I just found a bug in daemon/remote.c:

if( tr_bencDictFindInt( args,  "download-dir-free-space", &i ) ) 
    printf( "  Download directory free space: %s\n",  strlsize( buf, i, sizeof buf ) );

static char*
strlsize( char * buf, int64_t bytes, size_t buflen )
{
    if( !bytes )
        tr_strlcpy( buf, "None", buflen );
    else
        tr_formatter_size_B( buf, bytes, buflen );

    return buf;
}

strlsize() passes int64_t type value to tr_formatter_size_B()'s uint64_t type argument (bytes).

[taem]

Also, what do you think about to add available disk space info for incomplete directory?

[Elbandi]

Two things came to my mind:
first, if the torrentdir is a long path (with lots of '/'), the getdev reads the mtab/mounts many times. (mean: /this/is/the/torrent/path => 5 reads).
seconds, all sessionget with download-dir-free-space params need to resolve the quota device. this is unnecessary. the quota device is change, only when torrentdir is changed. (so we sould "cache" this device path, for example a RO variable near torrentdir).

[taem]

Please find attached proposed patch which adds free space info for torrent download directory.

Rationale: the torrent and the default download directories can be different.

[taem]

Replying to Elbandi:

Two things came to my mind:
first, if the torrentdir is a long path (with lots of '/'), the getdev reads the mtab/mounts many times. (mean: /this/is/the/torrent/path => 5 reads).
seconds, all sessionget with download-dir-free-space params need to resolve the quota device. this is unnecessary. the quota device is change, only when torrentdir is changed. (so we sould "cache" this device path, for example a RO variable near torrentdir).

True.

[jordan]

Could someone attach a single diff that has all the changes needed to apply this to trunk? There are several revisions here which overlap.

[taem]

Replying to jordan:

Could someone attach a single diff that has all the changes needed to apply this to trunk? There are several revisions here which overlap.

I can do that. Add freespacetorrent.patch to the diff? This patch adds new torrent-get argument.

[taem]

Please find attached refreshed patch freespacequota.patch - freespacequota2.patch and refreshed freespacetorrent.patch - freespacetorrent2.patch. These patches refreshed against the trunk.

An issue described in comment:32 is in my todo for the next release.

Thanks.

[jordan]

A couple of thoughts/suggestions

• ​http://osdir.com/ml/apple.fink.devel/2003-03/msg00309.html says "OS X doesn't have a getmntent; use getmntinfo() instead. Autoconf provides a AC_FUNT_GETMNTENT macro to help find getmntent or let you know if it's not present." Not building properly on BSD / OS X is a showstopper... can this be fixed?

• Since both session.c and torrent.c use tr_getFreeSpace() and tr_getQuotaFreeSpace() in the exact same way, I suggest we rename tr_getFreeSpace() as tr_getDiskFreeSpace(), making both it and tr_getQuotaFreeSpace() private to platform.c, and adding a public wrapper function tr_getFreeSpace():

platform.h:

    /** If the disk quota is enabled and readable, this returns how much is available in the quota.
        Otherwise, it returns how much is available on the disk, or -1 on error. */
    int64_t tr_getFreeSpace( const char * path );

platform.c:

    int64_t
    tr_getFreeSpace( const char * path )
    {
        int64_t i = tr_getQuotaFreeSpace( path );
        if( i < 0 )
            i = tr_getDiskFreeSpace( path );
        return i;
    }

torrent.c:

    int64_t
    tr_torrentGetDownloadDirFreeSpace( const tr_torrent * tor )
    {
        assert( tr_isTorrent( tor  ) );

        return tr_getFreeSpace( tor->downloadDir );
    }

session.c:

    int64_t
    tr_sessionGetDownloadDirFreeSpace( const tr_session * session )
    {
        assert( tr_isSession( session ) );

        return tr_getFreeSpace( session->downloadDir );
    }

[taem]

Not building properly on BSD / OS X I hope it fixed.
Add a public wrapper function tr_getFreeSpace() fixed.

Please find attached patches.

[jordan]

bah, this should have gone in 2.20 but it's too late now.

[taem]

Hi,

Please find attached patches that fix an issue described in the comment:32.

0001-Save-download-dir-block-device-for-the-next-tr_sessi.patch depends on freespacequota3.patch. 0001-Save-download-dir-block-device-for-the-next-tr_torre.patch depends on freespacetorrent3.patch.

[taem]

Please find attached patch freespace4.patch. It's combines freespacequota3.patch, freespacetorrent3.patch, 0001-Save-download-dir-block-device-for-the-next-tr_sessi.patch and 0001-Save-download-dir-block-device-for-the-next-tr_torre.patch.

[taem]

Hi,

Any comments on this ticket?

Thanks.

[Opperpanter]

Looks like we're almost there to get a working solution? I am in favour of scheduling it for the next release. Do you need me to test something?

[taem]

Replying to Opperpanter:

Looks like we're almost there to get a working solution? I am in favour of scheduling it for the next release. Do you need me to test something?

Hi,

It needs XFS quota support. I suppose to create the patch this week.

Thanks.

[livings124]

taem: ping

[taem]

Check for available quota when getting free disk space

[taem]

Add XFS quota support

[taem]

Cache download dir's block device and FS type

[taem]

Hi,

Please find attached patches. Please ignore previous patches.

Thanks.

[jordan]

Applied as-is in r13696, r13697, r13698.

Copyediting for whitespace/indentation/tabs in r13699.

[jordan]

Replaced new use of PATH_MAX with TR_PATH_MAX in r13700.

[jordan]

Looks like r13696 broke the Mac build: ​https://build.transmissionbt.com/job/trunk-mac/5511/console ... taem, do you have a fix for this?

[jordan]

taem, another FTBFS from this: #5197

[taem]

jordan, r13701 fixed Mac build?

[taem]

Replying to jordan:

taem, another FTBFS from this: #5197

Ok.

[jordan]

Two more build errors on Mac,

1. btodb() isn't being found

2. the arguments to quotactl() differ between BSD and Linux.

Latest log's at ​https://build.transmissionbt.com/job/trunk-mac/5516/console

[taem]

Replying to jordan:

Two more build errors on Mac,

1. btodb() isn't being found
2. the arguments to quotactl() differ between BSD and Linux.

Please find attached patch.

[jordan]

r13714: 0001-Port-quota-handling-to-BSD-and-Mac-OS-X.patch from taem

[jordan]

taem, you un-cc'ed yourself on this and the other FTBFS ticket. Are you still here?

Still not building on OS X. Ain't platform code fun? :)

​https://build.transmissionbt.com/job/trunk-mac/5525/console

[x190]

​http://bsdbased.com/2009/11/27/quota-support-on-mac-os-x

​http://lists.apple.com/archives/darwin-development/2003/Mar/msg00126.html

[JJTagy]

dq.dqb_curblocks ?

[taem]

Replying to jordan:

taem, you un-cc'ed yourself on this and the other FTBFS ticket. Are you still here?

Yes, I'm still receiving notifications :)

Still not building on OS X.

Ok.

Ain't platform code fun? :)

Yep :)

[taem]

Replying to JJTagy:

dq.dqb_curblocks ?

Yes, there is typo.

[taem]

Please find attached patch. I hope it fixed FTBFS.

[taem]

Please find attached patch that clean ups headers include.

[x190]

For SYS_DARWIN return freespace; (should be non-bitshifted and is already in bytes?)

return (freespace < 0) ? 0 : freespace * 1024;

[taem]

Replying to x190:

For SYS_DARWIN return freespace; (already in bytes?)

return (freespace < 0) ? 0 : freespace * 1024;

Maybe. I can't find reference for this. Can anyone test the patch?

[jordan]

r13715: 0001-Fix-FTBFS-for-OS-X-and-uClibc.patch

r13716: 0001-Headers-clean-up.patch

[x190]

Replying to taem:

Replying to x190:

For SYS_DARWIN return freespace; (already in bytes?)

return (freespace < 0) ? 0 : freespace * 1024;

Maybe. I can't find reference for this. Can anyone test the patch?

OS X quota.h: (all in bytes)

struct dqblk {
	u_int64_t dqb_bhardlimit;	/* absolute limit on disk bytes alloc */
	u_int64_t dqb_bsoftlimit;	/* preferred limit on disk bytes */
	u_int64_t dqb_curbytes;	        /* current byte count */

Also, why the bitshift (div2?), if I may ask?

[taem]

Replying to x190:

Replying to taem:

Replying to x190:

For SYS_DARWIN return freespace; (already in bytes?)

return (freespace < 0) ? 0 : freespace * 1024;

Maybe. I can't find reference for this. Can anyone test the patch?

OS X quota.h: (all in bytes)

struct dqblk {
	u_int64_t dqb_bhardlimit;	/* absolute limit on disk bytes alloc */
	u_int64_t dqb_bsoftlimit;	/* preferred limit on disk bytes */
	u_int64_t dqb_curbytes;	        /* current byte count */

Also, why the bitshift (div2?), if I may ask?

Do you run OS X? Would you please test attached patch?

[x190]

Replying to taem:

Do you run OS X? Would you please test attached patch?

Sorry, I can't build on 10.6. If you intend to return in KiB, then freespace / 1024 for SYS_DARWIN. FreeBSD bitshift---why? For all others---isn't the expected blocksize 512 KiB?

[taem]

Replying to x190:

If you intend to return in KiB, then freespace / 1024 for SYS_DARWIN.

I guess, freespace should be returned in bytes like for other archs.

FreeBSD bitshift---why? For all others---isn't the expected blocksize 512 KiB?

Yes, thats why variables should be bitshifted (divided by 2) to get correct values, because, as you have mentioned, quota stored in 512-bytes blocks. In FreeBSD, quota implemented the same way as in Linux - ​http://lists.freebsd.org/pipermail/freebsd-hackers/2008-February/023346.html.

[taem]

And I'm not sure how OS X stores quota, in 512 or 1024 bytes.

[x190]

Replying to taem:

And I'm not sure how OS X stores quota, in 512 or 1024 bytes.

I think you have it right as per your last patch. It was my mistake to think in terms of KiB instead of just bytes for blocksize and as shown by quota.h, OS X stores both limit and curbytes in bytes.

[jordan]

r13754: 0001-Headers-clean-up.patch

taem, if this finishes the ticket, go ahead and mark it as 'fixed' for 2.80 :)

[taem]

jordan, ok :)

[x190]

Replying to jordan:

r13754: 0001-Headers-clean-up.patch

taem, if this finishes the ticket, go ahead and mark it as 'fixed' for 2.80 :)

r13754 refers to 0001-In-OS-X-disk-quota-s-info-already-in-bytes.patch.