archive mirror
 help / color / mirror / Atom feed
From: Kent Overstreet <>
To: Rusty Russell <>
Cc: "Michael S. Tsirkin" <>,,
Subject: Re: [PATCH] virtio-blk: Fix kconfig option
Date: Thu, 6 Sep 2012 17:25:33 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Fri, Sep 07, 2012 at 09:10:25AM +0930, Rusty Russell wrote:
> Kent Overstreet <> writes:
> > On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> >> On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> >> > Do you not understand the difference between depends an selects?
> >> > Or did you not read my original mail?
> Now you're getting insulting.

Yes, but at least I'm not being intentionally obtuse.

> It's normal for options to depend on other options.  Sometimes they're
> directly nested (eg. E1000 depends on NETDEVICES, and it's nested under
> that option), sometimes they're not (eg. E1000 depends on PCI, which is
> selected elsewhere).
> The fact that you are only just realizing this is not Michael's problem.

Like I said, I'm well aware of that. The issue here isn't the
dependency, it's that it depends on something that isn't exposed

Think about it from the user's pov. They check what VIRTIO_BLK depends
on - just VIRTIO.

So they try to figure out how to flip on VIRTIO, or what VIRTIO even is.

See how that last step might be problematic? CONFIG_VIRTIO is not
exposed! It doesn't even seem to control anything!

Go back to your example. Checking the dependencies for E1000 would tell
you the user needs to flip on CONFIG_PCI. Done. Easy.

User checks the dependencies here and... what do _you_ expect people to

Look, depending on a kconfig option that's supposed to be user
controllable but isn't exposed anywhere is flat out broken. The fact
that it's in a different submenu just makes it worse.

The problem is that VIRTIO_BLK's dependencies are not actually specified
in the kconfig. If it depends on VIRTIO_PCI, that's what the kconfig
should say. If it depends on having any of multiple virtio backends
enabled, then specify that!


Or if you really want to have a fake config option that's enabled if you
have any virtio backend enabled, fix the damn comments and naming!

How is anyone supposed to know that CONFIG_VIRTIO really means "any
virtio backend?" Call it VIRTIO_ANY_BACKEND if that's what it really is.

And, if that is what you're doing with CONFIG_VIRTIO (I'm still not
sure) the comment at the top of drivers/virtio/Kconfig is _wrong_:

# Virtio always gets selected by whoever wants it.

How is _anyone_ supposed to know that really means "VIRTIO gets selected
by things that provide a virtio backend?"

C'mon, you've had to debug other people's code before. What would _you_
think if you were tripped up by something like that?

> >> > Flip off everything in drivers -> virtio
> >> > 
> >> > Now go to drivers -> block and try to turn on virtio-blk.
> >> > 
> >> > It's not listed!
> >> 
> >> Yes. Because you disabled all virtio backends.
> >> It does not make sense to have any frontends.
> >
> > How's a user - or even another kernel developer who isn't familiar with
> > virtio - supposed to know that?
> I get annoyed that menuconfig doesn't show options whose dependencies
> aren't possible, too.  (I got bitten the other way: it doesn't show
> dependencies which can't be disabled, and I was trying to turn KALLSYMS
> off).
> But as I found out just last week, the '/' key allows you to find any
> option, and shows what dependencies it has, and their values.

Yep, use it all the time. 

  reply	other threads:[~2012-09-07  0:25 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-03  4:41 [PATCH] virtio-blk: Fix kconfig option Kent Overstreet
2012-09-03  4:46 ` Kent Overstreet
2012-09-04  6:23 ` Rusty Russell
2012-09-04 23:12   ` Kent Overstreet
2012-09-05  4:22   ` Asias He
2012-09-05  5:48     ` Michael S. Tsirkin
2012-09-05  5:54       ` Asias He
2012-09-05  6:04         ` Michael S. Tsirkin
2012-09-05  6:12   ` Michael S. Tsirkin
2012-09-06  1:46     ` Rusty Russell
2012-09-06  7:41   ` Kent Overstreet
2012-09-06  8:44     ` Michael S. Tsirkin
2012-09-06  9:25       ` Kent Overstreet
2012-09-06  9:49         ` Michael S. Tsirkin
2012-09-06 10:02           ` Kent Overstreet
2012-09-06 10:18             ` Michael S. Tsirkin
2012-09-06 10:31               ` Kent Overstreet
2012-09-06 11:09                 ` Michael S. Tsirkin
2012-09-06 23:40             ` Rusty Russell
2012-09-07  0:25               ` Kent Overstreet [this message]
2012-09-07  2:57                 ` Rusty Russell

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* 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;
as well as URLs for NNTP newsgroup(s).