On 2/13/24 6:49 AM, Jan Kara wrote:
> On Fri 09-02-24 13:43:09, Eric Sandeen wrote:
>> Convert the UDF filesystem to the new mount API.
...
>> +static void udf_init_options(struct fs_context *fc, struct udf_options *uopt)
>> +{
>> + if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
>> + struct super_block *sb = fc->root->d_sb;
>> + struct udf_sb_info *sbi = UDF_SB(sb);
>> +
>> + uopt->flags = sbi->s_flags;
>> + uopt->uid = sbi->s_uid;
>> + uopt->gid = sbi->s_gid;
>> + uopt->umask = sbi->s_umask;
>> + uopt->fmode = sbi->s_fmode;
>> + uopt->dmode = sbi->s_dmode;
>> + uopt->nls_map = NULL;
>> + } else {
>> + uopt->flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
>> + /* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */
>
> Nit: Please wrap these two lines.
Done, noticed that after sending, sorry!
>> +static void udf_free_fc(struct fs_context *fc)
>> +{
>> + kfree(fc->fs_private);
>> +}
>
> So I think we need to unload uopt->nls_map in case we eventually failed the
> mount (which means we also need to zero uopt->nls_map if we copy it to the
> sbi).
Hmm let me look into that - I guess there are ways for i.e. udf_fill_super
can fail that wouldn't free it before we got here.
>> +static const struct fs_parameter_spec udf_param_spec[] = {
>> + fsparam_flag ("novrs", Opt_novrs),
>> + fsparam_flag ("nostrict", Opt_nostrict),
>> + fsparam_u32 ("bs", Opt_bs),
>> + fsparam_flag ("unhide", Opt_unhide),
>> + fsparam_flag ("undelete", Opt_undelete),
>> + fsparam_flag ("noadinicb", Opt_noadinicb),
>> + fsparam_flag ("adinicb", Opt_adinicb),
>
> We could actually use the fs_param_neg_with_no for the above. I don't
> insist but it's an interesting exercise :)
good idea, done.
...
>> + switch (token) {
>> + case Opt_novrs:
>> + uopt->novrs = 1;
>
> I guess we can make this just an ordinary flag as a prep patch?
Sorry, not sure what you mean by this?
Oh, a uopt->flag. Ok, I can do that.
>> + break;
>> + case Opt_bs:
>> + n = result.uint_32;;
> ^^ one is enough ;)
>
>> + if (n != 512 && n != 1024 && n != 2048 && n != 4096)
>> + return -EINVAL;
>> + uopt->blocksize = n;
>> + uopt->flags |= (1 << UDF_FLAG_BLOCKSIZE_SET);
>> + break;
>> + case Opt_unhide:
>> + uopt->flags |= (1 << UDF_FLAG_UNHIDE);
>> + break;
>> + case Opt_undelete:
>> + uopt->flags |= (1 << UDF_FLAG_UNDELETE);
>> + break;
>
> These two are nops so we should deprecate them and completely ignore them.
> I'm writing it here mostly as a reminder to myself as a work item after the
> conversion is done :)
ok ;)
>> + case Opt_noadinicb:
>> + uopt->flags &= ~(1 << UDF_FLAG_USE_AD_IN_ICB);
>> + break;
>> + case Opt_adinicb:
>> + uopt->flags |= (1 << UDF_FLAG_USE_AD_IN_ICB);
>> + break;
>> + case Opt_shortad:
>> + uopt->flags |= (1 << UDF_FLAG_USE_SHORT_AD);
>> + break;
>> + case Opt_longad:
>> + uopt->flags &= ~(1 << UDF_FLAG_USE_SHORT_AD);
>> + break;
>> + case Opt_gid:
>> + if (0 == kstrtoint(param->string, 10, &uv)) {
> Nit:
> ^^ I prefer "kstrtoint() == 0"
No problem.
-Eric
> Otherwise looks good.
>
> Honza
