Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Jens Axboe <axboe@kernel.dk>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>
Subject: Re: [GIT PULL] Block fixes for 5.2-rc4
Date: Sat, 8 Jun 2019 12:28:38 -0700
Message-ID: <CAHk-=whXfPjCtc5+471x83WApJxvxzvSfdzj_9hrdkj-iamA=g@mail.gmail.com> (raw)
In-Reply-To: <4f801c9b-4ab6-9a11-536c-ff509df8aa56@kernel.dk>

On Sat, Jun 8, 2019 at 1:21 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> Angelo Ruocco (2):
>       cgroup: let a symlink too be created with a cftype file

So I'm not seeing any acks by the cgroup people who actually maintain
that file, and honestly, the patch looks butt-ugly to me.

Why are you adding an odd "write_link_name" boolean argument to
cgroup_file_name() that is really hard to explain?

When you see this line of code, what does that "false" tell you?

        return cgroup_fill_name(cgrp, cft, buf, false);

Does that look legible to you?

It looks to me like it would have been much easier and straightforward
- and legible - to just pass in the name itself, and make
cgroup_file_name() do

        return cgroup_fill_name(cgrp, cft, buf, cft->name);

instead, and now the code kind of explains itself, in ways that
"false" does not. (And cgroup_link_name() would obviously just pass in
"cft->link_name").

That would have simplified the code, and I think would have made the
call be a lot more obvious than passing in a random "true/false"
parameter that makes no conceptual sense and just looks odd in that
context.

Maybe there's something I'm missing and there's some advantage to the
incomprehensible bool argument?

I've pulled this, but seriously - when you change files that aren't
maintained by you, you should get their approval.

And if this had been a completely trivial one-liner, I wouldn't care,
but when the change looks _ugly_, I really want that ack.

We've had enough block layer problems over the years that it's not ok
to start just randomly changing other sub-areas without even notifying
people.

                 Linus

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-08  8:20 Jens Axboe
2019-06-08 19:28 ` Linus Torvalds [this message]
2019-06-09  5:59   ` Jens Axboe
2019-06-09 16:06     ` Linus Torvalds
2019-06-10 10:15       ` Jens Axboe
2019-06-10 14:49         ` Paolo Valente
2019-06-10 15:48           ` Tejun Heo
2019-06-11 19:49             ` [PATCH block/for-5.2-fixes] bfq: use io.weight interface file instead of io.bfq.weight Tejun Heo
2019-06-11 21:17               ` Tejun Heo
2019-06-12  7:32                 ` Paolo Valente
2019-06-12 13:39                   ` Tejun Heo
2019-06-13  6:10                     ` Paolo Valente
2019-06-14 20:22                       ` Tejun Heo
2019-06-14 21:05                         ` Paolo Valente
2019-08-20  6:58                           ` Paolo Valente
2019-06-08 19:30 ` [GIT PULL] Block fixes for 5.2-rc4 pr-tracker-bot

Reply instructions:

You may reply publically 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='CAHk-=whXfPjCtc5+471x83WApJxvxzvSfdzj_9hrdkj-iamA=g@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=axboe@kernel.dk \
    --cc=linux-block@vger.kernel.org \
    /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

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org linux-block@archiver.kernel.org
	public-inbox-index linux-block


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/ public-inbox