GNOME Bugzilla – Bug 764099
Misc house arrest bug fixes
Last modified: 2016-06-09 10:00:00 UTC
The first one, we'll need to land into stable releases.
Created attachment 324611 [details] [review] afc: Fix handling of new HouseArrest paths In b8ad223, we re-rooted the HouseArrest paths to always include Documents/ as anything closed to the root isn't accessible. This caused 2 problems: - first, it meant that checking for the new path being "/" wasn't correct, as the path would always include at least a "Documents" path component - secondly, it meant that the display name for the root directory for an application was incorrect as it was always "Documents" instead of the app name.
Created attachment 324612 [details] [review] afc: Add guards against the operation mode not being set We usually did: if mode == AFC; <foo> else <bar> To guard against the mode not actually being HouseArrest in the else branch, change this to: if mode == AFC; <foo> else if mode == HOUSE_ARREST ; <bar> ; else ; <baz>
Review of attachment 324611 [details] [review]: ::: daemon/gvfsbackendafc.c @@ +878,3 @@ + *is_doc_root = TRUE; + else if (is_doc_root) + *is_doc_root = FALSE; It would be good to check is_doc_root first (just a nitpick), e.g.: if (is_doc_root) *is_doc_root = (comps[0] != NULL && comps[1] == NULL); @@ +885,3 @@ comps[0] = g_strdup ("Documents"); + *new_path = g_strjoinv ("/", comps); + if (is_doc_root) is_doc_root is not a boolean, it is a pointer in this case, is this really what you want? @@ +924,3 @@ goto is_dir_bail; + if (is_doc_root) It would be good to merge this with previous if statement as it is in other cases (just a nitpick)...
Review of attachment 324612 [details] [review]: May this really happen? Shouldn't we return some meaningful error instead of asserts?
(In reply to Ondrej Holy from comment #4) > Review of attachment 324612 [details] [review] [review]: > > May this really happen? Shouldn't we return some meaningful error instead of > asserts? It can't happen, that's why it's an assertion. That would mean we forgot to adapt the code if we ever added a new operation "mode".
Asking just for sure, it is ok then :-)
(In reply to Ondrej Holy from comment #3) > Review of attachment 324611 [details] [review] [review]: > > ::: daemon/gvfsbackendafc.c > @@ +878,3 @@ > + *is_doc_root = TRUE; > + else if (is_doc_root) > + *is_doc_root = FALSE; > > It would be good to check is_doc_root first (just a nitpick), e.g.: > if (is_doc_root) > *is_doc_root = (comps[0] != NULL && comps[1] == NULL); Fixed. > @@ +885,3 @@ > comps[0] = g_strdup ("Documents"); > + *new_path = g_strjoinv ("/", comps); > + if (is_doc_root) > > is_doc_root is not a boolean, it is a pointer in this case, is this really > what you want? should be if (*is_doc_root) indeed, fixed. > @@ +924,3 @@ > goto is_dir_bail; > > + if (is_doc_root) > > It would be good to merge this with previous if statement as it is in other > cases (just a nitpick)... Fixed.
Created attachment 329207 [details] [review] afc: Fix handling of new HouseArrest paths In b8ad223, we re-rooted the HouseArrest paths to always include Documents/ as anything closed to the root isn't accessible. This caused 2 problems: - first, it meant that checking for the new path being "/" wasn't correct, as the path would always include at least a "Documents" path component - secondly, it meant that the display name for the root directory for an application was incorrect as it was always "Documents" instead of the app name.
Review of attachment 329207 [details] [review]: ::: daemon/gvfsbackendafc.c @@ +883,3 @@ comps[0] = g_strdup ("Documents"); + *new_path = g_strjoinv ("/", comps); + if (*is_doc_root) This is still probably wrong, because is_doc_root may be NULL...
Created attachment 329369 [details] [review] afc: Fix handling of new HouseArrest paths In b8ad223, we re-rooted the HouseArrest paths to always include Documents/ as anything closed to the root isn't accessible. This caused 2 problems: - first, it meant that checking for the new path being "/" wasn't correct, as the path would always include at least a "Documents" path component - secondly, it meant that the display name for the root directory for an application was incorrect as it was always "Documents" instead of the app name.
Review of attachment 329369 [details] [review]: Looks good to me now, thanks, but please test it properly before pushing, I haven't got a chance to test...
Attachment 324612 [details] pushed as 828d6ae - afc: Add guards against the operation mode not being set Attachment 329369 [details] pushed as 5471b77 - afc: Fix handling of new HouseArrest paths