------------------------------ On Tue, Apr 8, 2014 6:21 PM BST Vyacheslav Dubeyko wrote: > >On Apr 8, 2014, at 9:09 PM, Anton Altaparmakov wrote: > >[snip] > >> >> Unfortunately, it doesn't make sense to have fix only in hfsplus_listxattr(). >> It fixes nothing for xattr names. Of course, you can mention name of the patch. :) >> But such partial fix doesn't make sense for me. It will be more clear way to have >> one patch for file names and another patch for xattr names. >> >> You are entitled to your opinion but I agree with Hin-Tak. This makes perfect sense as it fixes the use of a function which can otherwise cause an erroneous EIO error to be returned from listxattr(). This in itself is a worthwhile fix. Your idea of having to fix everything or nothing at all is frankly quite silly and would mean that hardly anything ever gets fixed... Fixing small problems individually is just fine, especially when they are such obvious ones. >> > >I have another opinion. :) But, of course, it is possible to see on things in >different ways. I don't think that topic of splitting fixes between patches >is really important. But my suggestion has more logical basis. > >But, anyway, I suggest to use kmem cache for the case of xattr names >instead of kmalloc. This patch fix usages of hfsplus_uni2asc(). There are only two. I think we all agree that there is a lot more work to be done in the attribute side to make sure that getting and setting long non-english attributes to work. And there will be a series of patches to it. The discussion is really on how this is split. There are two uses of HFSPLUS_MAX_STRLEN concern asc2uni(), and are correct where they are. There is one other use in super.c:319: buf->f_namelen = HFSPLUS_MAX_STRLEN; That may need some thinking. It is probably correct. For HFSPLUS_ATTR_MAX_STRLEN, besides the one I changed, there are 9 other uses as size of buffers (and a similar number in checks for length over-run, etc). As I wrote earlier, some of these are setters, rather than getters. Setters and getters are different. The setters probably should use a large enough buffer according to userland, (ie. not a file-system specific one), and let the lower-layer of the file system returns an error when one is trying to set a too-long attribute. The getters needs to cope with what listxttr() returns. So I see there should be at least two forth-coming patches related to attributes. One can bundle the attribute part of the current patch to the latter; but then that's assuming that the getter part of these further patches is logically one. It may be complex enough to require splitting up. We have 9 buffers, divisible at least now into two groups, getters and setters - it might need to be divided further. There are OS-prefixes complications along the way, so we may end up having to do 3-4 additional patches. At this point it is not obvious that half of the current patch should be deferred, and where/what it should defer to. So I am thinking just "commit early, commit often". Retrospectively - when we look back on it afterwards, in a few month's time -, the whole set of work will form a patch series; but one advantage of *not* declaring a patch series, is that it allows others to work on it. e.g. It may be important for Sergei to put his name down on one of these further patches, if claiming a change under his/her name is an important issue :-). 85+ CJK characters for file names isn't too uncommon. I have non-english file names on my hard disk with say, 50+ characters. If you insist on doing Chinese all the way, 127/3 = 42 is really a bit low for a descriptive string of any usage, but does anybody use non-english extended attributes on any OSes? Hin-Tak -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
