[Devel] Re: [PATCH 2/2] c/r: define s390-specific checkpoint-restart code (v5)

[Serge E. Hallyn]

Quoting Dave Hansen (dave at linux.vnet.ibm.com):
> On Tue, 2009-02-24 at 13:37 -0600, Serge E. Hallyn wrote:
> > 
> > > +/* dump the cpu state and registers of a given task */
> > > +int cr_write_cpu(struct cr_ctx *ctx, struct task_struct *t)
> > > +{
> > > +     struct cr_hdr h;
> > > +     struct cr_hdr_cpu *hh = cr_hbuf_get(ctx, sizeof(*hh));
> > > +     int ret;
> > > +
> > > +     h.type = CR_HDR_CPU;
> > > +     h.len = sizeof(*hh);
> > > +     h.parent = task_pid_vnr(t);
> > 
> > BTW - I know Dave mentioened using a generic helper for this
> > often-used stanza above, but I continue to be against that
> > bc the helper ends up having to take a bunch of eye-numbing
> > arguments and I think the code ends up hard to read.  But
> > maybe you can think of a way to make that clearer...
> 
> Yeah, that's true.  The plethora of types also makes it hard to do with
> a function.  For plain readability you can't beat what we already have
> there.
> 
> But, I do still wonder why we need the .parent member in the cr_hdr
> itself.  Shouldn't that be something that gets pushed down to where we
> can actually describe it, like in the cr_hdr_foo structures?

Hmm, yeah, moving it would have two upsides:  make the common
stanza a line shorter, and let us use more descriptive names
for the variables.  parent is really not helpful unless you've
spent years with the code...

OTOH I'm not eager to make such a change right now only to find
months later that there was a good reason to keep it in the hdr
after all  :)

-serge
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers