Fw: RE: [PATCH 2/3] exfat: remove useless check in exfat_move_file()

Fw: RE: [PATCH 2/3] exfat: remove useless check in exfat_move_file()

[Kohada.Tetsuhiro]

Thank you for continuing the discussion.
The reply was delayed to summarize the arguing points.

> I already gave my comment on previous thread, and I prefer de array handling I sent instead of only two entries.
We haven't discussed enough yet and I have some questions.
I still don't understand what's technically problem.

> It will avoid repetitive loops to get entries from cache buffer.
Is that loop the first verification and name extraction?
I don't understand why you can avoid the repetitive loop by using arrays.
I think getting from an array is equivalent to getting it via a function.
   A = obj->array[idx];
   B = get(obj, idx);
For using, what's the difference between A and B?

> I think it is also suitable for function definition and union type  entry structure.
I, too, think the combination is not bad.
However, it has no advantage compared to other methods.
(Can you give me any example?)
Also, as I said in my previous mail, union has the problem of too flexible for type.
(Especially file-de and stream-de are easy to confuse)
So I want to avoid to access union directly from the upper function, as possible.

> If you send the patches included this change again, I will actively look into your patches.
It will take some time as I haven't come up with a good idea yet.

We have discussed it so far, but there are still some unclear points.
First, I would like to clarify them.

1. About the need for TYPE_NAME-validation in exfat_get_dentry_set().
My opinion is
> It is possible to correctly determine that
> "Immediately follow the Stream Extension directory entry as a consecutive series"
> whether the TYPE_NAME check is implemented exfat_get_uniname_from_ext_entry() or
> exfat_get_dentry_set().
> It's functionally same, so it is also right to validate in either.

Your opinion is
> We have not checked the problem when it is removed because it was implemented
> according to the specification from the beginning.

I understand that you haven't thought about it yet.
What happens if I don't check here?
Please imagine if you can.


2. About TYPE_NAME-validation in exfat_get_uniname_from_ext_entry()
Below are the changes in '[PATCH v4 1/5] exfat: integrates dir-entry getting and validation'
> -     for (i = 2; i < es->num_entries; i++) {
> -             struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> -
> -             /* end of name entry */
> -             if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> -                     break;
>
> +     i = ES_INDEX_NAME;
> +     while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
>               exfat_extract_uni_name(ep, uniname);
>               uniname += EXFAT_FILE_NAME_LEN;
>       }

Your request for this change is
> Please find the way to access name entries like ep_file, ep_stream
> without calling exfat_get_validated_dentry().

What is the reason(or rationale) for such a request?
Please explain what the problem is with this change, if you can.

As I explained before, the reason for validating TYPE_NAME here is
> name-length and type validation and name-extraction should not be separated.
> These are closely related, so these should be placed physically and temporally close.

Please explain why you shouldn't validate TYPE_NAME here.


3. About using exfat_get_validated_dentry() in exfat_update_dir_chksum_with_entry_set()
Below are the changes in '[PATCH v4 1/5] exfat: integrates dir-entry getting and validation'
>       for (i = 0; i < es->num_entries; i++) {
> -             ep = exfat_get_dentry_cached(es, i);
> +             ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
>               chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
>                                            chksum_type);

Your request for this change is
> Ditto, You do not need to repeatedly call exfat_get_validated_dentry() for the entries
> which got from exfat_get_dentry_set().

Even if the entry was got from exfat_get_dentry_set(), we need to get the ep again to calculate the checksum.
exfat_get_validated_dentry() with TYPE_ALL is the same as exfat_get_dentry_cached() because it allows all TYPEs.
Please elaborate on what the problem is.


4. About double-checking name entries as TYPE_SECONDARY and TYPE_NAME.
You said in 'RE: [PATCH v3] exfat: integrates dir-entry getting and validation'.
> your v3 patch are
> already checking the name entries as TYPE_SECONDARY. And it check them with
> TYPE_NAME again in exfat_get_uniname_from_ext_entry(). If you check TYPE_NAME
> with stream->name_len, We don't need to perform the loop for extracting
> filename from the name entries if stream->name_len or name entry is invalid.

It is rare case that stream->name_len or name-entry are invalid.
Perform the loop to extract filename when stream->name_len or name-entry is invalid has little effect.
What is the probrem with perform the loop for extract filename when stream->name_len or name-entry are invalid?


5. About validate flags as argument of exfat_get_dentry_set().
You suggested
> You can add a validate flags as argument of exfat_get_dentry_set(), e.g. none, basic and strict.
> So as I suggested earlier, You can make it with an argument flags so that we skip the validation.

What are the advantages of skipping validation with this flag?
I don't think there's any advantage worth the complexity of the code.


This discussion may take some time, but I hope you continue the discussion.

BR
---
Tetsuhiro Kohada <kohada.t2@gmail.com>