Was it a local file or not? /run/user/ seems to imply a mounted removable media, but thats a local file is it not?

(In reply to comment #1)
> Was it a local file or not? /run/user/ seems to imply a mounted removable
> media, but thats a local file is it not?

It was a file on a samba share, and I passed the fuse path to it.

Right, so how do we handle this? Would really want to finally resolve this bug. Should we simply do what we'd do for HTTP URLs? Download first and then use the local copy?

Since it's fuse-mounted, it should more or less behave as a local file, before doing anything special, I'd try to understand why Boxes does not like such mountpoints..

(In reply to comment #4)
> Since it's fuse-mounted, it should more or less behave as a local file, before
> doing anything special, I'd try to understand why Boxes does not like such
> mountpoints..

Right, forgot that the local path is being passed not the gvfs URI. I wonder though if this is really related to file being remote and not that boxes fails on that ISO for some other reason.

After setting up a samba share, I can reproduce the issue but its a different one now in 3.9: Boxes gets stuck at 'Preparation' stage saying "Preparing to create new box" with 0% progress.

Created attachment 252668 [details] [review]
media-manager: Work around vala bug#706517

Its not one of those ugly work arounds and I'm pretty certain some
people will find the resulting code from this minor change, more
readable than before.

Created attachment 252669 [details] [review]
wizard: Don't hang on failure to handle a URL

Boxes would just hang in wizard's preparation stage if you pass a path
to a file on a locally mounted remote share (e.g SMB). Although the real
solution is to be able to handle those, we should have this in place
just in case this codepath is walked for some other reason.

Created attachment 252670 [details] [review]
wizard: Handle paths as well

In commit 3c1b44c, we lost the ability to handle local paths in wizard.
This patch brings back that ability.

Created attachment 252671 [details] [review]
wizard: Handle local paths to remote files

g_file_native() thinks of paths to locally mounted remote locations, as
non-native so Boxes errors out if given such paths. This patch fixes the
issue by replacing g_file_native() usage with a mannual check.

(In reply to comment #4)
> Since it's fuse-mounted, it should more or less behave as a local file, before
> doing anything special, I'd try to understand why Boxes does not like such
> mountpoints..

One of the main problems might be that only the user himself can access the gvfs fuse mount, not even root can access it, so it might block any virt code running as root from using that file.

(In reply to comment #11)
> (In reply to comment #4)
> > Since it's fuse-mounted, it should more or less behave as a local file, before
> > doing anything special, I'd try to understand why Boxes does not like such
> > mountpoints..
> 
> One of the main problems might be that only the user himself can access the
> gvfs fuse mount, not even root can access it, so it might block any virt code
> running as root from using that file.

Nah, we use session libvirt so that shouldn't be an issue and the patches I attached should fix the issue. Passing the path through commandline is still broken though and I'm looking into that..

Created attachment 252681 [details] [review]
app: Minor simplification of code

Created attachment 252682 [details] [review]
wizard: Handle local paths to remote files

There are two issues this patch fixes:

* g_file_native() thinks of paths to locally mounted remote locations, as
  non-native so Boxes errors out if given such paths. This patch fixes
  the issue by replacing g_file_native() usage with a mannual check.

* g_file_get_uri() gives us the remote URI even if you created the GFile
  argument using the local path of the remote file. This patch fixes the
  issue by using the original commandline argument if the argument is
  not a path rathan than URI.

Review of attachment 252682 [details] [review]:

::: src/app.vala
@@ +293,3 @@
             call_when_ready (() => {
                 var file = File.new_for_commandline_arg (arg);
+                var is_uri = (Uri.parse_scheme (arg) != null);

A relative file on a remote location won't match.

::: src/wizard.vala
@@ +228,3 @@
 
+        var is_uri = location.contains ("://");
+        var file = is_uri? File.new_for_uri (location) : File.new_for_path (location);

Why do you need that? Use File.new_for_commandline_arg instead of hacking around it.

@@ +232,1 @@
+        if (!is_uri || file.has_uri_scheme ("file")) {

Why do you need that? Are you trying to check whether the file is accessible through a file:/// URI? Get the path with g_file_get_path() and transform that to a URI with g_filename_to_uri().

Review of attachment 252668 [details] [review]:

yeah, this looks better than before.

Review of attachment 252670 [details] [review]:

::: src/wizard.vala
@@ +227,3 @@
             throw new Boxes.Error.INVALID ("empty location");
 
+        var file = location.contains ("://")? File.new_for_uri (location) : File.new_for_path (location);

This seems weird. Where does "location" come from, if its from the commandline (i.e. in the locale encoding) you should use g_file_new_for_commandline_arg, and if its utf8 (from the ui) you should use g_file_parse_name().

Review of attachment 252682 [details] [review]:

If you want a path, just use g_file_get_path(), no need for manual uri tweaking (which is bound to go wrong in complex cases).

Honestly, I wouldn't trust the fuse/gvfs stuff for passing into libvirt/qemu/etc without testing. Its totally gonna break for e.g. ftp uris. I think we should:

path = g_file_get_path (file);
if (g_file_is_native (file) ||
    (path != NULL &&
     g_file_has_uri_scheme (file, "smb")))
  {
     use path directly
  }
else
  {
     copy file to a local file and use
  }

I.e. only use the direct path for truly local files and for some whitelisted set of tested gvfs backends.

Review of attachment 252682 [details] [review]:

::: src/app.vala
@@ +293,3 @@
             call_when_ready (() => {
                 var file = File.new_for_commandline_arg (arg);
+                var is_uri = (Uri.parse_scheme (arg) != null);

and we don't want to match relative paths here or what did you mean? Example?

::: src/wizard.vala
@@ +228,3 @@
 
+        var is_uri = location.contains ("://");
+        var file = is_uri? File.new_for_uri (location) : File.new_for_path (location);

We used to use that but Alex pointed out how thats wrong for anything that comes from UI: https://bugzilla.gnome.org/show_bug.cgi?id=689840

@@ +232,1 @@
+        if (!is_uri || file.has_uri_scheme ("file")) {

I'm checking if its a local file or not by checking if its not a URI (i-e than its a path) or if its a URI with 'file' scheme.

Review of attachment 252670 [details] [review]:

::: src/wizard.vala
@@ +227,3 @@
             throw new Boxes.Error.INVALID ("empty location");
 
+        var file = location.contains ("://")? File.new_for_uri (location) : File.new_for_path (location);

It can come from both. We used to just use g_file_new_for_commandline_arg() here but I replaced it w/ g_file_new_for_uri(), which fixes filename coming from UI but broken filename coming from commandline.

(In reply to comment #19)
> Honestly, I wouldn't trust the fuse/gvfs stuff for passing into
> libvirt/qemu/etc without testing. Its totally gonna break for e.g. ftp uris. I
> think we should:
> 
> path = g_file_get_path (file);
> if (g_file_is_native (file) ||
>     (path != NULL &&
>      g_file_has_uri_scheme (file, "smb")))
>   {
>      use path directly
>   }
> else
>   {
>      copy file to a local file and use
>   }

Hmm.. I guess it makes sense and I'll trust your opinion on GVFS. I was gonna do the 'else {}' block for remote URIs anyways.

Created attachment 252763 [details] [review]
wizard: Handle local paths to remote files

There are two issues this patch fixes:

* g_file_native() thinks of paths to locally mounted remote locations, as
  non-native so Boxes errors out if given such paths. This patch fixes
  the issue by replacing g_file_native() usage with a mannual check.

* g_file_get_uri() gives us the remote URI even if you created the GFile
  argument using the local path of the remote file. This patch fixes the
  issue by using the original commandline argument if the argument is
  not a path rathan than URI.

Also we restrict the path/URI to filesystems that we know libvirt/qemu
can handle: local and mounted SMB shares.

Pushing these patches. Feel free to still review them and re-open the bug, if needed.

Attachment 252668 [details] pushed as 52ae670 - media-manager: Work around vala bug#706517
Attachment 252669 [details] pushed as 22ff0aa - wizard: Don't hang on failure to handle a URL
Attachment 252670 [details] pushed as 719eacd - wizard: Handle paths as well
Attachment 252681 [details] pushed as 60e4186 - app: Minor simplification of code
Attachment 252763 [details] pushed as e898b44 - wizard: Handle local paths to remote files