Thanks for the review ...
Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl@xxxxxxxxxxxxxxx):
>> We now handle anonymous and file-mapped shared memory. Support for IPC
>> shared memory requires support for IPC first. We extend cr_write_vma()
>> to detect shared memory VMAs and handle it separately than private
>> memory.
[...]
>> + switch (vma_type) {
>> + case CR_VMA_SHM_FILE:
>> + /* no need for contents that are stored in the file system */
>> + ret = vfs_fsync(vma->vm_file, vma->vm_file->f_path.dentry, 0);
>> + break;
>> + case CR_VMA_SHM_ANON:
>> + /* save the contents of this resgion */
>> + inode = vma->vm_file->f_dentry->d_inode;
>> + ret = cr_write_shmem_contents(ctx, inode);
>> + break;
>> + case CR_VMA_SHM_ANON_SKIP:
>> + case CR_VMA_SHM_FILE_SKIP:
>> + /* already saved before .. skip now */
>> + break;
>> + default:
>> + BUG();
>
> Well, no - since the user can feed in whatever crap they want,
> this isn't a *bug*, right?
... this is during checkpoint, no user input; it makes sure we don't
add a new type of VMA that we don't handle. On restart we complain
with -EINVAL.
[...]
>
>> struct cr_hdr_vma {
>> __u32 vma_type;
>> - __u32 vma_objref; /* for vma->vm_file */
>> + __s32 vma_objref; /* objref of backing file */
>> + __s32 shm_objref; /* objref of shared segment */
>
> You're going to upset Alexey again with the signeds, aren't you?
Yes, I put a comment about signed values somewhere. I cleaned up most of
the unsigned instances following Alexey's comment, but I think in some
cases it makes sense.
In particular, assume I take a pid, or an objref, which is an 'int' in
the code, and save it with __u32. During restart I need to test for a
valid value before blindly converting back to (signed) int, like:
ret = -EINVAL;
if (hh->pid > INT_MAX)
goto out;
in that case, I can just as well leave it signed and test
ret = -EINVAL;
if (hh->pid < 0)
goto out;
which is much more readable, and less error-prone if sometime later
we change objref type from (signed) int to (signed) long and forget
to change INT_MAX to LONG_MAX everywhere ...
Oren.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
