GNOME Bugzilla – Bug 785870
google backend doesn't report filesystem size/free/used
Last modified: 2017-08-16 13:15:46 UTC
The google gvfs backend does not currently support reporting how much of the user's quota is available or used. I'm not sure libgdata exposes that information right now? So this might need changes there, but I didn't dig very hard, and I'm not very familiar with the vocabulary it would use to report that information.
Created attachment 357169 [details] [review] Use GDataDocumentsMetadata Here's an initial patch. It requires the libgdata patch in bug 768594 (not yet reviewed), so some changes may happen there. But I'm posting for initial comments. Especially around my treatment of the unlimited quota case. Is there a better way to indicate "infinite" space? Am I breaking things to have "free != total - used" in that case? (which would be easy to fix if we want to guarantee that)
Ahem, bad copy/paste there. "It requires the libgdata patch in bug 785885" is what I meant to say.
Review of attachment 357169 [details] [review]: Thanks for your patch, I really appreciate that! Just have some notes: ::: daemon/gvfsbackendgoogle.c @@ +1974,3 @@ + GError **error) +{ + GCancellable *cancellable = G_VFS_JOB (job)->cancellable; I would pass this directly in gdata_documents_service_get_metadata and don't create another variable, but that's detail... @@ +1977,3 @@ + GDataDocumentsMetadata *metadata; + goffset gdata_total, gdata_used; + guint64 total, used, available; I don't think we need total and gdata_total, one of them is enough, isn't it? Same for used and gdata_used. @@ +1980,3 @@ + + metadata = gdata_documents_service_get_metadata (self->service, cancellable, error); + if (*error != NULL) This is not really nice since the error doesn't have to be passed in in theory... @@ +1986,3 @@ + gdata_used = gdata_documents_metadata_get_quota_used (metadata); + + /* Santize the values a bit */ I think that sanitizing should be done in gdata if really needed. @@ +1988,3 @@ + /* Santize the values a bit */ + if (gdata_used > gdata_total) + gdata_used = gdata_total; But can this really happen (except unlimited quota)? This would mean that we lost used in case of unlimited quota, which we probably don't want... @@ +1990,3 @@ + gdata_used = gdata_total; + if (gdata_used < 0) + gdata_used = 0; Can be also gdata_used negative if we remove the previous if? @@ +1993,3 @@ + used = gdata_used; + + if (gdata_total < 0) /* -1 means unlimited quota, so we'll fake a large size */ I would say that would be probably better to not set those in case of unlimited quota...
Created attachment 357209 [details] [review] v2 Here's v2, cleaned up a bit, now using async methods, and not setting FREE/SIZE when there is no quota.
The libgdata side of this was just committed with no API changes. The v2 patch here should be good to go then.
Created attachment 357215 [details] [review] v3 Of course as soon as I speak, I realize I left off a set of #if HAVE_LIBGDATA_0_17_9 guards. Ahem. Now it should be good for a review.
Review of attachment 357215 [details] [review]: Thanks for another patch, looks almost ok... Please add some commit description and bug number: https://wiki.gnome.org/Git/CommitMessages ::: daemon/gvfsbackendgoogle.c @@ +1974,3 @@ + gpointer user_data) +{ + GError *error = NULL; nitpick: Please put this below service and job declaration. @@ +1992,3 @@ + g_object_unref (metadata); + + used = MAX (0, used); Can be negative value ever returned? I don't see any mention of it in the docs... but I would rather let G_FILE_ATTRIBUTE_FILESYSTEM_USED unset in that case if we don't know what is meaning... and if it is just for sure, it should be rather in libgdata. @@ +1997,3 @@ + if (total >= 0) /* -1 means unlimited quota */ + { + used = MIN (used, total); Same question here?
Review of attachment 357215 [details] [review]: And I am currently a bit unsure why mutex is used in some cases in google backend and whether we should not use it also here. Not sure it is because of usage libgdata, or due to caching. Rishi?
> And I am currently a bit unsure why mutex is used in some cases in google > backend and whether we should not use it also here. Not sure it is because of > usage libgdata, or due to caching. Rishi? I'm not sure, but I suspect that the mutex is used for anything that can affect filenames/enumerating. Like, 'write' doesn't use the mutex, but query_info does. But I can add it to query_fs_info too, easy enough, just to be safe.
Created attachment 357342 [details] [review] v4 Now with a better commit message and no sanity checking. Let me make a quick pitch for sanity checking though: 1) I don't like trusting libraries. 2) I had imagined libgdata to be a thin layer around Google's services -- mostly leaving interpretation of responses to clients. So I didn't add any sanity checking in my patch to add the new API there (I just pass on what Google gives us). I can go back and try to add it there... But if that maintainer is going to say "upper layers should do it", I'm going to throw up my hands. (I asked in that review about sanity checking but didn't get a response.) 3) Maybe in the future Google/libgdata grow some state where it makes sense to return currently-unexpected results (like negative 'used' values start meaning something special to libgdata or 'used>total' because the user is temporarily over their quota or something). If that's a sane potential state for Google or libgdata, we'd still want to sanitize it on our side to future proof us.
(In reply to Ondrej Holy from comment #8) > Review of attachment 357215 [details] [review] [review]: > > And I am currently a bit unsure why mutex is used in some cases in google > backend and whether we should not use it also here. Not sure it is because > of usage libgdata, or due to caching. Rishi? The mutex is meant to guard the cache. I didn't want multiple threaded I/O jobs on the backend to modify the cache at the same time.
Review of attachment 357342 [details] [review]: Thanks all, looks ok then, although I haven't tested it yet... I am not against sanity, but "used = MAX (0, used);" is wrong. A negative value may mean error and it is incorrect to return zero instead (i.e. say that there are 0 bytes used). We should return an error or don't set the value, but don't return zero. I would say that the best is to just simply do not set the value in this case... ::: daemon/gvfsbackendgoogle.c @@ +1993,3 @@ + g_object_unref (metadata); + + g_file_info_set_attribute_uint64 (job->file_info, G_FILE_ATTRIBUTE_FILESYSTEM_USED, used); Maybe add if (used >= 0) for sure ;-)
Created attachment 357433 [details] [review] v5 (sanity check on 'used>=0') OK, here's a version with a simple >=0 check on 'used' before using it, which yeah, makes sense in case -1 is ever used as an error or special reply value. I can commit once we're out of freeze. Thanks for the reviews!
Review of attachment 357433 [details] [review]: Ok, thanks.
Attachment 357433 [details] pushed as 3508b1ae - google: Report FS total size and free space
I've just realized that the quota is shared with Gmail and Photos and doesn't reflect shared files probably, so it may be a bit confusing, but better than nothing...
By the way, we are in feature freeze period currently among others, which is the reason why I used "accepted-commit_after_freeze" and the patch was supposed to be pushed after the new stable release... or with an approval of the release-team. Although this is a really small change and also with ifdefs, it would be fair to let them know about at least. https://wiki.gnome.org/ReleasePlanning/Freezes#FeatureFreeze
Oh no! I'm so sorry. In my head, this was more like a bug fix and you had meant wait until the .90 release was finally done. I'm happy to revert too and wait for final release on this? No reason my misunderstanding should affect the release features. As for the quota, yes there is a lot of used quota that the user can't see, and files the user can see that don't take any quota. We *could* report only the Drive usage as the USED amount, but (A) free+used wouldn't equal total anymore and (B) we'll never be able to get to the point where all the visible files add up to the USED count anyway, because of shared files and backups etc.
The reversion is not needed. Hmm, you are right that this can be probably considered also as a bugfix. It doesn't affect any documentation also. So it is probably ok, just be careful next time.