[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 00/18] Add qemu-storage-daemon
Re: [RFC PATCH 00/18] Add qemu-storage-daemon
Thu, 7 Nov 2019 13:03:15 +0100
Am 07.11.2019 um 11:33 hat Daniel P. Berrangé geschrieben:
> On Thu, Oct 17, 2019 at 03:01:46PM +0200, Kevin Wolf wrote:
> > 2. I'm not completely sure if the command line syntax is the final
> > version that we want to support long-term. Many options directly use
> > QAPI visitors (--blockdev, --export, --nbd-server) and should be
> > fine. However, others use QemuOpts (--chardev, --monitor, --object).
> > This is the same as in the system emulator, so we wouldn't be adding
> > a new problem, but as there was talk about QAPIfying the command
> > line, and I wouldn't want later syntax changes or adding lots of
> > compatibility code to a new tool, I thought we should probably
> > discuss whether QAPIfying from the start would be an option.
> I think that following what the QEMU emulators currently do for
> CLI args should be an explicit anti-goal, because we know that it is
> a long standing source of pain. Fixing it in the emulator binaries
> is hard due to backward compatibility, but for this new binary we
> have a clean slate.
> This feels like a good opportunity to implement & demonstrate what
> we think QEMU configuration ought to look like. Work done for this
> in the qemu-storage-daemon may well help us understand how we'll
> be able to move the QEMU emulators into a new scheme later.
It might be, which is why I'm asking. Now that the storage daemon has
missed 4.2, we have a little more time to decide what the command line
should look like in detail.
However, I don't think this is something that should delay the storage
daemon until after 5.0.
> My personal wish would be to have no use of QemuOpts at all.
> Use GOptionContext *only* for parsing command line arguments
> related to execution of the daemon - ie things like --help,
> --version, --daemon, --pid-file.
I really don't believe that the solution for having too much variety in
option parsing is adding in yet another type. GOptionContext is not
something I'm considering at the moment.
But it's a getopt() replacement, not something that could actually parse
the more complex options, so it's a separate question anyway. If we ever
want to use it, we can replace getopt() in all binaries at once.
> The use a "--config /path/to/json/file" arg to point to the config
> file for everything else using QAPI schema to describe it fully.
If this implies that the storage daemon can only do useful things if you
specify a config file, I disagree.
I agree that it's not really nice if you can't use a config file to
specify a lengthy configuration and that supporting one would be good.
But it is at least equally unfriendly to require a config file for
simple configurations where using command line arguments is easily
> When loading the config file, things should be created in order
> in which they are specified. ie don't try and group things,
> otherwise we end up back with the horrific hacks for objects
> where some are created early & some late.
Yes. This is how the storage daemon command line works, too.
I think Markus already had some patches for command line QAPIfication
that were incomplete at least for the system emulator. It might be
easier to make it feature complete for the storage daemon because it
supports much less problematic options. Maybe he can post a useful
subset (if it's too much work to clean up the full thing right now) and
we can work from there.
The one that I expect to be a bit tricky to be QAPIfied is --object.
> For an ambitious stretch goal, I think we should seriously consider
> whether our use of chardevs is appropriate in all cases that exist,
> and whether we can avoid the trap of over-using chardev in the new
> CLI config.
> chardevs are designed for & reasonably well suited to attaching to
> devices like serial ports, parallel ports, etc. You have a 1:1
> remote:local peer relationship. The transport is a dumb byte
> stream, nothing special needed on top & the user can cope with
> any type of chardev.
> Many cases where we've used chardevs as a backend in QEMU are a
> poor fit. We just used chardevs as an "easy" way to configure a
> UNIX or TCP socket from the CLI, and don't care about, nor work
> with, any othuer chardev backends. As a result of this misuse
> we've had to put in an increasing number of hacks in the chardev
> code to deal with fact that callers want to know about & use
> socket semantics. eg FD passing, the chardev reconnection polling
> The monitor is a prime example of a bad fit - it would be better
> suited by simply referencing a SocketAddress QAPI type, instead
> of having the chardev indirection. It would then directly use
> the QIOChannelSocket APIs and avoid the inconvenient chardev
> abstractions which are a source of complexity & instability for
> no net gain. vhostuser is another prime example, responsible
> for much of the complexity & bugs recently added to chardevs
> to expose socket semantics
> This is a long winded way of saying that we should consider what
> syntax we expose for the monitor socket configuration with the
> new daemon. Even if the internal code still uses a chardev for
> the forseeable future, we have the option to hide this from the
> user facing configuration. Let the user specify a SocketAddress,
> which we use to secretly instantiate a chardev. Eventually we can
> convert the monitor code to stop using a chardev internally too,
> with a suitable deprecation period for main QEMU binarijes.
The monitor is actually the prime example for chardev backends like
stdio or vc. I'm using them all the time and they aren't covered by
SocketAddress, so I'm afraid SocketAddress alone is not an option.
In any case, the goal of the storage daemon code is to be as boring as
possible. It should just be a thin wrapper around existing code that
glues the command line, monitor and various backends together.
So any change to the usage of chardevs (that is more than a few lines of
wrapper code) would probably have to be made in the system emulator
first, otherwise we'd end up duplicating a lot of code for the storage
daemon instead of just reusing it. I guess using SocketAddress for the
storage daemon and internall creating a chardev-socket would be possible
with a few lines if that covered stdio - but it doesn't.