When user executes mlabel with prohibited label character then mlabel asks interactively user how to fix it. But because in GParted there is no terminal window for interaction, mlabel command just hangs forever.

This patch ensure that such situation does not happen.

I'll review this.

Good catch on ".  It is in the list of prohibited characters but missing
from the GParted code.

Why are you rejecting ', it is not in the prohibited list?

Why are you rejecting just \005 when mtools rejects all control
characters?
    # export MTOOLS_SKIP_CHECK=1
    # mlabel ::"" -i /dev/sdb14
    Long file name "" contains illegal character(s).
    a)utorename A)utorename-all r)ename R)ename-all 
    s)kip S)kip-all q)uit (aArRsSq): q

I copied prohibited characters from the mtools source code, vfat.c contains:

const char *short_illegals=";+=[]',\"*\\<>/?:|";
const char *long_illegals = "\"*\\<>/?:|\005";

The commit message should be describing why a change is required, making
it as easily as possible for the reviewer to say yes that's correct and
apply it.  So far justification is weak.  At the moment I am tempted to
only add " because it is a prohibited character the code currently
misses and leave it at that.

mtools in vfat.c contains above list of illegal characters. And " ' \005 are illegal too (specified in list), so gparted should not allow to pass them into mtools. My patch adds those 3 characters into gparted blacklist.

I am after some indication that you didn't just copy those lists of
illegal characters and assume mlabel will reject them.  What happened
when you used mlabel to write a label containing a single quote?

$ mlabel ::"label" -i fat
$ mdir :: -i fat
 Volume in drive : is LABEL

$ mlabel ::'"' -i fat
Long file name """ contains illegal character(s).

$ mlabel ::"'" -i fat
$ mdir :: -i fat
 Volume in drive : is ' (abbr=_~1        )
$ blkid -o value -s LABEL -p fat
_~1

So looks like specifying ' as label is accepted by mlabel, but then label stored in root directory looks like some garbage...

Anyway, in mtools handling of illegals is complicated, also any character which matches "c < ' ' && c != '\005' && !(c & 0x80)" is also illegal.

So all control characters (below space) should be prohibited.

$ mlabel ::`echo -e "\005"` -i fat
Long file name "" contains illegal character(s).

$ mlabel ::`echo -e "\006"` -i fat
Long file name "" contains illegal character(s).

Anyway, better would be to disallow in UI to enter those disallowed characters instead of current solution when label string is filtered prior to passing it into external program.

Created attachment 359267 [details] [review]
Update list of prohibited fat label characters (v2)

(In reply to Pali from comment #9)
> Anyway, better would be to disallow in UI to enter those disallowed
> characters instead of current solution when label string is filtered prior
> to passing it into external program.
Yes only valid characters should be accepted as input.  However there
are different sets of valid characters for different file system labels.
Non-trivial change which needs researching and testing.

Anyway, I am going to commit this change in a day or so unless you see
any issue.  It excludes all ASCII control characters below SPACE and
updates the commit message about why the change is needed.

Patch looks good for me.

> However there are different sets of valid characters for different file system labels.

E.g. in KDE Partition Manager filesystem classes can provide its own validator (e.g. regular expression) which is used by GUI input fields for entering label as constraint what can be entered by user.

I do not know if GTK's input field class supports such constraint, but Qt has it.

So this is just a suggestion for possible implementation.

The following commit has been pushed upstream to the GIT repo ready for
inclusion in the next GParted release.

Update list of prohibited fat label characters (#787202)
https://git.gnome.org/browse/gparted/commit/?id=c3ad49d9daafa7cab335ca069875a057c67c1761

This enhancement was included in the GParted 0.30.0 release on October 10, 2017.