Re: [PATCH] darray: Fix bug in the darray_remove() macro
Damien Grassart <[email protected]> Tue, 29 Aug 2017 12:08:35 +0200
| Newsgroups | org.ozlabs.lists.ccan |
|---|---|
| Message-ID | <CADZuF1mLkhZxr+p8PhyMHjhznUTgwH=nkLA5vmRp+DaaodUz_g@mail.gmail.com> |
--===============5180679098904258740== Content-Type: multipart/alternative; boundary="001a11c14d7230a5820557e19844" --001a11c14d7230a5820557e19844 Content-Type: text/plain; charset="UTF-8" Sorry about the confusion, I'll resend the whole set. On Tue, Aug 29, 2017 at 6:54 AM, David Gibson <[email protected]> wrote: > On Mon, Aug 28, 2017 at 07:09:35AM +0200, Damien Grassart wrote: > > The memmove() call should be using the index argument to determine the > > number of bytes to copy. To be consistent with the rest of the code, > > we should also not evaluate the index parameter multiple > > times. Calling this with rand() % arr.size would otherwise generally > > segfault. > > > > Finally, we want to avoid using "index" as an identifier so as to not > > shadow index(3) in the C library. > > Uh.. sorry, I think we're in a state of confusion because applied some > of the patches then removed them again due to problems discovered > later. > > Can you please rebase on the latest ccan tree and send me the whole > set of patches as a batch. > > > > > Signed-off-by: Damien Grassart <[email protected]> > > --- > > ccan/darray/darray.h | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h > > index 82726c05..58470fde 100644 > > --- a/ccan/darray/darray.h > > +++ b/ccan/darray/darray.h > > @@ -170,8 +170,8 @@ typedef darray(unsigned long) darray_ulong; > > memmove((arr).item+1, (arr).item, > ((arr).size-1)*sizeof(*(arr).item)); \ > > (arr).item[0] = (__VA_ARGS__); \ > > } while(0) > > -#define darray_insert(arr, index, ...) do { \ > > - size_t index_ = index; \ > > +#define darray_insert(arr, i, ...) do { \ > > + size_t index_ = (i); \ > > darray_resize(arr, (arr).size+1); \ > > memmove((arr).item+index_+1, (arr).item+index_, > ((arr).size-index_-1)*sizeof(*(arr).item)); \ > > (arr).item[index_] = (__VA_ARGS__); \ > > @@ -230,9 +230,10 @@ typedef darray(unsigned long) darray_ulong; > > #define darray_pop(arr) ((arr).item[--(arr).size]) > > #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NULL) > > /* Warning, slow: Requires copying all elements after removed item. */ > > -#define darray_remove(arr, index) do { \ > > - if (index < arr.size-1) \ > > - memmove(&(arr).item[index], &(arr).item[index+1], > ((arr).size-1-i)*sizeof(*(arr).item)); \ > > +#define darray_remove(arr, i) do { \ > > + size_t index_ = (i); \ > > + if (index_ < arr.size-1) \ > > + memmove(&(arr).item[index_], &(arr).item[index_+1], > ((arr).size-1-index_)*sizeof(*(arr).item)); \ > > (arr).size--; \ > > } while(0) > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ > _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson > --001a11c14d7230a5820557e19844 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr">Sorry about the confusion, I'll resend the whole set.<= /div><div class=3D"gmail_extra"><br><div class=3D"gmail_quote">On Tue, Aug = 29, 2017 at 6:54 AM, David Gibson <span dir=3D"ltr"><<a href=3D"mailto:d= [email protected]" target=3D"_blank">[email protected]</= a>></span> wrote:<br><blockquote class=3D"gmail_quote" style=3D"margin:0= 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=3D"">On = Mon, Aug 28, 2017 at 07:09:35AM +0200, Damien Grassart wrote:<br> > The memmove() call should be using the index argument to determine the= <br> > number of bytes to copy. To be consistent with the rest of the code,<b= r> > we should also not evaluate the index parameter multiple<br> > times. Calling this with rand() % arr.size would otherwise generally<b= r> > segfault.<br> ><br> > Finally, we want to avoid using "index" as an identifier so = as to not<br> > shadow index(3) in the C library.<br> <br> </span>Uh.. sorry, I think we're in a state of confusion because applie= d some<br> of the patches then removed them again due to problems discovered<br> later.<br> <br> Can you please rebase on the latest ccan tree and send me the whole<br> set of patches as a batch.<br> <div class=3D"HOEnZb"><div class=3D"h5"><br> ><br> > Signed-off-by: Damien Grassart <<a href=3D"mailto:[email protected]= om">[email protected]</a>><br> > ---<br> >=C2=A0 ccan/darray/darray.h | 11 ++++++-----<br> >=C2=A0 1 file changed, 6 insertions(+), 5 deletions(-)<br> ><br> > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h<br> > index 82726c05..58470fde 100644<br> > --- a/ccan/darray/darray.h<br> > +++ b/ccan/darray/darray.h<br> > @@ -170,8 +170,8 @@ typedef darray(unsigned long)=C2=A0 darray_ulong;<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memmove((arr).it= em+1, (arr).item, ((arr).size-1)*sizeof(*(arr).<wbr>item)); \<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(arr).item[0] = =3D (__VA_ARGS__); \<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0} while(0)<br> > -#define darray_insert(arr, index, ...) do { \<br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0size_t index_ =3D ind= ex; \<br> > +#define darray_insert(arr, i, ...) do { \<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0size_t index_ =3D (i)= ; \<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0darray_resize(ar= r, (arr).size+1); \<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memmove((arr).it= em+index_+1, (arr).item+index_, ((arr).size-index_-1)*sizeof(*<wbr>(arr).it= em)); \<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(arr).item[index= _] =3D (__VA_ARGS__); \<br> > @@ -230,9 +230,10 @@ typedef darray(unsigned long)=C2=A0 darray_ulong;= <br> >=C2=A0 #define darray_pop(arr) ((arr).item[--(arr).size])<br> >=C2=A0 #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NU= LL)<br> >=C2=A0 /* Warning, slow: Requires copying all elements after removed it= em. */<br> > -#define darray_remove(arr, index) do { \<br> > -=C2=A0 =C2=A0 =C2=A0if (index < arr.size-1)=C2=A0 =C2=A0 \<br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memmove(&(arr).it= em[index], &(arr).item[index+1], ((arr).size-1-i)*sizeof(*(arr)<wbr>.it= em)); \<br> > +#define darray_remove(arr, i) do { \<br> > +=C2=A0 =C2=A0 =C2=A0size_t index_ =3D (i); \<br> > +=C2=A0 =C2=A0 =C2=A0if (index_ < arr.size-1)=C2=A0 =C2=A0 \<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memmove(&(arr).it= em[index_], &(arr).item[index_+1], ((arr).size-1-index_)*sizeof(*<wbr>(= arr).item)); \<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0(arr).size--;=C2=A0 \<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0} while(0)<br> ><br> <br> --<br> </div></div><div class=3D"HOEnZb"><div class=3D"h5">David Gibson=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | I'll have= my music baroque, and my code<br> david AT <a href=3D"http://gibson.dropbear.id.au" rel=3D"noreferrer" target= =3D"_blank">gibson.dropbear.id.au</a>=C2=A0 | minimalist, thank you.=C2=A0 = NOT _the_ _other_<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | _way_ _around_!<br> <a href=3D"http://www.ozlabs.org/~dgibson" rel=3D"noreferrer" target=3D"_bl= ank">http://www.ozlabs.org/~dgibson</a><br> </div></div></blockquote></div><br></div> --001a11c14d7230a5820557e19844-- --===============5180679098904258740== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KY2NhbiBtYWls aW5nIGxpc3QKY2NhbkBsaXN0cy5vemxhYnMub3JnCmh0dHBzOi8vbGlzdHMub3psYWJzLm9yZy9s aXN0aW5mby9jY2FuCg== --===============5180679098904258740==--