[v2] bfd: bread: read in a loop when running with --remote

Submitted by Omri Kramer on July 10, 2017, 11:56 a.m.

Details

Message ID 1499687819-50428-1-git-send-email-omri.kramer@gmail.com
State New
Series "bfd: bread: read in a loop when running with --remote"
Headers show

Commit Message

Omri Kramer July 10, 2017, 11:56 a.m.
When the --remote option is used, the file descriptor
used in bread is a socketand it may return after only
part of the data is read. Keep going looping until all
the data is received when --remote option is on.

Signed-off-by: Omri Kramer <omri.kramer@gmail.com>
Signed-off-by: Lior Fisch <fischlior@gmail.com>
---
 criu/bfd.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/bfd.c b/criu/bfd.c
index 8269ab1..7c54d9b 100644
--- a/criu/bfd.c
+++ b/criu/bfd.c
@@ -15,6 +15,7 @@ 
 #include "util.h"
 #include "xmalloc.h"
 #include "page.h"
+#include "cr_options.h"
 
 #undef	LOG_PREFIX
 #define LOG_PREFIX "bfd: "
@@ -298,9 +299,29 @@  int bread(struct bfd *bfd, void *buf, int size)
 {
 	struct xbuf *b = &bfd->b;
 	int more = 1, filled = 0;
+	int ret;
+	size_t curr = 0;
 
-	if (!bfd_buffered(bfd))
-		return read(bfd->fd, buf, size);
+	if (!bfd_buffered(bfd)) {
+		if(!opts.remote)
+			return read(bfd->fd, buf, size);
+		/*
+		 * Required to keep reading,
+		 * for succesfully Reading all the data
+		 * from the socket, when on remote option.
+		 */
+		while (1) {
+			ret = read(bfd->fd, buf + curr, size - curr);
+			if (ret < 0) {
+				pr_perror("Can't read from buffer\n");
+				return -1;
+			}
+
+			curr += ret;
+			if (curr == size || ret == 0)
+				return curr;
+		}
+	}
 
 	while (more > 0) {
 		int chunk;

Comments

Rodrigo Bruno July 10, 2017, 12:15 p.m.
Hi,

isn't it a good idea to extract the read loop into a separate function (I
thought we already had one in the past...)?

I mean, the read syscall is not guaranteed to actually read the number
requested bytes (not even if we are reading from a local file).

Besides, if I remember correctly, there are multiple code locations that
could use that new function.

cheers,
rodrigo

2017-07-10 12:56 GMT+01:00 Omri Kramer <omri.kramer@gmail.com>:

> When the --remote option is used, the file descriptor
> used in bread is a socketand it may return after only
> part of the data is read. Keep going looping until all
> the data is received when --remote option is on.
>
> Signed-off-by: Omri Kramer <omri.kramer@gmail.com>
> Signed-off-by: Lior Fisch <fischlior@gmail.com>
> ---
>  criu/bfd.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/criu/bfd.c b/criu/bfd.c
> index 8269ab1..7c54d9b 100644
> --- a/criu/bfd.c
> +++ b/criu/bfd.c
> @@ -15,6 +15,7 @@
>  #include "util.h"
>  #include "xmalloc.h"
>  #include "page.h"
> +#include "cr_options.h"
>
>  #undef LOG_PREFIX
>  #define LOG_PREFIX "bfd: "
> @@ -298,9 +299,29 @@ int bread(struct bfd *bfd, void *buf, int size)
>  {
>         struct xbuf *b = &bfd->b;
>         int more = 1, filled = 0;
> +       int ret;
> +       size_t curr = 0;
>
> -       if (!bfd_buffered(bfd))
> -               return read(bfd->fd, buf, size);
> +       if (!bfd_buffered(bfd)) {
> +               if(!opts.remote)
> +                       return read(bfd->fd, buf, size);
> +               /*
> +                * Required to keep reading,
> +                * for succesfully Reading all the data
> +                * from the socket, when on remote option.
> +                */
> +               while (1) {
> +                       ret = read(bfd->fd, buf + curr, size - curr);
> +                       if (ret < 0) {
> +                               pr_perror("Can't read from buffer\n");
> +                               return -1;
> +                       }
> +
> +                       curr += ret;
> +                       if (curr == size || ret == 0)
> +                               return curr;
> +               }
> +       }
>
>         while (more > 0) {
>                 int chunk;
> --
> 2.7.4
>
>
Mike Rapoport July 10, 2017, 1:43 p.m.
On Mon, Jul 10, 2017 at 01:15:09PM +0100, Rodrigo Bruno wrote:
> Hi,
> 
> isn't it a good idea to extract the read loop into a separate function (I
> thought we already had one in the past...)?
> 
> I mean, the read syscall is not guaranteed to actually read the number
> requested bytes (not even if we are reading from a local file).

> Besides, if I remember correctly, there are multiple code locations that
> could use that new function.

I think it's the second, the first was in page-read
 
> cheers,
> rodrigo
> 
> 2017-07-10 12:56 GMT+01:00 Omri Kramer <omri.kramer@gmail.com>:
> 
> > When the --remote option is used, the file descriptor
> > used in bread is a socketand it may return after only
> > part of the data is read. Keep going looping until all
> > the data is received when --remote option is on.
> >
> > Signed-off-by: Omri Kramer <omri.kramer@gmail.com>
> > Signed-off-by: Lior Fisch <fischlior@gmail.com>
> > ---
> >  criu/bfd.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/criu/bfd.c b/criu/bfd.c
> > index 8269ab1..7c54d9b 100644
> > --- a/criu/bfd.c
> > +++ b/criu/bfd.c
> > @@ -15,6 +15,7 @@
> >  #include "util.h"
> >  #include "xmalloc.h"
> >  #include "page.h"
> > +#include "cr_options.h"
> >
> >  #undef LOG_PREFIX
> >  #define LOG_PREFIX "bfd: "
> > @@ -298,9 +299,29 @@ int bread(struct bfd *bfd, void *buf, int size)
> >  {
> >         struct xbuf *b = &bfd->b;
> >         int more = 1, filled = 0;
> > +       int ret;
> > +       size_t curr = 0;
> >
> > -       if (!bfd_buffered(bfd))
> > -               return read(bfd->fd, buf, size);
> > +       if (!bfd_buffered(bfd)) {
> > +               if(!opts.remote)
> > +                       return read(bfd->fd, buf, size);
> > +               /*
> > +                * Required to keep reading,
> > +                * for succesfully Reading all the data
> > +                * from the socket, when on remote option.
> > +                */
> > +               while (1) {
> > +                       ret = read(bfd->fd, buf + curr, size - curr);
> > +                       if (ret < 0) {
> > +                               pr_perror("Can't read from buffer\n");
> > +                               return -1;
> > +                       }
> > +
> > +                       curr += ret;
> > +                       if (curr == size || ret == 0)
> > +                               return curr;
> > +               }
> > +       }

I've looked at the code once more and it seems that with opts.remote we can
just fall through to chunk reading mode.
Pavel, Andrey, is there anything that prevents it?

> >         while (more > 0) {
> >                 int chunk;
> > --
> > 2.7.4
> >
> >