There's a problem with current streams after I switched them from
iohelper to thread implementation. Previously, iohelper made sure
not to exceed specified @length resulting in the pipe EOF
appearing at the exact right moment (the pipe was used to tunnel
the data from the iohelper to the daemon). Anyway, when switching
to thread I had to write the I/O code from scratch. Whilst doing
that I took an inspiration from the iohelper code, but since the
usage of pipe switched to slightly different meaning, there was
no 1:1 relationship between the codes.
Moreover, after introducing VIR_FDSTREAM_MSG_TYPE_HOLE, the
condition that should made sure we won't exceed @length was
completely wrong.
The fix is to:
a) account for holes for @length
b) cap not just data sections but holes too (if @length would be
exceeded)
For this purpose, the condition needs to be brought closer to the
code that handles holes and data sections.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/util/virfdstream.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 7ee58be13..ae6f78e01 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -420,6 +420,8 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
const int fdout,
const char *fdinname,
const char *fdoutname,
+ size_t length,
+ size_t total,
size_t *dataLen,
size_t buflen)
{
@@ -433,10 +435,18 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
if (virFileInData(fdin, &inData, §ionLen) < 0)
goto error;
+ if (length &&
+ sectionLen > length - total)
+ sectionLen = length - total;
+
if (inData)
*dataLen = sectionLen;
}
+ if (length &&
+ buflen > length - total)
+ buflen = length - total;
+
if (VIR_ALLOC(msg) < 0)
goto error;
@@ -578,13 +588,6 @@ virFDStreamThread(void *opaque)
while (1) {
ssize_t got;
- if (length &&
- (length - total) < buflen)
- buflen = length - total;
-
- if (buflen == 0)
- break; /* End of requested data from client */
-
while (doRead == (fdst->msg != NULL) &&
!fdst->threadQuit) {
if (virCondWait(&fdst->threadCond, &fdst->parent.lock)) {
@@ -608,6 +611,7 @@ virFDStreamThread(void *opaque)
got = virFDStreamThreadDoRead(fdst, sparse,
fdin, fdout,
fdinname, fdoutname,
+ length, total,
&dataLen, buflen);
else
got = virFDStreamThreadDoWrite(fdst, sparse,
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jun 05, 2017 at 02:53:19PM +0200, Michal Privoznik wrote: >There's a problem with current streams after I switched them from >iohelper to thread implementation. Previously, iohelper made sure >not to exceed specified @length resulting in the pipe EOF >appearing at the exact right moment (the pipe was used to tunnel >the data from the iohelper to the daemon). Anyway, when switching >to thread I had to write the I/O code from scratch. Whilst doing >that I took an inspiration from the iohelper code, but since the >usage of pipe switched to slightly different meaning, there was >no 1:1 relationship between the codes. > >Moreover, after introducing VIR_FDSTREAM_MSG_TYPE_HOLE, the >condition that should made sure we won't exceed @length was >completely wrong. > >The fix is to: > >a) account for holes for @length >b) cap not just data sections but holes too (if @length would be >exceeded) > >For this purpose, the condition needs to be brought closer to the >code that handles holes and data sections. > >Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >--- > src/util/virfdstream.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/05/2017 06:53 AM, Michal Privoznik wrote: > There's a problem with current streams after I switched them from > iohelper to thread implementation. Previously, iohelper made sure > not to exceed specified @length resulting in the pipe EOF > appearing at the exact right moment (the pipe was used to tunnel > the data from the iohelper to the daemon). Anyway, when switching > to thread I had to write the I/O code from scratch. Whilst doing > that I took an inspiration from the iohelper code, but since the > usage of pipe switched to slightly different meaning, there was > no 1:1 relationship between the codes. > > Moreover, after introducing VIR_FDSTREAM_MSG_TYPE_HOLE, the > condition that should made sure we won't exceed @length was > completely wrong. > > The fix is to: > > a) account for holes for @length > b) cap not just data sections but holes too (if @length would be > exceeded) > > For this purpose, the condition needs to be brought closer to the > code that handles holes and data sections. For the record, this fixes the libvirt-tck failures I mentioned. Thanks! Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/06/2017 07:24 PM, Jim Fehlig wrote: > On 06/05/2017 06:53 AM, Michal Privoznik wrote: >> There's a problem with current streams after I switched them from >> iohelper to thread implementation. Previously, iohelper made sure >> not to exceed specified @length resulting in the pipe EOF >> appearing at the exact right moment (the pipe was used to tunnel >> the data from the iohelper to the daemon). Anyway, when switching >> to thread I had to write the I/O code from scratch. Whilst doing >> that I took an inspiration from the iohelper code, but since the >> usage of pipe switched to slightly different meaning, there was >> no 1:1 relationship between the codes. >> >> Moreover, after introducing VIR_FDSTREAM_MSG_TYPE_HOLE, the >> condition that should made sure we won't exceed @length was >> completely wrong. >> >> The fix is to: >> >> a) account for holes for @length >> b) cap not just data sections but holes too (if @length would be >> exceeded) >> >> For this purpose, the condition needs to be brought closer to the >> code that handles holes and data sections. > > For the record, this fixes the libvirt-tck failures I mentioned. Thanks! I think this made it harder to hit the hang. The proper fix should be this one (still unreviewed ;-)): https://www.redhat.com/archives/libvir-list/2017-June/msg00280.html Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/07/2017 12:55 AM, Michal Privoznik wrote: > On 06/06/2017 07:24 PM, Jim Fehlig wrote: >> On 06/05/2017 06:53 AM, Michal Privoznik wrote: >>> There's a problem with current streams after I switched them from >>> iohelper to thread implementation. Previously, iohelper made sure >>> not to exceed specified @length resulting in the pipe EOF >>> appearing at the exact right moment (the pipe was used to tunnel >>> the data from the iohelper to the daemon). Anyway, when switching >>> to thread I had to write the I/O code from scratch. Whilst doing >>> that I took an inspiration from the iohelper code, but since the >>> usage of pipe switched to slightly different meaning, there was >>> no 1:1 relationship between the codes. >>> >>> Moreover, after introducing VIR_FDSTREAM_MSG_TYPE_HOLE, the >>> condition that should made sure we won't exceed @length was >>> completely wrong. >>> >>> The fix is to: >>> >>> a) account for holes for @length >>> b) cap not just data sections but holes too (if @length would be >>> exceeded) >>> >>> For this purpose, the condition needs to be brought closer to the >>> code that handles holes and data sections. >> >> For the record, this fixes the libvirt-tck failures I mentioned. Thanks! > > I think this made it harder to hit the hang. Indeed. Looping through the tests 25 times produced no failures. Before this patch, roughly 2/3 of those would fail. > The proper fix should be this one (still unreviewed ;-)): > > https://www.redhat.com/archives/libvir-list/2017-June/msg00280.html Thanks for the pointer. I reviewed it. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.