From: Dan Smith <danms@us•ibm.com>
To: "Serge E. Hallyn" <serue@us•ibm.com>
Cc: containers@lists•osdl.org, netdev@vger•kernel.org
Subject: Re: [PATCH 2/4] C/R: Basic support for network namespaces and devices (v3)
Date: Wed, 10 Feb 2010 11:30:54 -0800 [thread overview]
Message-ID: <87hbpohor5.fsf@caffeine.danplanet.com> (raw)
In-Reply-To: <20100210192019.GA18879@us.ibm.com> (Serge E. Hallyn's message of "Wed\, 10 Feb 2010 13\:20\:19 -0600")
SH> rw_lockt is effectively a spinlock, so I don't think you can sleep
SH> here.
Yep, thanks.
>> + for_each_netdev(net, dev) {
>> + if (!dev->netdev_ops->ndo_checkpoint)
>> + continue;
SH> Won't the checkpoint_obj() call checkpoint_netdev(), which will return
SH> -EINVAL if ndo_checkpoint is not defined?
Yes, but this isn't the only place that checkpoint_netdev() could be
called (dev->peer in the veth example) so I figured that it would be
best to test it there too before I blindly call a NULL function
pointer. It should never happen, but seemed prudent.
SH> But here you skip the checkpoint_obj() call (which seems wrong to
SH> me). Which do you want to have happen?
What the code is doing is "skipping any interfaces in a netns that
don't have a checkpoint operation" but would fail if you called
checkpoint_obj() on a veth peer that happened to be missing that
operation for some reason.
I suppose you could argue that we should fail in the netns case
instead, which will make this a bit messier for things we get for
"free" in a new netns, like sit0. If preferable, I can just add an
ndo_checkpoint() to sit0 as well and simply checkpoint the presence of
it until later when we decide if we care about it.
SH> By hard-coding veth stuff into generic-sounding functions in
SH> net/checkpoint_dev.c you seem to be assuming that only veth will
SH> ever be supported for checkpoint/restart? what about macvlan?
SH> (Not to mention that eventually we intend to support moving
SH> physical nics into containers)
No, that's not what I'm assuming. The only interface type I need to
control with RTNL is veth right now. So, if you'd prefer a
single-case of:
if (type == veth)
do_veth_message();
else
fail();
to record the goal of having more types later I'll happily add that
unreachable code to the patch :)
--
Dan Smith
IBM Linux Technology Center
email: danms@us•ibm.com
next prev parent reply other threads:[~2010-02-10 19:30 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-02-09 21:25 Network device and namespace checkpoint/restart (v2) Dan Smith
2010-02-09 21:25 ` [PATCH 1/4] Add checkpoint and collect hooks to net_device_ops Dan Smith
2010-02-09 21:25 ` [PATCH 2/4] C/R: Basic support for network namespaces and devices (v3) Dan Smith
[not found] ` <1265750713-15749-3-git-send-email-danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
2010-02-10 17:24 ` Serge E. Hallyn
2010-02-10 17:38 ` Dan Smith
[not found] ` <87pr4dgfdz.fsf-FLMGYpZoEPULwtHQx/6qkW3U47Q5hpJU@public.gmane.org>
2010-02-10 20:01 ` Oren Laadan
2010-02-10 20:30 ` Serge E. Hallyn
2010-02-10 17:55 ` Dan Smith
2010-02-10 19:20 ` Serge E. Hallyn
2010-02-10 19:30 ` Dan Smith [this message]
2010-02-10 20:25 ` Serge E. Hallyn
2010-02-10 20:31 ` Dan Smith
2010-02-10 20:34 ` Serge E. Hallyn
[not found] ` <87ljf1gemh.fsf-FLMGYpZoEPULwtHQx/6qkW3U47Q5hpJU@public.gmane.org>
2010-02-11 11:02 ` Louis Rilling
2010-02-11 15:59 ` Dan Smith
2010-02-11 17:20 ` Oren Laadan
2010-02-11 17:51 ` Oren Laadan
2010-02-09 21:25 ` [PATCH 3/4] Add checkpoint support for veth devices Dan Smith
2010-02-10 17:57 ` Serge E. Hallyn
2010-02-09 21:25 ` [PATCH 4/4] Add loopback checkpoint support Dan Smith
2010-02-11 17:26 ` Network device and namespace checkpoint/restart (v2) Oren Laadan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87hbpohor5.fsf@caffeine.danplanet.com \
--to=danms@us$(echo .)ibm.com \
--cc=containers@lists$(echo .)osdl.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=serue@us$(echo .)ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox