All of lore.kernel.org
 help / color / mirror / Atom feed
From: Avi Kivity <avi@redhat.com>
To: Anthony Liguori <anthony@codemonkey.ws>
Cc: qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>,
	Bruce Rogers <brogers@novell.com>
Subject: Re: [Qemu-devel] [PATCH] move 'unsafe' to end of caching modes in help
Date: Mon, 26 Jul 2010 22:19:56 +0300	[thread overview]
Message-ID: <4C4DDFDC.3000608@redhat.com> (raw)
In-Reply-To: <4C4DDB25.90000@codemonkey.ws>

  On 07/26/2010 09:59 PM, Anthony Liguori wrote:
>>> If libvirt is going to parse -help output, they should do a better 
>>> job at it.  I can't expect QEMU developers to have detailed 
>>> knowledge of how libvirt parses the help output to ensure that we 
>>> don't break their code.
>>
>> Correct.  libvirt could have done much better parsing.  qemu 
>> developers are not familiar with libvirt code.  But is there a 
>> problem in accepting the patch that rearranges the output?
>
>
> What if another tool is parsing -help output? 

Then we have an even bigger problem.  As for now, we have a manageable 
problem that this patch solves.  Let's apply this workaround to fix the 
one real problem we know about, and work towards documented capability 
reporting to make sure this doesn't hit us in the future when we have 
more users.

> Is what we are supporting just what libvirt expects there to be or 
> what any tool out there expects there to be?

We should try to support all users, prioritized by the number of end 
users they represent.  If this patch broke some other large user we'd be 
in a bind.  But likely this isn't the case so we aren't.

>
>>   As far as I can tell, it's just as good for a user, and better for 
>> libvirt, so there are no drawbacks to accepting the patch?
>
> It's not.  Our help output is unreadable.  The (artificial) 
> restrictions we're putting ourselves with respect to the help output 
> prevents it from being improved.

Are you saying


     "       [,cache=writethrough|writeback|unsafe|none][,format=f]\n"

is more readable than

     "       [,cache=writethrough|writeback|none|unsafe][,format=f]\n"

?  if so I disagree.  If you say our help output is in general 
unreadable, then this has nothing to do with this patch.  Implementing a 
capabilities system is more important than improving help output 
readability, let's to this first and then rewrite the help output in 
Shakespearean glory.

>>>
>>> Version is entirely reliable for detecting whether -drive supports 
>>> cache.
>>
>> It's not a reliable interface for detecting features in the face of 
>> backports.
>
> Backports are such a special case.  Honestly, we're talking about RHEL 
> and it's trivially easy for libvirt to just special case RHEL.

As it happens the patch came from a Novell employee, so it isn't about 
RHEL.  I applaud your choice of enterprise operating systems, however.

Regardless, outside of Windows users qemu will mostly be consumed via 
distribution branches, with different levels of backport happiness.  We 
should recognize that and work with it, not against it.

>>
>> I don't see what we gain by not doing this.
>
> We're losing the ability to make *any* change to our help system by 
> encouraging it to be used in this fashion.

If we could point libvirt towards a better way of doing things (not a 
better regexp, which could just break under different circumstances; a 
supported interface) I'd agree.  Go RTFM chapter-this-or-that and don't 
bother me no more.  But we can't.

>> If you want libvirt to do the right thing, provide a proper 
>> capabilities interface.  Using the version has its downsides as much 
>> as the help text.
>
> That's simply not the case.  Please, provide an actual example where 
> version is not reliable and backports aren't trivially easy to detect.


t=0 starting point, cache=unsafe is unknown
t=1 qemu upstream adds cache=unknown
t=2 libvirt adds support for cache=unsafe, releases
t=3 evil distro backports cache=unsafe, releases qemu-kvm-1.2.3.4
t=4 user tries libvirt from t=2 with qemu from t=3, cache=unsafe not 
detected

version numbers force a libvirt update every time a feature is backported.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

  reply	other threads:[~2010-07-26 19:20 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-07-21 20:32 [Qemu-devel] [PATCH] move 'unsafe' to end of caching modes in help Bruce Rogers
2010-07-21 20:55 ` Anthony Liguori
2010-07-21 21:32   ` Daniel P. Berrange
2010-07-21 21:45     ` Anthony Liguori
2010-07-21 21:58       ` Daniel P. Berrange
2010-07-21 23:39         ` Anthony Liguori
2010-07-22  0:45           ` Jamie Lokier
2010-07-22  6:53           ` Markus Armbruster
2010-07-22  8:42           ` Daniel P. Berrange
2010-07-22 14:19             ` Anthony Liguori
2010-07-23  8:00               ` Markus Armbruster
2010-07-26 15:53                 ` Anthony Liguori
2010-07-26 16:26                   ` Avi Kivity
2010-07-26 16:29                     ` Avi Kivity
2010-07-26 19:03                       ` Anthony Liguori
2010-07-26 19:24                         ` Avi Kivity
2010-07-26 19:43                           ` [Qemu-devel] " Juan Quintela
2010-07-26 16:40                     ` [Qemu-devel] " Anthony Liguori
2010-07-26 16:54                       ` Avi Kivity
2010-07-26 18:59                         ` Anthony Liguori
2010-07-26 19:19                           ` Avi Kivity [this message]
2010-07-26 19:35                             ` Anthony Liguori
2010-07-26 19:43                               ` Avi Kivity
2010-07-26 20:03                                 ` Anthony Liguori
2010-07-27  8:11                               ` Markus Armbruster
2010-07-27  9:47                                 ` Jes Sorensen
2010-07-27 12:30                                   ` Cole Robinson
2010-07-27 12:50                                     ` Anthony Liguori
2010-07-27 13:35                                       ` Cole Robinson
2010-07-27  8:26                               ` Markus Armbruster
2010-08-03  9:43                                 ` Richard W.M. Jones
2010-07-27  8:56                   ` Kevin Wolf
2010-07-22 13:45   ` Luiz Capitulino
2010-07-27 10:26 ` Daniel P. Berrange
2010-07-27 13:11   ` Bruce Rogers

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=4C4DDFDC.3000608@redhat.com \
    --to=avi@redhat.com \
    --cc=anthony@codemonkey.ws \
    --cc=armbru@redhat.com \
    --cc=brogers@novell.com \
    --cc=qemu-devel@nongnu.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.