[CRIU] [PATCH 6/7] read mode pre-dump implementation

[Dmitry Safonov]

On 8/29/19 5:08 AM, abhishek dubey wrote:
> On 29/08/19 3:14 AM, Dmitry Safonov wrote:
[..]
>> It probably not what you want on arm64, ppc64 where page_size could be
>> different from 4Kb with Large Pages. Furthermore, you use it only in one
>> function as a temporary buffer - could we move it from .data section
>> into dynamic allocation with malloc()?
> 
> is this a correct replacement? architecture independent one?
> 
> char* userbuf = malloc(PIPE_MAX_SIZE * PAGE_SIZE);

I believe so. You may want to use criu's xmalloc() which will also print
an error on fail [not to print it yourself].

[..]
>>> +
>>> +    /* Handling Case 3-Part 3.2*/
>>> +    offset = (partial_read_bytes)? partial_read_bytes : PAGE_SIZE;
>>> +
>>> +    dummy.iov_base = riov[faulty_index].iov_base + offset;
>>> +    dummy.iov_len = riov[faulty_index].iov_len - offset;
>>> +
>>> +    if (!partial_read_bytes)
>>> +        cnt_sub(CNT_PAGES_WRITTEN, 1);
>>> +
>>> +    while (dummy.iov_len) {
>>> +
>>> +        bytes_read = process_vm_readv(pid, bufvec, 1, &dummy, 1, 0);
>> Could you check errno please?
> 
> In the very first patch I had that check. In the review it was not found
> very useful, so omitted that.

Heh, wasn't aware of that.
Correct me if I'm wrong, but you expect that it may fail with EFAULT.
But other errors look to be unexpected here?

At least those three from `man 2 process_vm_readv`:
> ENOMEM Could not allocate memory for internal copies  of  the  iovec
> structures.
> EPERM  The  caller  does  not have permission to access the address
> space ofthe process pid.
> ESRCH  No process with ID pid exists.

> 
> @Andrei @Xemul : how to go about it?
> 
>>
[..]
>>> +        ret = vmsplice(ppb->p[1], &bufvec, 1, SPLICE_F_NONBLOCK);
>>> +
>>> +        if (ret == -1 || ret != bytes_read) {
>>> +            pr_err("vmsplice: Failed to splice user buffer to pipe
>>> %ld\n", ret);
>> Probably you want pr_error()
> 
> Yes, maybe. Other splicing failure also had pr_error().
> 
> What is difference between two?

Sorry, misinformed you - the correct name is pr_perror().

It will print decoding for errno:

: #define pr_perror(fmt, ...)				\
:	pr_err(fmt ": %s\n", ##__VA_ARGS__, strerror(errno))


Thanks,
          Dmitry