| Message ID | 20200623175534.38286-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vvfat: Two small patches | expand |
On 6/23/20 12:55 PM, Kevin Wolf wrote: > array_remove_slice() calls array_roll() with array->next - 1 as the > destination index. This is only correct for count == 1, otherwise we're > writing past the end of the array. array->next - count would be correct. > > However, this is the only place ever calling array_roll(), so this > rather complicated operation isn't even necessary. > > Fix the problem and simplify the code by replacing it with a single > memmove() call. array_roll() can now be removed. > > Reported-by: Nathan Huckleberry <nhuck15@gmail.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/vvfat.c | 42 +++++------------------------------------- > 1 file changed, 5 insertions(+), 37 deletions(-) > > diff --git a/block/vvfat.c b/block/vvfat.c > index 2fab371258..d6e464c595 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -140,48 +140,16 @@ static inline void* array_insert(array_t* array,unsigned int index,unsigned int > return array->pointer+index*array->item_size; > } > > -/* this performs a "roll", so that the element which was at index_from becomes > - * index_to, but the order of all other elements is preserved. */ > -static inline int array_roll(array_t* array,int index_to,int index_from,int count) If I understand the intent from just the comment, the old code would take a directory listing of six files: ABCDEF and on the request to delete file C, would produce: ABFDE by moving just F, instead of all of DEF. That might be what legacy FAT filesystems do natively, but I don't see it as a mandatory correctness issue; ABDEF is still a directory with the same contents. And the bug for reading beyond array bounds when deleting more than one file is indeed nasty, so your simpler code is fine. > static inline int array_remove_slice(array_t* array,int index, int count) > { > assert(index >=0); > assert(count > 0); > assert(index + count <= array->next); > - if(array_roll(array,array->next-1,index,count)) > - return -1; > + > + memmove(array->pointer + index * array->item_size, > + array->pointer + (index + count) * array->item_size, > + (array->next - index - count) * array->item_size); > + Reviewed-by: Eric Blake <eblake@redhat.com> > array->next -= count; > return 0; > } >
Am 23.06.2020 um 20:30 hat Eric Blake geschrieben: > On 6/23/20 12:55 PM, Kevin Wolf wrote: > > array_remove_slice() calls array_roll() with array->next - 1 as the > > destination index. This is only correct for count == 1, otherwise we're > > writing past the end of the array. array->next - count would be correct. > > > > However, this is the only place ever calling array_roll(), so this > > rather complicated operation isn't even necessary. > > > > Fix the problem and simplify the code by replacing it with a single > > memmove() call. array_roll() can now be removed. > > > > Reported-by: Nathan Huckleberry <nhuck15@gmail.com> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/vvfat.c | 42 +++++------------------------------------- > > 1 file changed, 5 insertions(+), 37 deletions(-) > > > > diff --git a/block/vvfat.c b/block/vvfat.c > > index 2fab371258..d6e464c595 100644 > > --- a/block/vvfat.c > > +++ b/block/vvfat.c > > @@ -140,48 +140,16 @@ static inline void* array_insert(array_t* array,unsigned int index,unsigned int > > return array->pointer+index*array->item_size; > > } > > -/* this performs a "roll", so that the element which was at index_from becomes > > - * index_to, but the order of all other elements is preserved. */ > > -static inline int array_roll(array_t* array,int index_to,int index_from,int count) > > If I understand the intent from just the comment, the old code would take a > directory listing of six files: > > ABCDEF > > and on the request to delete file C, would produce: > > ABFDE > > by moving just F, instead of all of DEF. I think what the old code did was actually moving C, not F, so you get: ABDEFC And then you reduce the array size by one so that C isn't visible any more. My code preserves this behaviour, except that the invisible final element is a copy of F instead C now. Kevin
diff --git a/block/vvfat.c b/block/vvfat.c index 2fab371258..d6e464c595 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -140,48 +140,16 @@ static inline void* array_insert(array_t* array,unsigned int index,unsigned int return array->pointer+index*array->item_size; } -/* this performs a "roll", so that the element which was at index_from becomes - * index_to, but the order of all other elements is preserved. */ -static inline int array_roll(array_t* array,int index_to,int index_from,int count) -{ - char* buf; - char* from; - char* to; - int is; - - if(!array || - index_to<0 || index_to>=array->next || - index_from<0 || index_from>=array->next) - return -1; - - if(index_to==index_from) - return 0; - - is=array->item_size; - from=array->pointer+index_from*is; - to=array->pointer+index_to*is; - buf=g_malloc(is*count); - memcpy(buf,from,is*count); - - if(index_to<index_from) - memmove(to+is*count,to,from-to); - else - memmove(from,from+is*count,to-from); - - memcpy(to,buf,is*count); - - g_free(buf); - - return 0; -} - static inline int array_remove_slice(array_t* array,int index, int count) { assert(index >=0); assert(count > 0); assert(index + count <= array->next); - if(array_roll(array,array->next-1,index,count)) - return -1; + + memmove(array->pointer + index * array->item_size, + array->pointer + (index + count) * array->item_size, + (array->next - index - count) * array->item_size); + array->next -= count; return 0; }
array_remove_slice() calls array_roll() with array->next - 1 as the destination index. This is only correct for count == 1, otherwise we're writing past the end of the array. array->next - count would be correct. However, this is the only place ever calling array_roll(), so this rather complicated operation isn't even necessary. Fix the problem and simplify the code by replacing it with a single memmove() call. array_roll() can now be removed. Reported-by: Nathan Huckleberry <nhuck15@gmail.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vvfat.c | 42 +++++------------------------------------- 1 file changed, 5 insertions(+), 37 deletions(-)