| Message ID | 42569c768066e334186ea8567847d96c8ebb0ad9.1608702853.git.elena.ufimtseva@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for multi-process Qemu | expand |
Hi On Wed, Dec 23, 2020 at 10:17 AM <elena.ufimtseva@oracle.com> wrote: > From: Elena Ufimtseva <elena.ufimtseva@oracle.com> > > Adds qio_channel_readv_full_all() to read both data and FDs. > Refactors existing code to use this function. > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > --- > include/io/channel.h | 25 +++++++++++++ > io/channel.c | 85 +++++++++++++++++++++++++++++++------------- > 2 files changed, 85 insertions(+), 25 deletions(-) > > diff --git a/include/io/channel.h b/include/io/channel.h > index 2378567d4b..429ece9a05 100644 > --- a/include/io/channel.h > +++ b/include/io/channel.h > @@ -774,6 +774,31 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc, > IOHandler *io_write, > void *opaque); > > +/** > + * qio_channel_readv_full_all: > + * @ioc: the channel object > + * @iov: the array of memory regions to read data to > + * @niov: the length of the @iov array > + * @fds: an array of file handles to read > + * @nfds: number of file handles in @fds > + * @errp: pointer to a NULL-initialized error object > + * > + * > + * Behaves like qio_channel_readv_full but will attempt > + * to read all data specified (file handles and memory regions). > + * The function will wait for all requested data > + * to be read, yielding from the current coroutine > + * if required. > + * > + * Returns: 0 if all bytes were read, or -1 on error > It may also returns -ECANCEL. I am not sure it's a good idea. + */ > + > +int qio_channel_readv_full_all(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + int **fds, size_t *nfds, > + Error **errp); > + > /** > * qio_channel_writev_full_all: > * @ioc: the channel object > diff --git a/io/channel.c b/io/channel.c > index bde1f6d0f4..5edaea1fac 100644 > --- a/io/channel.c > +++ b/io/channel.c > @@ -91,11 +91,49 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > const struct iovec *iov, > size_t niov, > Error **errp) > +{ > + int ret = qio_channel_readv_full_all(ioc, iov, niov, NULL, NULL, > errp); > + > + if (ret == -ECANCELED) { > No io/ functions use -errno return values so far. Maybe the simplest is to use the same return values as read_all_eof: * Returns: 1 if all bytes were read, 0 if end-of-file occurs * without data, or -1 on error + error_prepend(errp, > + "Unexpected end-of-file before all bytes were read: > "); > + ret = -1; > + } > + > + return ret; > +} > + > +int qio_channel_readv_all(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + Error **errp) > +{ > + int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp); > + > It looks like it would make more sense to call readv_full_all directly instead now. + if (ret == 0) { > + error_setg(errp, > + "Unexpected end-of-file before all bytes were read"); > + return -1; > + } > + if (ret == 1) { > + return 0; > + } > + > + return ret; > +} > + > +int qio_channel_readv_full_all(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + int **fds, size_t *nfds, > + Error **errp) > { > int ret = -1; > struct iovec *local_iov = g_new(struct iovec, niov); > struct iovec *local_iov_head = local_iov; > unsigned int nlocal_iov = niov; > + int **local_fds = fds; > + size_t *local_nfds = nfds; > bool partial = false; > > nlocal_iov = iov_copy(local_iov, nlocal_iov, > @@ -104,7 +142,8 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > > while (nlocal_iov > 0) { > ssize_t len; > - len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp); > + len = qio_channel_readv_full(ioc, local_iov, nlocal_iov, > local_fds, > + local_nfds, errp); > if (len == QIO_CHANNEL_ERR_BLOCK) { > if (qemu_in_coroutine()) { > qio_channel_yield(ioc, G_IO_IN); > @@ -112,20 +151,33 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > qio_channel_wait(ioc, G_IO_IN); > } > continue; > - } else if (len < 0) { > - goto cleanup; > - } else if (len == 0) { > - if (partial) { > - error_setg(errp, > - "Unexpected end-of-file before all bytes were > read"); > + } > + > + if (len <= 0) { > + size_t fd_idx = nfds ? *nfds : 0; > + if (partial && (len == 0)) { > + ret = -ECANCELED; > } else { > - ret = 0; > + ret = len; > + } > + > + while (fds && fd_idx) { > + close(*fds[fd_idx - 1]); > + fd_idx--; > + } > + > + if (fds) { > + g_free(*fds); > } > + > goto cleanup; > } > > partial = true; > iov_discard_front(&local_iov, &nlocal_iov, len); > + > + local_fds = NULL; > + local_nfds = 0; > } > > ret = 1; > @@ -135,23 +187,6 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > return ret; > } > > -int qio_channel_readv_all(QIOChannel *ioc, > - const struct iovec *iov, > - size_t niov, > - Error **errp) > -{ > - int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp); > - > - if (ret == 0) { > - ret = -1; > - error_setg(errp, > - "Unexpected end-of-file before all bytes were read"); > - } else if (ret == 1) { > - ret = 0; > - } > - return ret; > -} > - > int qio_channel_writev_all(QIOChannel *ioc, > const struct iovec *iov, > size_t niov, > -- > 2.25.GIT > >
> On Dec 23, 2020, at 6:24 AM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Wed, Dec 23, 2020 at 10:17 AM <elena.ufimtseva@oracle.com> wrote: > From: Elena Ufimtseva <elena.ufimtseva@oracle.com> > > Adds qio_channel_readv_full_all() to read both data and FDs. > Refactors existing code to use this function. > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > --- > include/io/channel.h | 25 +++++++++++++ > io/channel.c | 85 +++++++++++++++++++++++++++++++------------- > 2 files changed, 85 insertions(+), 25 deletions(-) > > diff --git a/include/io/channel.h b/include/io/channel.h > index 2378567d4b..429ece9a05 100644 > --- a/include/io/channel.h > +++ b/include/io/channel.h > @@ -774,6 +774,31 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc, > IOHandler *io_write, > void *opaque); > > +/** > + * qio_channel_readv_full_all: > + * @ioc: the channel object > + * @iov: the array of memory regions to read data to > + * @niov: the length of the @iov array > + * @fds: an array of file handles to read > + * @nfds: number of file handles in @fds > + * @errp: pointer to a NULL-initialized error object > + * > + * > + * Behaves like qio_channel_readv_full but will attempt > + * to read all data specified (file handles and memory regions). > + * The function will wait for all requested data > + * to be read, yielding from the current coroutine > + * if required. > + * > + * Returns: 0 if all bytes were read, or -1 on error > > It may also returns -ECANCEL. I am not sure it's a good idea. > > + */ > + > +int qio_channel_readv_full_all(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + int **fds, size_t *nfds, > + Error **errp); > + > /** > * qio_channel_writev_full_all: > * @ioc: the channel object > diff --git a/io/channel.c b/io/channel.c > index bde1f6d0f4..5edaea1fac 100644 > --- a/io/channel.c > +++ b/io/channel.c > @@ -91,11 +91,49 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > const struct iovec *iov, > size_t niov, > Error **errp) > +{ > + int ret = qio_channel_readv_full_all(ioc, iov, niov, NULL, NULL, errp); > + > + if (ret == -ECANCELED) { > > No io/ functions use -errno return values so far. > > Maybe the simplest is to use the same return values as read_all_eof: > * Returns: 1 if all bytes were read, 0 if end-of-file occurs > * without data, or -1 on error Hi Marc-Andre, qio_channel_readv_all_eof() also sets the Error variable when the channel is closed in the middle of reading (partial read). qio_channel_readv_full_all() needs to return a special value to qio_channel_readv_all_eof() in the case of partial reads, aside from '-1' for all other error scenarios. qio_channel_readv_full_all() returns '-ECANCEL' to identify partial reads. qio_channel_readv_full_all() could directly set this error variable for partial read scenarios, but then there wouldn't be any difference between the _full_all() version and _all_eof() version. Is that alright? Thank you! — Jag > > + error_prepend(errp, > + "Unexpected end-of-file before all bytes were read: "); > + ret = -1; > + } > + > + return ret; > +} > + > +int qio_channel_readv_all(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + Error **errp) > +{ > + int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp); > + > > It looks like it would make more sense to call readv_full_all directly instead now. > > + if (ret == 0) { > + error_setg(errp, > + "Unexpected end-of-file before all bytes were read"); > + return -1; > + } > + if (ret == 1) { > + return 0; > + } > + > + return ret; > +} > + > +int qio_channel_readv_full_all(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + int **fds, size_t *nfds, > + Error **errp) > { > int ret = -1; > struct iovec *local_iov = g_new(struct iovec, niov); > struct iovec *local_iov_head = local_iov; > unsigned int nlocal_iov = niov; > + int **local_fds = fds; > + size_t *local_nfds = nfds; > bool partial = false; > > nlocal_iov = iov_copy(local_iov, nlocal_iov, > @@ -104,7 +142,8 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > > while (nlocal_iov > 0) { > ssize_t len; > - len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp); > + len = qio_channel_readv_full(ioc, local_iov, nlocal_iov, local_fds, > + local_nfds, errp); > if (len == QIO_CHANNEL_ERR_BLOCK) { > if (qemu_in_coroutine()) { > qio_channel_yield(ioc, G_IO_IN); > @@ -112,20 +151,33 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > qio_channel_wait(ioc, G_IO_IN); > } > continue; > - } else if (len < 0) { > - goto cleanup; > - } else if (len == 0) { > - if (partial) { > - error_setg(errp, > - "Unexpected end-of-file before all bytes were read"); > + } > + > + if (len <= 0) { > + size_t fd_idx = nfds ? *nfds : 0; > + if (partial && (len == 0)) { > + ret = -ECANCELED; > } else { > - ret = 0; > + ret = len; > + } > + > + while (fds && fd_idx) { > + close(*fds[fd_idx - 1]); > + fd_idx--; > + } > + > + if (fds) { > + g_free(*fds); > } > + > goto cleanup; > } > > partial = true; > iov_discard_front(&local_iov, &nlocal_iov, len); > + > + local_fds = NULL; > + local_nfds = 0; > } > > ret = 1; > @@ -135,23 +187,6 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > return ret; > } > > -int qio_channel_readv_all(QIOChannel *ioc, > - const struct iovec *iov, > - size_t niov, > - Error **errp) > -{ > - int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp); > - > - if (ret == 0) { > - ret = -1; > - error_setg(errp, > - "Unexpected end-of-file before all bytes were read"); > - } else if (ret == 1) { > - ret = 0; > - } > - return ret; > -} > - > int qio_channel_writev_all(QIOChannel *ioc, > const struct iovec *iov, > size_t niov, > -- > 2.25.GIT > > > > -- > Marc-André Lureau
On Tue, Dec 22, 2020 at 10:14:43PM -0800, elena.ufimtseva@oracle.com wrote: > From: Elena Ufimtseva <elena.ufimtseva@oracle.com> > > Adds qio_channel_readv_full_all() to read both data and FDs. > Refactors existing code to use this function. > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > --- > include/io/channel.h | 25 +++++++++++++ > io/channel.c | 85 +++++++++++++++++++++++++++++++------------- > 2 files changed, 85 insertions(+), 25 deletions(-) > > diff --git a/include/io/channel.h b/include/io/channel.h > index 2378567d4b..429ece9a05 100644 > --- a/include/io/channel.h > +++ b/include/io/channel.h > @@ -774,6 +774,31 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc, > IOHandler *io_write, > void *opaque); > > +/** > + * qio_channel_readv_full_all: > + * @ioc: the channel object > + * @iov: the array of memory regions to read data to > + * @niov: the length of the @iov array > + * @fds: an array of file handles to read > + * @nfds: number of file handles in @fds > + * @errp: pointer to a NULL-initialized error object > + * > + * > + * Behaves like qio_channel_readv_full but will attempt > + * to read all data specified (file handles and memory regions). > + * The function will wait for all requested data > + * to be read, yielding from the current coroutine > + * if required. > + * > + * Returns: 0 if all bytes were read, or -1 on error > + */ > + > +int qio_channel_readv_full_all(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + int **fds, size_t *nfds, > + Error **errp); There parameters are one character mis-aligned here. > diff --git a/io/channel.c b/io/channel.c > index bde1f6d0f4..5edaea1fac 100644 > --- a/io/channel.c > +++ b/io/channel.c > @@ -91,11 +91,49 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > const struct iovec *iov, > size_t niov, > Error **errp) > +{ > + int ret = qio_channel_readv_full_all(ioc, iov, niov, NULL, NULL, errp); > + > + if (ret == -ECANCELED) { > + error_prepend(errp, > + "Unexpected end-of-file before all bytes were read: "); > + ret = -1; > + } > + > + return ret; > +} > + > +int qio_channel_readv_all(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + Error **errp) > +{ > + int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp); > + > + if (ret == 0) { > + error_setg(errp, > + "Unexpected end-of-file before all bytes were read"); > + return -1; > + } > + if (ret == 1) { > + return 0; > + } > + > + return ret; > +} > + > +int qio_channel_readv_full_all(QIOChannel *ioc, > + const struct iovec *iov, > + size_t niov, > + int **fds, size_t *nfds, > + Error **errp) > { > int ret = -1; > struct iovec *local_iov = g_new(struct iovec, niov); > struct iovec *local_iov_head = local_iov; > unsigned int nlocal_iov = niov; > + int **local_fds = fds; > + size_t *local_nfds = nfds; It doesn't look like we actually need these as local variables, as opposed to just replacing the parameters. > bool partial = false; > > nlocal_iov = iov_copy(local_iov, nlocal_iov, > @@ -104,7 +142,8 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > > while (nlocal_iov > 0) { > ssize_t len; > - len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp); > + len = qio_channel_readv_full(ioc, local_iov, nlocal_iov, local_fds, > + local_nfds, errp); > if (len == QIO_CHANNEL_ERR_BLOCK) { > if (qemu_in_coroutine()) { > qio_channel_yield(ioc, G_IO_IN); > @@ -112,20 +151,33 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > qio_channel_wait(ioc, G_IO_IN); > } > continue; > - } else if (len < 0) { > - goto cleanup; > - } else if (len == 0) { > - if (partial) { > - error_setg(errp, > - "Unexpected end-of-file before all bytes were read"); > + } > + > + if (len <= 0) { > + size_t fd_idx = nfds ? *nfds : 0; > + if (partial && (len == 0)) { > + ret = -ECANCELED; As Marc-Andre mentioned, we intentionally avoid returning 'errno' from any of the QIO functions, so I definitely don't want to see this added. If I look at the current set of APIs: qio_channel_readv_full qio_channel_readv qio_channel_readv_all qio_channel_readv_all_eof qio_channel_read qio_channel_read_all qio_channel_read_all_eof I think the problem is that you're introducing just 1 new function, when we really should have two in order to complete the code pattern we have. ie qio_channel_readv_full_all qio_channel_readv_full_all_eof the former should call the latter. > } else { > - ret = 0; > + ret = len; > + } > + > + while (fds && fd_idx) { > + close(*fds[fd_idx - 1]); > + fd_idx--; > + } > + > + if (fds) { > + g_free(*fds); > } > + > goto cleanup; > } > > partial = true; > iov_discard_front(&local_iov, &nlocal_iov, len); > + > + local_fds = NULL; > + local_nfds = 0; > } > > ret = 1; > @@ -135,23 +187,6 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, > return ret; > } > > -int qio_channel_readv_all(QIOChannel *ioc, > - const struct iovec *iov, > - size_t niov, > - Error **errp) > -{ > - int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp); > - > - if (ret == 0) { > - ret = -1; > - error_setg(errp, > - "Unexpected end-of-file before all bytes were read"); > - } else if (ret == 1) { > - ret = 0; > - } > - return ret; > -} > - Try not to mix in code movement with functional changes, as it obscures the diffs. IOW, if you want to re-arrange funtions in the file, do this as a separate patch first. Regards, Daniel
diff --git a/include/io/channel.h b/include/io/channel.h index 2378567d4b..429ece9a05 100644 --- a/include/io/channel.h +++ b/include/io/channel.h @@ -774,6 +774,31 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc, IOHandler *io_write, void *opaque); +/** + * qio_channel_readv_full_all: + * @ioc: the channel object + * @iov: the array of memory regions to read data to + * @niov: the length of the @iov array + * @fds: an array of file handles to read + * @nfds: number of file handles in @fds + * @errp: pointer to a NULL-initialized error object + * + * + * Behaves like qio_channel_readv_full but will attempt + * to read all data specified (file handles and memory regions). + * The function will wait for all requested data + * to be read, yielding from the current coroutine + * if required. + * + * Returns: 0 if all bytes were read, or -1 on error + */ + +int qio_channel_readv_full_all(QIOChannel *ioc, + const struct iovec *iov, + size_t niov, + int **fds, size_t *nfds, + Error **errp); + /** * qio_channel_writev_full_all: * @ioc: the channel object diff --git a/io/channel.c b/io/channel.c index bde1f6d0f4..5edaea1fac 100644 --- a/io/channel.c +++ b/io/channel.c @@ -91,11 +91,49 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, const struct iovec *iov, size_t niov, Error **errp) +{ + int ret = qio_channel_readv_full_all(ioc, iov, niov, NULL, NULL, errp); + + if (ret == -ECANCELED) { + error_prepend(errp, + "Unexpected end-of-file before all bytes were read: "); + ret = -1; + } + + return ret; +} + +int qio_channel_readv_all(QIOChannel *ioc, + const struct iovec *iov, + size_t niov, + Error **errp) +{ + int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp); + + if (ret == 0) { + error_setg(errp, + "Unexpected end-of-file before all bytes were read"); + return -1; + } + if (ret == 1) { + return 0; + } + + return ret; +} + +int qio_channel_readv_full_all(QIOChannel *ioc, + const struct iovec *iov, + size_t niov, + int **fds, size_t *nfds, + Error **errp) { int ret = -1; struct iovec *local_iov = g_new(struct iovec, niov); struct iovec *local_iov_head = local_iov; unsigned int nlocal_iov = niov; + int **local_fds = fds; + size_t *local_nfds = nfds; bool partial = false; nlocal_iov = iov_copy(local_iov, nlocal_iov, @@ -104,7 +142,8 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, while (nlocal_iov > 0) { ssize_t len; - len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp); + len = qio_channel_readv_full(ioc, local_iov, nlocal_iov, local_fds, + local_nfds, errp); if (len == QIO_CHANNEL_ERR_BLOCK) { if (qemu_in_coroutine()) { qio_channel_yield(ioc, G_IO_IN); @@ -112,20 +151,33 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, qio_channel_wait(ioc, G_IO_IN); } continue; - } else if (len < 0) { - goto cleanup; - } else if (len == 0) { - if (partial) { - error_setg(errp, - "Unexpected end-of-file before all bytes were read"); + } + + if (len <= 0) { + size_t fd_idx = nfds ? *nfds : 0; + if (partial && (len == 0)) { + ret = -ECANCELED; } else { - ret = 0; + ret = len; + } + + while (fds && fd_idx) { + close(*fds[fd_idx - 1]); + fd_idx--; + } + + if (fds) { + g_free(*fds); } + goto cleanup; } partial = true; iov_discard_front(&local_iov, &nlocal_iov, len); + + local_fds = NULL; + local_nfds = 0; } ret = 1; @@ -135,23 +187,6 @@ int qio_channel_readv_all_eof(QIOChannel *ioc, return ret; } -int qio_channel_readv_all(QIOChannel *ioc, - const struct iovec *iov, - size_t niov, - Error **errp) -{ - int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp); - - if (ret == 0) { - ret = -1; - error_setg(errp, - "Unexpected end-of-file before all bytes were read"); - } else if (ret == 1) { - ret = 0; - } - return ret; -} - int qio_channel_writev_all(QIOChannel *ioc, const struct iovec *iov, size_t niov,