[CRIU] Re: [PATCH 10/10] files: Use sys_kcmp to find file descriptor duplicates

[Pavel Emelyanov]

On 02/27/2012 07:21 PM, Cyrill Gorcunov wrote:
> On Mon, Feb 27, 2012 at 06:01:41PM +0400, Cyrill Gorcunov wrote:
>>
>> Yes, 0 will work as well but I would prefer to keep some predefined
>> value other than 0 which actually gives us a good hint in debugging
>> purpose.
>>
> 
> Does this one look better?

Almost perfect. See comments inline.

> @@ -120,10 +124,21 @@ static int dump_one_reg_file(int type, struct fd_parms *p, int lfd,
>         e.flags = p->flags;
>         e.pos   = p->pos;
>         e.addr  = p->fd_name;
> -       if (p->id)
> -               memcpy(e.id, p->id, FD_ID_SIZE);
> -       else
> -               memzero(e.id, FD_ID_SIZE);
> +
> +       if (likely(!fd_is_special(&e))) {
> +               struct fd_id_entry *fd_id_entry;
> +
> +               fd_id_entry = fd_id_entry_collect((u32)p->id, p->pid, p->fd_name);
> +               if (!fd_id_entry) {
> +                       pr_err("No memory to collect fidinfo for file %li (pid %d)\n",
> +                               p->fd_name, p->pid);

Useless pr_err -- the fd_id_entry_collect emits one itself.

> +       e = xmalloc(sizeof(*e));
> +       if (!e) {
> +               pr_err("No memory to allocate fd_id_entry: "
> +                      "genid %8x pid %6d fd %4d\n",
> +                       genid, pid, fd);

xmalloc prints the message himself as well.

> +               goto err;
> +       }
> +
> +       e->u.key.subid  = -2U; /* Just for easier parsing on dumps */

For symmetry this should be fd_id_entries_subid++;

> +       e->u.key.genid  = genid;
> +       e->pid          = pid;
> +       e->fd           = fd;
> +
> +       rb_init_node(&e->node);
> +       rb_init_node(&e->subtree_node);
> +       rb_attach_node(&e->subtree_root, &e->subtree_node);
> +err:
> +       return e;
> +}
> +
> +static struct fd_id_entry *
> +lookup_alloc_subtree(struct fd_id_entry *e, u32 genid, pid_t pid, int fd)
> +{
> +       struct rb_node *node = e->subtree_root.rb_node;
> +       struct fd_id_entry *sub = NULL;
> +
> +       struct rb_node **new = &e->subtree_root.rb_node;
> +       struct rb_node *parent = NULL;
> +
> +       while (node) {
> +               struct fd_id_entry *this = rb_entry(node, struct fd_id_entry, subtree_node);
> +               int ret = sys_kcmp(this->pid, pid, KCMP_FILE, this->fd, fd);
> +
> +               parent = *new;
> +               if (ret < 0)
> +                       node = node->rb_left, new = &((*new)->rb_left);
> +               else if (ret > 0)
> +                       node = node->rb_right, new = &((*new)->rb_right);
> +               else
> +                       return this;
> +       }
> +
> +       sub = alloc_fd_id_entry(genid, pid, fd);
> +       if (!sub)
> +               goto err;
> +
> +       sub->u.key.subid = fd_id_entries_subid++;
> +
> +       rb_link_and_balance(&e->subtree_root, &sub->subtree_node, parent, new);
> +err:
> +       return sub;
> +}
> +
> +struct fd_id_entry *fd_id_entry_collect(u32 genid, pid_t pid, int fd)
> +{
> +       struct rb_node *node = fd_id_root.rb_node;
> +       struct fd_id_entry *e = NULL;
> +
> +       struct rb_node **new = &fd_id_root.rb_node;
> +       struct rb_node *parent = NULL;
> +
> +       while (node) {
> +               struct fd_id_entry *this = rb_entry(node, struct fd_id_entry, node);
> +
> +               parent = *new;
> +               if (genid < this->u.key.genid)
> +                       node = node->rb_left, new = &((*new)->rb_left);
> +               else if (genid > this->u.key.genid)
> +                       node = node->rb_right, new = &((*new)->rb_right);
> +               else
> +                       return lookup_alloc_subtree(this, genid, pid, fd);
> +       }
> +
> +       e = alloc_fd_id_entry(genid, pid, fd);
> +       if (!e)
> +               goto err;
> +
> +       /*
> +        * Make sure the union is still correlate with structure
> +        * we write to disk.
> +        */
> +       {
> +               struct fdinfo_entry __entry __always_unused;
> +               BUILD_BUG_ON(sizeof(e->u.key) != sizeof(__entry.id));

This build bug on should be where you assign entry's id to image entry's id, not here.

> +       }
> +
> +       rb_link_and_balance(&fd_id_root, &e->node, parent, new);
> +err:
> +       return e;
> +}