<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small">Hi Cyril,</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 11, 2021 at 12:34 AM Martin Doucha <<a href="mailto:mdoucha@suse.cz">mdoucha@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 10. 03. 21 17:22, Cyril Hrubis wrote:<br>
> Hi!<br>
>>> +static int has_kernel_support(const char *fs_type, const char *const *skiplist)<br>
>>>  {<br>
>>>     static int fuse_supported = -1;<br>
>>>     const char *tmpdir = getenv("TMPDIR");<br>
>>>     char buf[128];<br>
>>>     int ret;<br>
>>>  <br>
>>> +   if (tst_fs_in_skiplist(fs_type, skiplist))<br>
>>> +           return 0;<br>
>>> +<br>
>>>     if (!tmpdir)<br>
>>>             tmpdir = "/tmp";<br>
>>>  <br>
>>> @@ -84,26 +105,24 @@ static int has_kernel_support(const char *fs_type, int flags)<br>
>>>             return 0;<br>
>>>     }<br>
>>>  <br>
>>> -   if (flags & TST_FS_SKIP_FUSE) {<br>
>>> -           tst_res(TINFO, "Skipping FUSE as requested by the test");<br>
>>> +   if (tst_fs_in_skiplist("fuse", skiplist))<br>
>>>             return 0;<br>
>>> -   }<br>
>>>  <br>
>>>     tst_res(TINFO, "FUSE does support %s", fs_type);<br>
>>>     return 1;<br>
>>>  }<br>
>><br>
>> I don't think that has_kernel_support() should look at the skiplist at<br>
>> all. The entire skiplist logic should be handled in<br>
>> tst_get_supported_fs_types(). But has_kernel_support() could return<br>
>> different (non-zero) values for native support and for FUSE support.<br>
> <br>
> I do not agree, that would add more complexity to an internal function<br>
> that is not exported outside the library.<br>
<br>
Your patchset adds complexity to tst_fs_is_supported() which is a public<br>
wrapper of has_kernel_support(), even though it's only used indirectly<br>
in shell tests. Some tests might use that function directly in the<br>
future so let's make the interface cleaner, not hairier.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">+1</div><br></div><div><div class="gmail_default" style="font-size:small">I have the same view as Martin. Since only regarding the function name</div><div class="gmail_default" style="font-size:small">has_kernel_support() should only take care of the kernel supported filesystem</div><div class="gmail_default" style="font-size:small">but not include any blacklist from users. That will make people confused in</div><div class="gmail_default" style="font-size:small">code reading.</div></div><div><br></div></div><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>