Re: [PATCH 4/8] staging: erofs: cleanup z_erofs_vle_work_{lookup, register}
Hi Dan, On 2018/8/13 21:05, Dan Carpenter wrote: > Yeah. You'd have to remove the const. > > Anyway, on looking at it more, I guess this patch is fine for now. We > will probably end up changing z_erofs_vle_work_lookup() and > z_erofs_vle_work_register() some more in the future. > Thanks for review, I personally tend to leave this patch unmodified for now :( . The struct is wrote in order to read-only or assign and read at once, and let compilers notice that (all modifications are local so compilers can handle them safely)... Thanks, Gao Xiang > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] staging: erofs: cleanup z_erofs_vle_work_{lookup, register}
Yeah. You'd have to remove the const. Anyway, on looking at it more, I guess this patch is fine for now. We will probably end up changing z_erofs_vle_work_lookup() and z_erofs_vle_work_register() some more in the future. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] staging: erofs: cleanup z_erofs_vle_work_{lookup, register}
Hi Dan,
On 2018/8/13 20:00, Dan Carpenter wrote:
> On Sun, Aug 12, 2018 at 10:01:46PM +0800, Chao Yu wrote:
>> From: Gao Xiang
>>
>> This patch introduces 'struct z_erofs_vle_work_finder' to clean up
>> arguments of z_erofs_vle_work_lookup and z_erofs_vle_work_register.
>>
>> Signed-off-by: Gao Xiang
>> Reviewed-by: Chao Yu
>> Signed-off-by: Chao Yu
>> ---
>> drivers/staging/erofs/unzip_vle.c | 89 ---
>> 1 file changed, 47 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/unzip_vle.c
>> b/drivers/staging/erofs/unzip_vle.c
>> index b2e05e2b4116..5032b3b05de1 100644
>> --- a/drivers/staging/erofs/unzip_vle.c
>> +++ b/drivers/staging/erofs/unzip_vle.c
>> @@ -271,36 +271,42 @@ static inline bool try_to_claim_workgroup(
>> return true;/* lucky, I am the followee :) */
>> }
>>
>> +struct z_erofs_vle_work_finder {
>> +struct super_block *sb;
>> +pgoff_t idx;
>> +unsigned pageofs;
>> +
>> +struct z_erofs_vle_workgroup **grp_ret;
>> +enum z_erofs_vle_work_role *role;
>> +z_erofs_vle_owned_workgrp_t *owned_head;
>> +bool *hosted;
>> +};
>> +
>> static struct z_erofs_vle_work *
>> -z_erofs_vle_work_lookup(struct super_block *sb,
>> -pgoff_t idx, unsigned pageofs,
>> -struct z_erofs_vle_workgroup **grp_ret,
>> -enum z_erofs_vle_work_role *role,
>> -z_erofs_vle_owned_workgrp_t *owned_head,
>> -bool *hosted)
>> +z_erofs_vle_work_lookup(const struct z_erofs_vle_work_finder *f)
>> {
>> bool tag, primary;
>> struct erofs_workgroup *egrp;
>> struct z_erofs_vle_workgroup *grp;
>> struct z_erofs_vle_work *work;
>>
>> -egrp = erofs_find_workgroup(sb, idx, );
>> +egrp = erofs_find_workgroup(f->sb, f->idx, );
>> if (egrp == NULL) {
>> -*grp_ret = NULL;
>> +*f->grp_ret = NULL;
>
> All these pointers to pointer seem a bit messy. Just do this:
>
> struct z_erofs_vle_workgroup *grp;
>
> Then replace "grp" in z_erofs_vle_work_iter_begin() with finder.grp;
>
I wrote this because I am not sure of all compiler behaviors.
Notice that the struct `struct z_erofs_vle_work_finder' has been all marked as
const.
If I use `struct z_erofs_vle_workgroup *grp;' and drop the `const' decorator,
compilers could do some re-read operations bacause its value could potentially
change by its caller at the same time.
Thanks,
Gao Xiang
> regards,
> dan carpenter
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] staging: erofs: cleanup z_erofs_vle_work_{lookup, register}
On Sun, Aug 12, 2018 at 10:01:46PM +0800, Chao Yu wrote:
> From: Gao Xiang
>
> This patch introduces 'struct z_erofs_vle_work_finder' to clean up
> arguments of z_erofs_vle_work_lookup and z_erofs_vle_work_register.
>
> Signed-off-by: Gao Xiang
> Reviewed-by: Chao Yu
> Signed-off-by: Chao Yu
> ---
> drivers/staging/erofs/unzip_vle.c | 89 ---
> 1 file changed, 47 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/staging/erofs/unzip_vle.c
> b/drivers/staging/erofs/unzip_vle.c
> index b2e05e2b4116..5032b3b05de1 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -271,36 +271,42 @@ static inline bool try_to_claim_workgroup(
> return true;/* lucky, I am the followee :) */
> }
>
> +struct z_erofs_vle_work_finder {
> + struct super_block *sb;
> + pgoff_t idx;
> + unsigned pageofs;
> +
> + struct z_erofs_vle_workgroup **grp_ret;
> + enum z_erofs_vle_work_role *role;
> + z_erofs_vle_owned_workgrp_t *owned_head;
> + bool *hosted;
> +};
> +
> static struct z_erofs_vle_work *
> -z_erofs_vle_work_lookup(struct super_block *sb,
> - pgoff_t idx, unsigned pageofs,
> - struct z_erofs_vle_workgroup **grp_ret,
> - enum z_erofs_vle_work_role *role,
> - z_erofs_vle_owned_workgrp_t *owned_head,
> - bool *hosted)
> +z_erofs_vle_work_lookup(const struct z_erofs_vle_work_finder *f)
> {
> bool tag, primary;
> struct erofs_workgroup *egrp;
> struct z_erofs_vle_workgroup *grp;
> struct z_erofs_vle_work *work;
>
> - egrp = erofs_find_workgroup(sb, idx, );
> + egrp = erofs_find_workgroup(f->sb, f->idx, );
> if (egrp == NULL) {
> - *grp_ret = NULL;
> + *f->grp_ret = NULL;
All these pointers to pointer seem a bit messy. Just do this:
struct z_erofs_vle_workgroup *grp;
Then replace "grp" in z_erofs_vle_work_iter_begin() with finder.grp;
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
