Messages in this thread |  | | Subject | Re: [PATCH 3/8] staging: erofs: add error handling for xattr submodule | From | Gao Xiang <> | Date | Mon, 13 Aug 2018 20:46:28 +0800 |
| |
Hi Dan,
On 2018/8/13 20:40, Dan Carpenter wrote: > On Mon, Aug 13, 2018 at 08:17:27PM +0800, Gao Xiang wrote: >>>> @@ -294,8 +322,11 @@ static int inline_getxattr(struct inode *inode, struct getxattr_iter *it) >>>> ret = xattr_foreach(&it->it, &find_xattr_handlers, &remaining); >>>> if (ret >= 0) >>>> break; >>>> + >>>> + if (unlikely(ret != -ENOATTR)) /* -ENOMEM, -EIO, etc. */ >>> I have held off commenting on all the likely/unlikely annotations we >>> are adding because I don't know what the fast paths are in this code. >>> However, this is clearly an error path here, not on a fast path. >>> >>> Generally the rule on likely/unlikely is that they hurt readability so >>> we should only add them if it makes a difference in benchmarking. >>> >> In my opinion, return values other than 0 and ENOATTR(ENODATA) rarely happens, >> it should be in the slow path... >> > What I'm trying to say is please stop adding so many likely/unlikely > annotations. You should only add them if you have the benchmark data to > show the it really is required. > > OK, I'll follow your suggestion.
I could say it is my personal code tendency(style). If you don't like it/think them useless, I will remove them all.
Thanks for your suggestion.
Thanks, Gao Xiang
> regards, > dan carpenter >
|  |