| Message ID | 20230731154718.31048-1-tiwai@suse.de |
|---|---|
| Headers | show |
| Series | ALSA: Generic PCM copy ops using sockptr_t | expand |
On Tue, 01 Aug 2023 19:51:39 +0200, Andy Shevchenko wrote: > > On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote: > > On Mon, 31 Jul 2023 21:40:20 +0200, > > Mark Brown wrote: > > > On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote: > > > > Mark Brown wrote: > > > > > > > > It really feels like we ought to rename, or add an alias for, the type > > > > > if we're going to start using it more widely - it's not helping to make > > > > > the code clearer. > > > > > > > That was my very first impression, too, but I changed my mind after > > > > seeing the already used code. An alias might work, either typedef or > > > > define genptr_t or such as sockptr_t. But we'll need to copy the > > > > bunch of helper functions, too... > > > > > > I would predict that if the type becomes more widely used that'll happen > > > eventually and the longer it's left the more work it'll be. > > > > That's true. The question is how more widely it'll be used, then. > > > > Is something like below what you had in mind, too? > > I agree with your proposal (uniptr_t also works for me), but see below. > > ... > > > +#include <linux/slab.h> > > +#include <linux/uaccess.h> > > But let make the list of the headers right this time. > > It seems to me that > > err.h > minmax.h // maybe not, see a remark at the bottom > string.h > types.h > > are missing. OK, makes sense to add them. > > More below. > > ... > > > + void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN); > > + > > + if (!p) > > + return ERR_PTR(-ENOMEM); > > This can use cleanup.h. Hm, I don't think it can be replaced with that. There may be more use of cleanup.h, but it's no direct alternative for kmalloc_track_caller()... > > + if (copy_from_uniptr(p, src, len)) { > > + kfree(p); > > + return ERR_PTR(-EFAULT); > > + } > > + return p; > > +} > > + > > +static inline void *memdup_uniptr_nul(uniptr_t src, size_t len) > > +{ > > + char *p = kmalloc_track_caller(len + 1, GFP_KERNEL); > > Ditto. > > > + if (!p) > > + return ERR_PTR(-ENOMEM); > > + if (copy_from_uniptr(p, src, len)) { > > + kfree(p); > > + return ERR_PTR(-EFAULT); > > + } > > + p[len] = '\0'; > > + return p; > > +} > > ... > > > +static inline long strncpy_from_uniptr(char *dst, uniptr_t src, size_t count) > > +{ > > + if (uniptr_is_kernel(src)) { > > + size_t len = min(strnlen(src.kernel, count - 1) + 1, count); > > I didn't get why do we need min()? To check the count == 0 case? > > Wouldn't > > size_t len; > > len = strnlen(src.kernel, count); > if (len == 0) > return 0; > > /* Copy a trailing NUL if found */ > if (len < count) > len++; > > be a good equivalent? A good question. I rather wonder why it can't be simple strncpy(). > > + memcpy(dst, src.kernel, len); > > + return len; > > + } > > + return strncpy_from_user(dst, src.user, count); > > +} > > ... > > > +static inline int check_zeroed_uniptr(uniptr_t src, size_t offset, size_t size) > > +{ > > + if (!uniptr_is_kernel(src)) > > Why not to align all the functions to use same conditional (either always > positive or negative)? A different person, a different taste :) But it's trivial to fix. > > + return check_zeroed_user(src.user + offset, size); > > + return memchr_inv(src.kernel + offset, 0, size) == NULL; > > +} > > ... > > Taking all remarks into account I would rather go with sockptr.h being > untouched for now, just a big > > /* DO NOT USE, it's obsolete, use uniptr.h instead! */ > > to be added. Possibly that's a safer choice. But the biggest question is whether we want a generic type or not. Let's try to ask it first. Interestingly, this file doesn't belong to any subsystem in MAINTAINERS, so I'm not sure who to be Cc'ed. Chirstoph as the original author and net dev, maybe. thanks, Takashi
On Mon, Aug 07, 2023 at 05:22:18PM +0200, Takashi Iwai wrote: > On Tue, 01 Aug 2023 19:51:39 +0200, Andy Shevchenko wrote: > > On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote: ... > I rather wonder why it can't be simple strncpy(). This is obvious. To avoid compiler warning about 0 (possible) truncation. ... > > Taking all remarks into account I would rather go with sockptr.h being > > untouched for now, just a big > > > > /* DO NOT USE, it's obsolete, use uniptr.h instead! */ > > > > to be added. > > Possibly that's a safer choice. But the biggest question is whether > we want a generic type or not. Let's try to ask it first. > > Interestingly, this file doesn't belong to any subsystem in > MAINTAINERS, so I'm not sure who to be Cc'ed. Chirstoph as the > original author and net dev, maybe. Yes, but actually it's fine to just copy and change sockptr.h to say "that's blablabla for net subsystem" (maybe this is implied by the name?). In that case we just introduce our copy and can do whatever modifications we want (see previous reply).