| Message ID | 20220824085231.1630804-1-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] util/main-loop: Fix maximum number of wait objects for win32 | expand |
On Wed, Aug 24, 2022 at 4:52 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > From: Bin Meng <bin.meng@windriver.com> > > The maximum number of wait objects for win32 should be > MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1. > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > Changes in v3: > - move the check of adding the same HANDLE twice to a separete patch > > Changes in v2: > - fix the logic in qemu_add_wait_object() to avoid adding > the same HANDLE twice > > util/main-loop.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/util/main-loop.c b/util/main-loop.c > index f00a25451b..cb018dc33c 100644 > --- a/util/main-loop.c > +++ b/util/main-loop.c > @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque) > /* Wait objects support */ > typedef struct WaitObjects { > int num; > - int revents[MAXIMUM_WAIT_OBJECTS + 1]; > - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; > - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; > - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; > + int revents[MAXIMUM_WAIT_OBJECTS]; > + HANDLE events[MAXIMUM_WAIT_OBJECTS]; > + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; > + void *opaque[MAXIMUM_WAIT_OBJECTS]; > } WaitObjects; > > static WaitObjects wait_objects = {0}; > @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) > if (w->events[i] == handle) { > found = 1; > } > + if (i == MAXIMUM_WAIT_OBJECTS - 1) { > + break; > + } > if (found) { > w->events[i] = w->events[i + 1]; > w->func[i] = w->func[i + 1]; > -- Ping for this series?
On Fri, Sep 2, 2022 at 12:19 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Wed, Aug 24, 2022 at 4:52 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > From: Bin Meng <bin.meng@windriver.com> > > > > The maximum number of wait objects for win32 should be > > MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1. > > > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > --- > > > > Changes in v3: > > - move the check of adding the same HANDLE twice to a separete patch > > > > Changes in v2: > > - fix the logic in qemu_add_wait_object() to avoid adding > > the same HANDLE twice > > > > util/main-loop.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/util/main-loop.c b/util/main-loop.c > > index f00a25451b..cb018dc33c 100644 > > --- a/util/main-loop.c > > +++ b/util/main-loop.c > > @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque) > > /* Wait objects support */ > > typedef struct WaitObjects { > > int num; > > - int revents[MAXIMUM_WAIT_OBJECTS + 1]; > > - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; > > - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; > > - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; > > + int revents[MAXIMUM_WAIT_OBJECTS]; > > + HANDLE events[MAXIMUM_WAIT_OBJECTS]; > > + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; > > + void *opaque[MAXIMUM_WAIT_OBJECTS]; > > } WaitObjects; > > > > static WaitObjects wait_objects = {0}; > > @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) > > if (w->events[i] == handle) { > > found = 1; > > } > > + if (i == MAXIMUM_WAIT_OBJECTS - 1) { > > + break; > > + } > > if (found) { > > w->events[i] = w->events[i + 1]; > > w->func[i] = w->func[i + 1]; > > -- > > Ping for this series? Ping?
Hi On Wed, Aug 24, 2022 at 12:52 PM Bin Meng <bmeng.cn@gmail.com> wrote: > From: Bin Meng <bin.meng@windriver.com> > > The maximum number of wait objects for win32 should be > MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1. > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > Changes in v3: > - move the check of adding the same HANDLE twice to a separete patch > > Changes in v2: > - fix the logic in qemu_add_wait_object() to avoid adding > the same HANDLE twice > > util/main-loop.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/util/main-loop.c b/util/main-loop.c > index f00a25451b..cb018dc33c 100644 > --- a/util/main-loop.c > +++ b/util/main-loop.c > @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void > *opaque) > /* Wait objects support */ > typedef struct WaitObjects { > int num; > - int revents[MAXIMUM_WAIT_OBJECTS + 1]; > - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; > - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; > - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; > + int revents[MAXIMUM_WAIT_OBJECTS]; > + HANDLE events[MAXIMUM_WAIT_OBJECTS]; > + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; > + void *opaque[MAXIMUM_WAIT_OBJECTS]; > } WaitObjects; > > static WaitObjects wait_objects = {0}; > @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, > WaitObjectFunc *func, void *opaque) > if (w->events[i] == handle) { > found = 1; > } > + if (i == MAXIMUM_WAIT_OBJECTS - 1) { > + break; > + } > hmm > if (found) { > w->events[i] = w->events[i + 1]; > w->func[i] = w->func[i + 1]; > The way deletion works is by moving the i+1 element (which is always zeroed for i == MAXIMUM_WAIT_OBJECTS) to i. After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the last value, and instead rely simply on updated w->num: if (found) { w->num--; } So your patch looks ok to me, but I prefer the current code. Paolo, what do you say?
Hi Paolo, On Tue, Sep 13, 2022 at 5:52 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Wed, Aug 24, 2022 at 12:52 PM Bin Meng <bmeng.cn@gmail.com> wrote: >> >> From: Bin Meng <bin.meng@windriver.com> >> >> The maximum number of wait objects for win32 should be >> MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1. >> >> Signed-off-by: Bin Meng <bin.meng@windriver.com> >> --- >> >> Changes in v3: >> - move the check of adding the same HANDLE twice to a separete patch >> >> Changes in v2: >> - fix the logic in qemu_add_wait_object() to avoid adding >> the same HANDLE twice >> >> util/main-loop.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/util/main-loop.c b/util/main-loop.c >> index f00a25451b..cb018dc33c 100644 >> --- a/util/main-loop.c >> +++ b/util/main-loop.c >> @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque) >> /* Wait objects support */ >> typedef struct WaitObjects { >> int num; >> - int revents[MAXIMUM_WAIT_OBJECTS + 1]; >> - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; >> - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; >> - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; >> + int revents[MAXIMUM_WAIT_OBJECTS]; >> + HANDLE events[MAXIMUM_WAIT_OBJECTS]; >> + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; >> + void *opaque[MAXIMUM_WAIT_OBJECTS]; >> } WaitObjects; >> >> static WaitObjects wait_objects = {0}; >> @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) >> if (w->events[i] == handle) { >> found = 1; >> } >> + if (i == MAXIMUM_WAIT_OBJECTS - 1) { >> + break; >> + } > > > hmm > >> >> if (found) { >> w->events[i] = w->events[i + 1]; >> w->func[i] = w->func[i + 1]; > > > The way deletion works is by moving the i+1 element (which is always zeroed for i == MAXIMUM_WAIT_OBJECTS) to i. > > After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the last value, and instead rely simply on updated w->num: > > if (found) { > w->num--; > } > > So your patch looks ok to me, but I prefer the current code. > > Paolo, what do you say? Ping? Regards, Bin
Hi Paolo, On Sun, Sep 25, 2022 at 9:07 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Paolo, > > On Tue, Sep 13, 2022 at 5:52 PM Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > > > Hi > > > > On Wed, Aug 24, 2022 at 12:52 PM Bin Meng <bmeng.cn@gmail.com> wrote: > >> > >> From: Bin Meng <bin.meng@windriver.com> > >> > >> The maximum number of wait objects for win32 should be > >> MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1. > >> > >> Signed-off-by: Bin Meng <bin.meng@windriver.com> > >> --- > >> > >> Changes in v3: > >> - move the check of adding the same HANDLE twice to a separete patch > >> > >> Changes in v2: > >> - fix the logic in qemu_add_wait_object() to avoid adding > >> the same HANDLE twice > >> > >> util/main-loop.c | 11 +++++++---- > >> 1 file changed, 7 insertions(+), 4 deletions(-) > >> > >> diff --git a/util/main-loop.c b/util/main-loop.c > >> index f00a25451b..cb018dc33c 100644 > >> --- a/util/main-loop.c > >> +++ b/util/main-loop.c > >> @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque) > >> /* Wait objects support */ > >> typedef struct WaitObjects { > >> int num; > >> - int revents[MAXIMUM_WAIT_OBJECTS + 1]; > >> - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; > >> - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; > >> - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; > >> + int revents[MAXIMUM_WAIT_OBJECTS]; > >> + HANDLE events[MAXIMUM_WAIT_OBJECTS]; > >> + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; > >> + void *opaque[MAXIMUM_WAIT_OBJECTS]; > >> } WaitObjects; > >> > >> static WaitObjects wait_objects = {0}; > >> @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) > >> if (w->events[i] == handle) { > >> found = 1; > >> } > >> + if (i == MAXIMUM_WAIT_OBJECTS - 1) { > >> + break; > >> + } > > > > > > hmm > > > >> > >> if (found) { > >> w->events[i] = w->events[i + 1]; > >> w->func[i] = w->func[i + 1]; > > > > > > The way deletion works is by moving the i+1 element (which is always zeroed for i == MAXIMUM_WAIT_OBJECTS) to i. > > > > After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the last value, and instead rely simply on updated w->num: > > > > if (found) { > > w->num--; > > } > > > > So your patch looks ok to me, but I prefer the current code. > > > > Paolo, what do you say? > > Ping? > Ping? Regards, Bin
+more people On Mon, Oct 3, 2022 at 6:21 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Paolo, > > On Sun, Sep 25, 2022 at 9:07 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > Hi Paolo, > > > > On Tue, Sep 13, 2022 at 5:52 PM Marc-André Lureau > > <marcandre.lureau@gmail.com> wrote: > > > > > > Hi > > > > > > On Wed, Aug 24, 2022 at 12:52 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > >> > > >> From: Bin Meng <bin.meng@windriver.com> > > >> > > >> The maximum number of wait objects for win32 should be > > >> MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1. > > >> > > >> Signed-off-by: Bin Meng <bin.meng@windriver.com> > > >> --- > > >> > > >> Changes in v3: > > >> - move the check of adding the same HANDLE twice to a separete patch > > >> > > >> Changes in v2: > > >> - fix the logic in qemu_add_wait_object() to avoid adding > > >> the same HANDLE twice > > >> > > >> util/main-loop.c | 11 +++++++---- > > >> 1 file changed, 7 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/util/main-loop.c b/util/main-loop.c > > >> index f00a25451b..cb018dc33c 100644 > > >> --- a/util/main-loop.c > > >> +++ b/util/main-loop.c > > >> @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque) > > >> /* Wait objects support */ > > >> typedef struct WaitObjects { > > >> int num; > > >> - int revents[MAXIMUM_WAIT_OBJECTS + 1]; > > >> - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; > > >> - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; > > >> - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; > > >> + int revents[MAXIMUM_WAIT_OBJECTS]; > > >> + HANDLE events[MAXIMUM_WAIT_OBJECTS]; > > >> + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; > > >> + void *opaque[MAXIMUM_WAIT_OBJECTS]; > > >> } WaitObjects; > > >> > > >> static WaitObjects wait_objects = {0}; > > >> @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) > > >> if (w->events[i] == handle) { > > >> found = 1; > > >> } > > >> + if (i == MAXIMUM_WAIT_OBJECTS - 1) { > > >> + break; > > >> + } > > > > > > > > > hmm > > > > > >> > > >> if (found) { > > >> w->events[i] = w->events[i + 1]; > > >> w->func[i] = w->func[i + 1]; > > > > > > > > > The way deletion works is by moving the i+1 element (which is always zeroed for i == MAXIMUM_WAIT_OBJECTS) to i. > > > > > > After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the last value, and instead rely simply on updated w->num: > > > > > > if (found) { > > > w->num--; > > > } > > > > > > So your patch looks ok to me, but I prefer the current code. > > > > > > Paolo, what do you say? > > > > Ping? > > > > Ping? > Could this series be merged? Thanks, Regards, Bin
+Daniel, On Tue, Oct 11, 2022 at 8:04 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > +more people > > On Mon, Oct 3, 2022 at 6:21 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > Hi Paolo, > > > > On Sun, Sep 25, 2022 at 9:07 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > Hi Paolo, > > > > > > On Tue, Sep 13, 2022 at 5:52 PM Marc-André Lureau > > > <marcandre.lureau@gmail.com> wrote: > > > > > > > > Hi > > > > > > > > On Wed, Aug 24, 2022 at 12:52 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > > >> > > > >> From: Bin Meng <bin.meng@windriver.com> > > > >> > > > >> The maximum number of wait objects for win32 should be > > > >> MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1. > > > >> > > > >> Signed-off-by: Bin Meng <bin.meng@windriver.com> > > > >> --- > > > >> > > > >> Changes in v3: > > > >> - move the check of adding the same HANDLE twice to a separete patch > > > >> > > > >> Changes in v2: > > > >> - fix the logic in qemu_add_wait_object() to avoid adding > > > >> the same HANDLE twice > > > >> > > > >> util/main-loop.c | 11 +++++++---- > > > >> 1 file changed, 7 insertions(+), 4 deletions(-) > > > >> > > > >> diff --git a/util/main-loop.c b/util/main-loop.c > > > >> index f00a25451b..cb018dc33c 100644 > > > >> --- a/util/main-loop.c > > > >> +++ b/util/main-loop.c > > > >> @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque) > > > >> /* Wait objects support */ > > > >> typedef struct WaitObjects { > > > >> int num; > > > >> - int revents[MAXIMUM_WAIT_OBJECTS + 1]; > > > >> - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; > > > >> - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; > > > >> - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; > > > >> + int revents[MAXIMUM_WAIT_OBJECTS]; > > > >> + HANDLE events[MAXIMUM_WAIT_OBJECTS]; > > > >> + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; > > > >> + void *opaque[MAXIMUM_WAIT_OBJECTS]; > > > >> } WaitObjects; > > > >> > > > >> static WaitObjects wait_objects = {0}; > > > >> @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) > > > >> if (w->events[i] == handle) { > > > >> found = 1; > > > >> } > > > >> + if (i == MAXIMUM_WAIT_OBJECTS - 1) { > > > >> + break; > > > >> + } > > > > > > > > > > > > hmm > > > > > > > >> > > > >> if (found) { > > > >> w->events[i] = w->events[i + 1]; > > > >> w->func[i] = w->func[i + 1]; > > > > > > > > > > > > The way deletion works is by moving the i+1 element (which is always zeroed for i == MAXIMUM_WAIT_OBJECTS) to i. > > > > > > > > After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the last value, and instead rely simply on updated w->num: > > > > > > > > if (found) { > > > > w->num--; > > > > } > > > > > > > > So your patch looks ok to me, but I prefer the current code. > > > > > > > > Paolo, what do you say? > > > > > > Ping? > > > > > > > Ping? > > > > Could this series be merged? Thanks, > Since Polo keeps silent, Daniel would you help queue this series? Thanks! Regards, Bin
On Wed, Aug 24, 2022 at 04:52:29PM +0800, Bin Meng wrote: > From: Bin Meng <bin.meng@windriver.com> > > The maximum number of wait objects for win32 should be > MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1. > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > Changes in v3: > - move the check of adding the same HANDLE twice to a separete patch > > Changes in v2: > - fix the logic in qemu_add_wait_object() to avoid adding > the same HANDLE twice > > util/main-loop.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/util/main-loop.c b/util/main-loop.c > index f00a25451b..cb018dc33c 100644 > --- a/util/main-loop.c > +++ b/util/main-loop.c > @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque) > /* Wait objects support */ > typedef struct WaitObjects { > int num; > - int revents[MAXIMUM_WAIT_OBJECTS + 1]; > - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; > - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; > - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; > + int revents[MAXIMUM_WAIT_OBJECTS]; > + HANDLE events[MAXIMUM_WAIT_OBJECTS]; > + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; > + void *opaque[MAXIMUM_WAIT_OBJECTS]; > } WaitObjects; > > static WaitObjects wait_objects = {0}; > @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) > if (w->events[i] == handle) { > found = 1; > } > + if (i == MAXIMUM_WAIT_OBJECTS - 1) { > + break; > + } Took me a while to realize this was protecting the body of the next if from out of bounds access. Can we redo this to make it explicit: > if (found) { if (found && i < (MAXIMUM_WAIT_OBJECTS - 1)) { > w->events[i] = w->events[i + 1]; > w->func[i] = w->func[i + 1]; With regards, Daniel
diff --git a/util/main-loop.c b/util/main-loop.c index f00a25451b..cb018dc33c 100644 --- a/util/main-loop.c +++ b/util/main-loop.c @@ -363,10 +363,10 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque) /* Wait objects support */ typedef struct WaitObjects { int num; - int revents[MAXIMUM_WAIT_OBJECTS + 1]; - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; + int revents[MAXIMUM_WAIT_OBJECTS]; + HANDLE events[MAXIMUM_WAIT_OBJECTS]; + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; + void *opaque[MAXIMUM_WAIT_OBJECTS]; } WaitObjects; static WaitObjects wait_objects = {0}; @@ -395,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) if (w->events[i] == handle) { found = 1; } + if (i == MAXIMUM_WAIT_OBJECTS - 1) { + break; + } if (found) { w->events[i] = w->events[i + 1]; w->func[i] = w->func[i + 1];