Re: [PATCHES] proxy monitor fixes + mount by device file

[David Zeuthen <david fubar dk>]

Hey,

Thanks for the review, an updated version is here

http://people.freedesktop.org/~david/gvfs-proxy-fixes-2.patch

On Mon, 2009-02-23 at 11:47 +0100, Alexander Larsson wrote:
> On Fri, 2009-02-20 at 17:30 -0500, David Zeuthen wrote:
> > Hey,
> > 
> > Here are two patches. The first one is very simple, it teaches
> > gvfs-mount about the --device (short -d) option so you can do things
> > like this
> > 
> >         $ gvfs-mount -d /dev/sdb3 
> >         The device "Generic STORAGE DEVICE" contains encrypted data on partition 3.
> >         Password: 
> >         Mounted /dev/dm-0 at /media/Encrypted Stuff
> >         
> > The other patch is, uh, a bit more complex. It's basically a huge set of
> > fixes for the proxy monitor code. I needed all these fixes for the
> > DeviceKit-disks/gnome-disk-utility volume monitor I'm working on. Here's
> > a bonus screenshot of that
> 
> g_proxy_volume_mount()
>  - if already canceled, leaks data
>  - g_cancellable_is_cancelled() can handle cancellable == NULL, just
> move
>    this check to start of function
> Same for other calls in drive, mount, etc

Fixed.

> proxy volme/drive: mount_cancelled()/operation_cancelled()
> This calls g_proxy_volume_monitor_get_dbus_connection() etc and may be
> called on a thread.
> Shouldn't it get the proxy lock?

Done.

> proxy volume:
> mount_cb and mount_cancelled can race if cancellation runs on another
> thread
> This is actually a bit hard to fix, we probably need some API in gio for
> this, similar
> to what you asked about before in the connect case. I'll look into this.
> All you
> should need to do is disconnect before starting to free data.

Yeah, this is

 http://bugzilla.gnome.org/show_bug.cgi?id=572844

and it looks like we won't get that in this round. I'll update the gvfs
patch on that bug.

> proxy volume: DBusOp->is_cancelled
> Why do you need this, you can just look at g_cancellable_is_cancelled()
> Same for drive/mount/etc

Good point.

> proxy volume:   
> volume->hash_mount_op_id_to_data is leaked

Fixed.

> Also, is it a good idea to send the password in clear text over the
> session bus?

It's trivially obtainable _anyway_ by just ptracing the processes and
since everything in your session effectively runs in the same security
domain it doesn't make sense to encrypt it. In the future all this might
change but we are not there yet.

However, your point on IRC that it would be useful to obfuscate the
password (to avoid it showing up in the clear in dbus-monitor etc.) is
good; I'm now base64 encoding it.

Another fix included in this patch is that for shadow mounts we should
be redirecting can_eject() and eject() to the volume for the shadow
mount. Without this patch eject on e.g. cdda:// volumes won't work since
cdda:// volumes are GDaedmonMount and these don't implement eject.

What about the other patch? (gvfs-mount-allow-device-file.patch)

     David