* Alexey Dobriyan <adobriyan@xxxxxxxxx> wrote:
> +int cr_restore_file(struct cr_context *ctx, loff_t pos)
> +{
I tried to review this code, but it's almost unreadable to me, due
to basic code structure mistakes like:
> + struct cr_image_file *i, *tmp;
> + struct file *file;
> + struct cr_object *obj;
> + char *cr_name;
> + int rv;
> +
> + i = kzalloc(sizeof(*i), GFP_KERNEL);
> + if (!i)
> + return -ENOMEM;
> + rv = cr_pread(ctx, i, sizeof(*i), pos);
> + if (rv < 0) {
> + kfree(i);
> + return rv;
> + }
> + if (i->cr_hdr.cr_type != CR_OBJ_FILE) {
> + kfree(i);
> + return -EINVAL;
> + }
> + /* Image of struct file is variable-sized. */
> + tmp = i;
> + i = krealloc(i, i->cr_hdr.cr_len + 1, GFP_KERNEL);
> + if (!i) {
> + kfree(tmp);
> + return -ENOMEM;
> + }
> + cr_name = (char *)(i + 1);
> + rv = cr_pread(ctx, cr_name, i->cr_name_len, pos + sizeof(*i));
> + if (rv < 0) {
> + kfree(i);
> + return -ENOMEM;
> + }
> + cr_name[i->cr_name_len] = '\0';
> +
> + file = filp_open(cr_name, i->cr_f_flags, 0);
> + if (IS_ERR(file)) {
> + kfree(i);
> + return PTR_ERR(file);
> + }
> + if (file->f_dentry->d_inode->i_mode != i->cr_i_mode) {
> + fput(file);
> + kfree(i);
> + return -EINVAL;
> + }
> + if (vfs_llseek(file, i->cr_f_pos, SEEK_SET) != i->cr_f_pos) {
> + fput(file);
> + kfree(i);
> + return -EINVAL;
> + }
> +
> + obj = cr_object_create(file);
> + if (!obj) {
> + fput(file);
> + kfree(i);
> + return -ENOMEM;
> + }
This contains 7 kfree()s of the same thing (!), 3 fput()s of the
same thing, replicated all over the place obscuring the real essence
of the code.
This should be restructured to move all the failure exception cases
into a clean out of line inverse teardown sequence with proper goto
labels. That way it will be 70% real code 30% teardown - not 10%
real code mixed into 90% teardown like above.
Also, whoever named a local variable with a type of
"struct cr_image_file *" as 'i' should be sent back to
coding primary school.
You really should not write new kernel code until you know, follow
and respect basic code cleanliness principles. I am not inserting
any more review feedback value into this code until it does not meet
_basic_ quality standards that make review efforts smooth and
efficient.
Oh, and then i saw this sequence:
> + /* Known good and unknown bad flags. */
> + vm_flags &= ~VM_READ;
> + vm_flags &= ~VM_WRITE;
> + vm_flags &= ~VM_EXEC;
> +// vm_flags &= ~VM_SHARED;
> + vm_flags &= ~VM_MAYREAD;
> + vm_flags &= ~VM_MAYWRITE;
> + vm_flags &= ~VM_MAYEXEC;
> +// vm_flags &= ~VM_MAYSHARE;
> + vm_flags &= ~VM_GROWSDOWN;
> +// vm_flags &= ~VM_GROWSUP;
> +// vm_flags &= ~VM_PFNMAP;
> + vm_flags &= ~VM_DENYWRITE;
> + vm_flags &= ~VM_EXECUTABLE;
> +// vm_flags &= ~VM_LOCKED;
> +// vm_flags &= ~VM_IO;
> +// vm_flags &= ~VM_SEQ_READ;
> +// vm_flags &= ~VM_RAND_READ;
> +// vm_flags &= ~VM_DONTCOPY;
> + vm_flags &= ~VM_DONTEXPAND;
> +// vm_flags &= ~VM_RESERVED;
> + vm_flags &= ~VM_ACCOUNT;
> +// vm_flags &= ~VM_NORESERVE;
> +// vm_flags &= ~VM_HUGETLB;
> +// vm_flags &= ~VM_NONLINEAR;
> +// vm_flags &= ~VM_MAPPED_COPY;
> +// vm_flags &= ~VM_INSERTPAGE;
> + vm_flags &= ~VM_ALWAYSDUMP;
> + vm_flags &= ~VM_CAN_NONLINEAR;
> +// vm_flags &= ~VM_MIXEDMAP;
> +// vm_flags &= ~VM_SAO;
> +// vm_flags &= ~VM_PFN_AT_MMAP;
No comment ...
Ingo
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
