| Message ID | 1288794584-6099-1-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: > Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() > handler that deletes its IOHandler is exposed to .fd_write() being > called on the deleted IOHandler. > > This patch fixes deletion so that .fd_read() and .fd_write() are never > called on an IOHandler that is marked for deletion. Reviewed by: Juan Quintela <quintela@redhat.com> But once here, what (appart from networking) reads and writes to the same fd? And that removes the IOHandler on read while there are write stuff pending? Later, Juan. > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > --- > vl.c | 15 ++++++++------- > 1 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/vl.c b/vl.c > index 7038952..6f56123 100644 > --- a/vl.c > +++ b/vl.c > @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking) > IOHandlerRecord *pioh; > > QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) { > - if (ioh->deleted) { > - QLIST_REMOVE(ioh, next); > - qemu_free(ioh); > - continue; > - } > - if (ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) { > + if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) { > ioh->fd_read(ioh->opaque); > } > - if (ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) { > + if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) { > ioh->fd_write(ioh->opaque); > } > + > + /* Do this last in case read/write handlers marked it for deletion */ > + if (ioh->deleted) { > + QLIST_REMOVE(ioh, next); > + qemu_free(ioh); > + } > } > }
On Wed, Nov 3, 2010 at 2:39 PM, Juan Quintela <quintela@redhat.com> wrote: > Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: >> Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() >> handler that deletes its IOHandler is exposed to .fd_write() being >> called on the deleted IOHandler. >> >> This patch fixes deletion so that .fd_read() and .fd_write() are never >> called on an IOHandler that is marked for deletion. > > Reviewed by: Juan Quintela <quintela@redhat.com> > > But once here, what (appart from networking) reads and writes to the > same fd? And that removes the IOHandler on read while there are write > stuff pending? VNC Stefan
On 11/03/2010 09:29 AM, Stefan Hajnoczi wrote: > Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() > handler that deletes its IOHandler is exposed to .fd_write() being > called on the deleted IOHandler. > > This patch fixes deletion so that .fd_read() and .fd_write() are never > called on an IOHandler that is marked for deletion. > > Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> > --- > vl.c | 15 ++++++++------- > 1 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/vl.c b/vl.c > index 7038952..6f56123 100644 > --- a/vl.c > +++ b/vl.c > @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking) > IOHandlerRecord *pioh; > > QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) { > - if (ioh->deleted) { > - QLIST_REMOVE(ioh, next); > - qemu_free(ioh); > - continue; > - } > - if (ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { > + if (!ioh->deleted&& ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { > ioh->fd_read(ioh->opaque); > } > - if (ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { > + if (!ioh->deleted&& ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { > ioh->fd_write(ioh->opaque); > } > + > + /* Do this last in case read/write handlers marked it for deletion */ > + if (ioh->deleted) { > + QLIST_REMOVE(ioh, next); > + qemu_free(ioh); > + } > } > This isn't enough. If you end up with a handler deleting the next pointer and the current pointer, you'll end up running off the end of the list. The original commit should be reverted. Regards, Anthony Liguori > } > >
On 11/03/2010 09:29 AM, Stefan Hajnoczi wrote: > Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() > handler that deletes its IOHandler is exposed to .fd_write() being > called on the deleted IOHandler. > > This patch fixes deletion so that .fd_read() and .fd_write() are never > called on an IOHandler that is marked for deletion. > > Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> > --- > vl.c | 15 ++++++++------- > 1 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/vl.c b/vl.c > index 7038952..6f56123 100644 > --- a/vl.c > +++ b/vl.c > @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking) > IOHandlerRecord *pioh; > > QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) { > - if (ioh->deleted) { > - QLIST_REMOVE(ioh, next); > - qemu_free(ioh); > - continue; > - } > - if (ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { > + if (!ioh->deleted&& ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { > ioh->fd_read(ioh->opaque); > } > - if (ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { > + if (!ioh->deleted&& ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { > ioh->fd_write(ioh->opaque); > } > + > + /* Do this last in case read/write handlers marked it for deletion */ > + if (ioh->deleted) { > + QLIST_REMOVE(ioh, next); > + qemu_free(ioh); > + } > Actually, on second thought, I think this is correct although I still think reverting is a better strategy. Regards, Anthony Liguori > } > } > >
Anthony Liguori <anthony@codemonkey.ws> wrote: > On 11/03/2010 09:29 AM, Stefan Hajnoczi wrote: >> Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() >> handler that deletes its IOHandler is exposed to .fd_write() being >> called on the deleted IOHandler. >> >> This patch fixes deletion so that .fd_read() and .fd_write() are never >> called on an IOHandler that is marked for deletion. >> >> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> >> --- >> vl.c | 15 ++++++++------- >> 1 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index 7038952..6f56123 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking) >> IOHandlerRecord *pioh; >> >> QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) { >> - if (ioh->deleted) { >> - QLIST_REMOVE(ioh, next); >> - qemu_free(ioh); >> - continue; >> - } >> - if (ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { >> + if (!ioh->deleted&& ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { >> ioh->fd_read(ioh->opaque); >> } >> - if (ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { >> + if (!ioh->deleted&& ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { >> ioh->fd_write(ioh->opaque); >> } >> + >> + /* Do this last in case read/write handlers marked it for deletion */ >> + if (ioh->deleted) { >> + QLIST_REMOVE(ioh, next); >> + qemu_free(ioh); >> + } >> } >> > > This isn't enough. If you end up with a handler deleting the next > pointer and the current pointer, you'll end up running off the end of > the list. What is the point of that? That a handler can remove itself is ok. But that a handler can remove also the next in a list that is used for other things looks pretty insane to me. > The original commit should be reverted. If that behaviour is expected, then I agree that we should revert it. But I would consider that behaviour wrong. Later, Juan. > Regards, > > Anthony Liguori > >> } >> >>
On 11/03/2010 10:12 AM, Juan Quintela wrote: > Anthony Liguori<anthony@codemonkey.ws> wrote: > >> On 11/03/2010 09:29 AM, Stefan Hajnoczi wrote: >> >>> Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() >>> handler that deletes its IOHandler is exposed to .fd_write() being >>> called on the deleted IOHandler. >>> >>> This patch fixes deletion so that .fd_read() and .fd_write() are never >>> called on an IOHandler that is marked for deletion. >>> >>> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> >>> --- >>> vl.c | 15 ++++++++------- >>> 1 files changed, 8 insertions(+), 7 deletions(-) >>> >>> diff --git a/vl.c b/vl.c >>> index 7038952..6f56123 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking) >>> IOHandlerRecord *pioh; >>> >>> QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) { >>> - if (ioh->deleted) { >>> - QLIST_REMOVE(ioh, next); >>> - qemu_free(ioh); >>> - continue; >>> - } >>> - if (ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { >>> + if (!ioh->deleted&& ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { >>> ioh->fd_read(ioh->opaque); >>> } >>> - if (ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { >>> + if (!ioh->deleted&& ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { >>> ioh->fd_write(ioh->opaque); >>> } >>> + >>> + /* Do this last in case read/write handlers marked it for deletion */ >>> + if (ioh->deleted) { >>> + QLIST_REMOVE(ioh, next); >>> + qemu_free(ioh); >>> + } >>> } >>> >>> >> This isn't enough. If you end up with a handler deleting the next >> pointer and the current pointer, you'll end up running off the end of >> the list. >> > What is the point of that? > > That a handler can remove itself is ok. > But that a handler can remove also the next in a list that is used for > other things looks pretty insane to me. > If you have multiple file descriptors registered for something and you get an EOF on one of the file descriptors, your clean-up action that happens as a result of closing the session may involve deleting more than one file descriptor callback. Regards, Anthony Liguori >> The original commit should be reverted. >> > If that behaviour is expected, then I agree that we should revert it. > But I would consider that behaviour wrong. > > Later, Juan. > > >> Regards, >> >> Anthony Liguori >> >> >>> } >>> >>> >>>
Anthony Liguori <anthony@codemonkey.ws> wrote: > On 11/03/2010 10:12 AM, Juan Quintela wrote: >> Anthony Liguori<anthony@codemonkey.ws> wrote: >> >>> On 11/03/2010 09:29 AM, Stefan Hajnoczi wrote: >>> >>>> Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() >>>> handler that deletes its IOHandler is exposed to .fd_write() being >>>> called on the deleted IOHandler. >>>> >>>> This patch fixes deletion so that .fd_read() and .fd_write() are never >>>> called on an IOHandler that is marked for deletion. >>>> >>>> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> >>>> --- >>>> vl.c | 15 ++++++++------- >>>> 1 files changed, 8 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/vl.c b/vl.c >>>> index 7038952..6f56123 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking) >>>> IOHandlerRecord *pioh; >>>> >>>> QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) { >>>> - if (ioh->deleted) { >>>> - QLIST_REMOVE(ioh, next); >>>> - qemu_free(ioh); >>>> - continue; >>>> - } >>>> - if (ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { >>>> + if (!ioh->deleted&& ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { >>>> ioh->fd_read(ioh->opaque); >>>> } >>>> - if (ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { >>>> + if (!ioh->deleted&& ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { >>>> ioh->fd_write(ioh->opaque); >>>> } >>>> + >>>> + /* Do this last in case read/write handlers marked it for deletion */ >>>> + if (ioh->deleted) { >>>> + QLIST_REMOVE(ioh, next); >>>> + qemu_free(ioh); >>>> + } >>>> } >>>> >>>> >>> This isn't enough. If you end up with a handler deleting the next >>> pointer and the current pointer, you'll end up running off the end of >>> the list. >>> >> What is the point of that? >> >> That a handler can remove itself is ok. >> But that a handler can remove also the next in a list that is used for >> other things looks pretty insane to me. >> > > If you have multiple file descriptors registered for something and you > get an EOF on one of the file descriptors, your clean-up action that > happens as a result of closing the session may involve deleting more > than one file descriptor callback. But that is completely wrong. you just put an ioh->deleted=1 for the others, and you are right, no? Later, Juan.
On Wed, Nov 3, 2010 at 6:39 PM, Juan Quintela <quintela@redhat.com> wrote: > Anthony Liguori <anthony@codemonkey.ws> wrote: >> On 11/03/2010 10:12 AM, Juan Quintela wrote: >>> Anthony Liguori<anthony@codemonkey.ws> wrote: >>> >>>> On 11/03/2010 09:29 AM, Stefan Hajnoczi wrote: >>>> >>>>> Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() >>>>> handler that deletes its IOHandler is exposed to .fd_write() being >>>>> called on the deleted IOHandler. >>>>> >>>>> This patch fixes deletion so that .fd_read() and .fd_write() are never >>>>> called on an IOHandler that is marked for deletion. >>>>> >>>>> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> >>>>> --- >>>>> vl.c | 15 ++++++++------- >>>>> 1 files changed, 8 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/vl.c b/vl.c >>>>> index 7038952..6f56123 100644 >>>>> --- a/vl.c >>>>> +++ b/vl.c >>>>> @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking) >>>>> IOHandlerRecord *pioh; >>>>> >>>>> QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) { >>>>> - if (ioh->deleted) { >>>>> - QLIST_REMOVE(ioh, next); >>>>> - qemu_free(ioh); >>>>> - continue; >>>>> - } >>>>> - if (ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { >>>>> + if (!ioh->deleted&& ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { >>>>> ioh->fd_read(ioh->opaque); >>>>> } >>>>> - if (ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { >>>>> + if (!ioh->deleted&& ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { >>>>> ioh->fd_write(ioh->opaque); >>>>> } >>>>> + >>>>> + /* Do this last in case read/write handlers marked it for deletion */ >>>>> + if (ioh->deleted) { >>>>> + QLIST_REMOVE(ioh, next); >>>>> + qemu_free(ioh); >>>>> + } >>>>> } >>>>> >>>>> >>>> This isn't enough. If you end up with a handler deleting the next >>>> pointer and the current pointer, you'll end up running off the end of >>>> the list. >>>> >>> What is the point of that? >>> >>> That a handler can remove itself is ok. >>> But that a handler can remove also the next in a list that is used for >>> other things looks pretty insane to me. >>> >> >> If you have multiple file descriptors registered for something and you >> get an EOF on one of the file descriptors, your clean-up action that >> happens as a result of closing the session may involve deleting more >> than one file descriptor callback. > > But that is completely wrong. you just put an ioh->deleted=1 for the > others, and you are right, no? Yes, other IOHandlerRecords will not be removed from the list yet, they will only be marked as ioh->deleted=1. Anthony, are you happy to merge this patch? Stefan
On 11/03/2010 09:29 AM, Stefan Hajnoczi wrote: > Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() > handler that deletes its IOHandler is exposed to .fd_write() being > called on the deleted IOHandler. > > This patch fixes deletion so that .fd_read() and .fd_write() are never > called on an IOHandler that is marked for deletion. > > Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> > Applied. Thanks. Regards, Anthony Liguori > --- > vl.c | 15 ++++++++------- > 1 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/vl.c b/vl.c > index 7038952..6f56123 100644 > --- a/vl.c > +++ b/vl.c > @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking) > IOHandlerRecord *pioh; > > QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) { > - if (ioh->deleted) { > - QLIST_REMOVE(ioh, next); > - qemu_free(ioh); > - continue; > - } > - if (ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { > + if (!ioh->deleted&& ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { > ioh->fd_read(ioh->opaque); > } > - if (ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { > + if (!ioh->deleted&& ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { > ioh->fd_write(ioh->opaque); > } > + > + /* Do this last in case read/write handlers marked it for deletion */ > + if (ioh->deleted) { > + QLIST_REMOVE(ioh, next); > + qemu_free(ioh); > + } > } > } > >
diff --git a/vl.c b/vl.c index 7038952..6f56123 100644 --- a/vl.c +++ b/vl.c @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking) IOHandlerRecord *pioh; QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) { - if (ioh->deleted) { - QLIST_REMOVE(ioh, next); - qemu_free(ioh); - continue; - } - if (ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) { + if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) { ioh->fd_read(ioh->opaque); } - if (ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) { + if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) { ioh->fd_write(ioh->opaque); } + + /* Do this last in case read/write handlers marked it for deletion */ + if (ioh->deleted) { + QLIST_REMOVE(ioh, next); + qemu_free(ioh); + } } }
Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read() handler that deletes its IOHandler is exposed to .fd_write() being called on the deleted IOHandler. This patch fixes deletion so that .fd_read() and .fd_write() are never called on an IOHandler that is marked for deletion. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- vl.c | 15 ++++++++------- 1 files changed, 8 insertions(+), 7 deletions(-)