mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Serge Hallyn <serge.hallyn@ubuntu.com>
Cc: linux-kernel@vger.kernel.org, adityakali@google.com,
	linux-api@vger.kernel.org, containers@lists.linux-foundation.org,
	cgroups@vger.kernel.org, lxc-devel@lists.linuxcontainers.org,
	akpm@linux-foundation.org, ebiederm@xmission.com,
	gregkh@linuxfoundation.org, lizefan@huawei.com,
	hannes@cmpxchg.org,
	"Serge E. Hallyn" <serge.hallyn@canonical.com>
Subject: Re: [PATCH 1/8] kernfs: Add API to generate relative kernfs path
Date: Wed, 9 Dec 2015 17:36:51 -0500	[thread overview]
Message-ID: <20151209223651.GQ30240@mtj.duckdns.org> (raw)
In-Reply-To: <20151209221327.GA13029@ubuntumail>

Hey,

On Wed, Dec 09, 2015 at 10:13:27PM +0000, Serge Hallyn wrote:
> we can rename kn_root to from here if you think that's clearer (and
> change the order here as well).

I think it'd be better for them to be consistent and in the same order
- the target and then the optional base.

> > Was converting the path functions to return
> > length too much work?  If so, that's fine but please explain what
> > decisions were made.
> 
> Yes, I had replied saying:
> 
>  |I can change that, but the callers right now don't re-try with
>  |larger buffer anyway, so this would actually complicate them just
>  |a smidgeon.  Would you want them changed to do that?  (pr_cont_kernfs_path
>  |right now writes into a static char[] for instance)
> 
> I can still make that change if you like.

Oops, sorry I forgot about that.  The reason why kernfs_path() is
written the current way was me being lazy.  While I think it'd be
better to make the functions behave like normal string handling
functions if we're extending it, I don't think it's that important.
If it's easy, please go ahead.  If not, we can get back to it later
when necessary.

> > I skimmed through the series and spotted several other review points
> > which didn't get addressed.  Can you please go over the previous
> > review cycle and address the review points?
> 
> I did go through every email twice, once while making changes (one
> branch per response) and once while making changelog for each patch,
> sorry about whatever I missed.  I'll go through each again.

The other chunk I noticed was inline conversions of internal functions
which didn't seem to belong to the patch.  I asked whether those were
stray chunks.  Maybe the comment was too buried to notice?  Anyways,
that part actually causes conflicts when applying to cgroup/for-4.5.

There are a couple more things.

* Can you please put the ns related decls after the regular cgroup
  stuff in cgroup.h?

* I think I might need to edit the documentation anyway but it'd be
  great if you can make the namespace section more in line with the
  rest of the documentation - e.g. s/CGroup/cgroup/ and more
  structured sectioning.

At this point, it all generally looks good to me.  Let's get the
nits out of the way and merge it.

Thanks.

-- 
tejun

  reply	other threads:[~2015-12-09 22:36 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-09 19:28 CGroup Namespaces (v7) serge.hallyn
2015-12-09 19:28 ` [PATCH 1/8] kernfs: Add API to generate relative kernfs path serge.hallyn
2015-12-09 21:38   ` Tejun Heo
2015-12-09 22:13     ` Serge Hallyn
2015-12-09 22:36       ` Tejun Heo [this message]
2015-12-09 22:51         ` Serge E. Hallyn
2015-12-10  1:28         ` Serge E. Hallyn
2015-12-09 19:28 ` [PATCH 2/8] sched: new clone flag CLONE_NEWCGROUP for cgroup namespace serge.hallyn
2015-12-09 19:28 ` [PATCH 3/8] cgroup: introduce cgroup namespaces serge.hallyn
2015-12-09 19:28 ` [PATCH 4/8] cgroup: cgroup namespace setns support serge.hallyn
2015-12-09 19:28 ` [PATCH 5/8] kernfs: define kernfs_node_dentry serge.hallyn
2015-12-09 19:28 ` [PATCH 6/8] cgroup: mount cgroupns-root when inside non-init cgroupns serge.hallyn
2015-12-09 19:29 ` [PATCH 7/8] cgroup: Add documentation for cgroup namespaces serge.hallyn
2015-12-09 19:29 ` [PATCH 8/8] Add FS_USERNS_FLAG to cgroup fs serge.hallyn
  -- strict thread matches above, loose matches on Subject: below --
2016-01-29  8:54 CGroup Namespaces (v10) serge.hallyn
2016-01-29  8:54 ` [PATCH 1/8] kernfs: Add API to generate relative kernfs path serge.hallyn
2016-01-04 19:54 CGroup Namespaces (v9) serge.hallyn
2016-01-04 19:54 ` [PATCH 1/8] kernfs: Add API to generate relative kernfs path serge.hallyn
2015-12-23  4:23 CGroup Namespaces (v8) serge.hallyn
2015-12-23  4:23 ` [PATCH 1/8] kernfs: Add API to generate relative kernfs path serge.hallyn
2015-12-23 16:08   ` Tejun Heo
2015-12-23 16:36     ` Serge E. Hallyn
2015-12-23 16:24   ` Tejun Heo
2015-12-23 16:51     ` Greg KH
2015-11-16 19:51 CGroup Namespaces (v4) serge
2015-11-16 19:51 ` [PATCH 1/8] kernfs: Add API to generate relative kernfs path serge
2015-11-24 16:16   ` Tejun Heo
2015-11-24 16:17     ` Tejun Heo
2015-11-24 17:43       ` Serge E. Hallyn
2015-11-27  5:25     ` Serge E. Hallyn
2015-11-30 15:11       ` Tejun Heo
2015-11-30 18:37         ` Serge E. Hallyn
2015-11-30 22:53           ` Tejun Heo
2015-12-01  2:08             ` Serge E. Hallyn

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=20151209223651.GQ30240@mtj.duckdns.org \
    --to=tj@kernel.org \
    --cc=adityakali@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=cgroups@vger.kernel.org \
    --cc=containers@lists.linux-foundation.org \
    --cc=ebiederm@xmission.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hannes@cmpxchg.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lizefan@huawei.com \
    --cc=lxc-devel@lists.linuxcontainers.org \
    --cc=serge.hallyn@canonical.com \
    --cc=serge.hallyn@ubuntu.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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

all inboxes | Powered by JetHome®