lkml.org 
[lkml]   [2018]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v7] Add udmabuf misc device
> > >> +	if (WARN_ON(vmf->pgoff >= ubuf->pagecount))
> > >> + return VM_FAULT_SIGBUS;
> > >
> > > Just curious, when do you expect this to happen ?
> >
> > It should not. If it actually happens it would be a bug somewhere,
> > thats why the WARN_ON.
>
> But you seem to consider that this condition that should never happen still
> has a high enough chance of happening that it's worth a WARN_ON(). I was
> wondering why this one in particular, and not other conditions that also can't
> happen and are not checked through the code.

Added it while writing the code, to get any coding mistake I make
flagged right away instead of things exploding later on.

I can drop it.

> > >> + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL);
> > >
> > > sizeof(*ubuf)
> >
> > Why? Should not make a difference ...
>
> Because the day we replace
>
> struct udmabuf *ubuf;
>
> with
>
> struct udmabuf_ext *ubuf;
>
> and forget to change the next line, we'll introduce a bug. That's why
> sizeof(variable) is preferred over sizeof(type). Another reason is that I can
> easily see that
>
> ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
>
> is correct, while using sizeof(type) requires me to go and look up the
> declaration of the variable.

So it simplifies review, ok, will change it.

BTW: Maybe the kernel should pick up a neat trick from glib:

g_new0() is a macro which takes the type instead of the size as first
argument, and it casts the return value to that type. So the compiler
will throw warnings in case of a mismatch. That'll work better than
depending purely on the coder being careful and review catching the
remaining issues.

cheers,
Gerd

\
 
 \ /
  Last update: 2018-09-11 14:04    [W:0.071 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site