Just FYI, here's where I'm going to use it

http://git.gnome.org/browse/gvfs/commit/?h=wip/udisks2&id=9c2a9b4da43edf22d24fb7b9f349cba9a83bdc5c

Review of attachment 197842 [details] [review]:

Not really convinced of this as a pure 'pass-through' thing.
Eg right next to this we find g_unix_mount_point_guess_icon() - whats the relation of that api to a gvfs.icon 'comment' ?
Should it not pick up the icon from the comment ?

::: gio/gunixmounts.c
@@ +1719,3 @@
+ *
+ * Since: 2.32
+ */

This is not really useful documentation.
At the very least, you should say something about the expected format of the returned string.

(In reply to comment #2)
> Review of attachment 197842 [details] [review]:
> 
> Not really convinced of this as a pure 'pass-through' thing.

But gunixmounts.c is already a low-level pass-through thing. At least the useful parts of it.

> Eg right next to this we find g_unix_mount_point_guess_icon() - whats the
> relation of that api to a gvfs.icon 'comment' ?
> Should it not pick up the icon from the comment ?

I you wanted to know, I actually don't like the guess_icon() and guess_name() functions - they're policy polluting a nice low-level API ... they should probably not have been made public but have stayed in the GUnixVolumeMonitor implementation where they are used. But if you insist, I'll make guess_name() and guess_icon() use comment=gvfs.name and comment=gvfs.icon_name like in the udisks2 monitor. I actually did at first but decided it was a mistake.
 
> ::: gio/gunixmounts.c
> @@ +1719,3 @@
> + *
> + * Since: 2.32
> + */
> 
> This is not really useful documentation.
> At the very least, you should say something about the expected format of the
> returned string.

And what would I say? That the options string is a comma-separated list of mount options? I don't think that adds anything but paint us into a corner where this actually might not be true (remember that this API is supposed to work on all kinds of Unix-like OSes).

(In reply to comment #3)

> > + */
> > 
> > This is not really useful documentation.
> > At the very least, you should say something about the expected format of the
> > returned string.
> 
> And what would I say? That the options string is a comma-separated list of
> mount options? I don't think that adds anything but paint us into a corner
> where this actually might not be true (remember that this API is supposed to
> work on all kinds of Unix-like OSes).

If you cannot come up with meaningful documentation, then this doesn't belong in the public api, I'd say. How will it work on all kinds of OSes if you can't even describe what it does on this one ?

Anyway, this is not really my corner, best to get Alex to comment.

(In reply to comment #4)
> (In reply to comment #3)
> 
> > > + */
> > > 
> > > This is not really useful documentation.
> > > At the very least, you should say something about the expected format of the
> > > returned string.
> > 
> > And what would I say? That the options string is a comma-separated list of
> > mount options? I don't think that adds anything but paint us into a corner
> > where this actually might not be true (remember that this API is supposed to
> > work on all kinds of Unix-like OSes).
> 
> If you cannot come up with meaningful documentation, then this doesn't belong
> in the public api, I'd say. How will it work on all kinds of OSes if you can't
> even describe what it does on this one ?

Excuse me? The answer, I guess, is the same way I'd expect get_device_path() and get_mount_path() and get_fs_type() to work? Neither of these have any docs apart from what I added here - for example, you will need to know a bunch of low-level details to use this in any meaningful way. Which is perfectly fine.

Anyway, I think you are missing the point that GUnixMountPoint and its monitor are low-level utilities used to implement the higher-level user-facing types such as GVolumeMonitor. They are just thin abstractions and, as such, it's fine to do pass-through stuff like this.

Feel free to close WONTFIX or whatever - then I'll just copy-paste http://people.freedesktop.org/~david/udisks2-20110929/UDisksFstabMonitor.html into my wip/udisks2 branch.

Alex: please let me know what you think or close WONTFIX. Thanks.

I think this is all right, its in line with the rest of gunixmounts.c i.e. its a somewhat portable fstab parser. One thing i worry about slightly is if escaping and options separator char differ between different unixes (and if so, should we return a pre-split unescaped list?). However, I've been unable to find a system so far that doesn't comma separate, and we don't unescape the other fields, so it would be kinda weird if this one is.

So, I'd say this can go i as-is.

(In reply to comment #7)
> I think this is all right, its in line with the rest of gunixmounts.c i.e. its
> a somewhat portable fstab parser. One thing i worry about slightly is if
> escaping and options separator char differ between different unixes (and if so,
> should we return a pre-split unescaped list?). However, I've been unable to
> find a system so far that doesn't comma separate, and we don't unescape the
> other fields, so it would be kinda weird if this one is.

Sounds good to me. Another data-point is that the routines currently returns unescaped data, e.g. the following line

 server:/mount/point  /media/Cool\040Media  nfs4  defaults,users,comment=gvfs.name=My\040Media,comment=gvfs.icon_name=folder-videos 0 0

will return "/media/Cool Media" and "defaults,users,comment=gvfs.name=My Media,...".

> So, I'd say this can go i as-is.

OK, thanks. Committed:

 http://git.gnome.org/browse/glib/commit/?id=3f982cb9ab1dc9a435c5bba2f0438f165eba25dc

Oh, Its already unescaped?

Thats problematic. What if there is an escaped comma in there? You need to split at comma before you unescape...

(In reply to comment #9)
> Oh, Its already unescaped?
> 
> Thats problematic. What if there is an escaped comma in there?

I think the way it's supposed to work is that you can only use \040 to escape a space because that's all you need in the fstab file. That said, I think the various fstab parsers out there (including libc's getmntent(3)) will unescape any octal sequence.

> You need to
> split at comma before you unescape...

Hmm. I was just mentioning in comment 8 that getmntent(3) and friends unescape octal-codes that appear in the /etc/fstab (on Linux at least). And that we just pass that through. I remain convinced that this low-level API should not be concerned with what the options string contains - it should just treat it as a string passed through from the low-level OS runtime.

From a 50,000 feet view I think that trying to make guarantees (such as it's a ','-separated string of tokens and that ',' can be escaped as '\,') about data you ultimately do not control is just a can of worms and can quickly paint you into a corner that is hard to escape from. So it's not really something you want to do unless you really need it.

And in this case, you really don't need to: I just wanted this feature for a Linux-specific piece of code (udisks2 volume monitor) so you can set a display name/icon for fstab mounts using the Linux-specific comment=<anything> option that can be used in Linux's /etc/fstab file. It's completely just a value-add kind of thing - totally something we can live without - but also something that's nice to have.

If it turns out we want to interpret the options string later we can add API such as

 gchar **g_unix_mount_point_get_mount_options (GUnixMountPoint *);
 gchar  *g_unix_mount_point_lookup_mount_option (GUnixMountPoint *);

and so on that actually tries to interpret the options string - on Linux it would split the comma-separated string and unescape \, so you'd get e.g.

 'defaults', 'user', 'comment=gvfs.name=My Photos, Videos and Movies'

as the tokens from the options string

 'defaults,user,comment=gvfs.name=My\040Photos\,040Videos\040and\040Movies'

in /etc/fstab.

But until we need this kind of guarantees, I would suggest not adding it (and this is why I'm wary of making the guess() methods use this). More importantly, if we do need API like this, I would prefer to not put it on the GUnixMountPoint type - I'd try to do a higher-level abstraction - or, even better, not even try to attempt to solve this problem on anything but Linux.

(In reply to comment #10)
> on Linux it
> would split the comma-separated string and unescape \, so you'd get e.g.
> 
>  'defaults', 'user', 'comment=gvfs.name=My Photos, Videos and Movies'
> 
> as the tokens from the options string
> 
>  'defaults,user,comment=gvfs.name=My\040Photos\,040Videos\040and\040Movies'

This should have been

 'defaults,user,comment=gvfs.name=My\040Photos\,Videos\040and\040Movies'

Btw, I just looked and it turns out that glibc's getmnt(3) (which is used in GUnixMountPoint) actually only will unescape

 space (\040),
 tab (\011),
 newline (\012) and
 backslash (\134)

and anything else will be passed through.

But there are no guarantees in either the mount(8) program or the mount(2) system call that you can unescape a comma in the options string - it just does not appear to be supported. Briefly looking through the util-linux source code (which provides the mount(8) command) it appears this is correct.

So for e.g. gvfs.name we should probably use URI escaping so comma's are escaped as %2C.

If someone else uses \054 to escape comma (which seems more natural than \, to me) inside e.g. a comment we would not be able to properly split the options string correctly.

Additionally, using "\," to escape commas doesn't work because other apps will parse the options line and split at the "," in "\," since they don't know about that escape method.

Ah, saw your update. Yes, using URI escaping would solve this.

(In reply to comment #11)
> So for e.g. gvfs.name we should probably use URI escaping so comma's are
> escaped as %2C.

Done here

 http://git.gnome.org/browse/gvfs/commit/?h=wip/udisks2&id=0d47c8cf3b15a5787f569ec3326836038d20d880