[libvirt] Fwd: [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi

[Nicholas A. Bellinger]

On Wed, 2014-07-02 at 10:27 +0100, Daniel P. Berrange wrote:
> On Tue, Jul 01, 2014 at 06:51:46PM -0700, Nicholas A. Bellinger wrote:
> > Hi Mike & Co,
> > 
> > > ---------- Forwarded message ----------
> > > From: Mike Perez <thingee at gmail.com>
> > > Date: Fri, Jun 27, 2014 at 7:39 PM
> > > Subject: Re: [libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to
> > > virtio-scsi
> > > To: Ján Tomko <jtomko at redhat.com>
> > > Cc: Eric Blake <eblake at redhat.com>, libvir-list at redhat.com,
> > > berrange at redhat.com, "Nab at daterainc.com" <Nab at daterainc.com>
> > > 
> > > 
> > > On Thu, Jun 5, 2014 at 6:57 AM, Ján Tomko <jtomko at redhat.com> wrote:
> > > > On 05/23/2014 12:06 AM, Eric Blake wrote:
> > > >> On 05/22/2014 12:22 PM, Mike Perez wrote:
> > > >>> This introduces two new attributes "cmd_per_lun" and "max_sectors" same
> > > >>> with the names QEMU uses for virtio-scsi. An example of the XML:
> > > >>>
> > > >>> <controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50'
> > > >>> max_sectors='512'/>
> > > >>
> > > >> I'm not a fan of underscore (why type the shift key, when a dash will
> > > >> do).  The libvirt xml name does not have to exactly match the qemu
> > > >> option name, so maybe there's some room for bikeshedding what names we
> > > >> should actually present to the user.
> > > >>
> > > >
> > > 
> > > > IMO using underscores here would be consistent with other disk-related options
> > > > like read_iops_sec; dashes are mostly used for network-related options.
> > > >
> > > > We could also use camelCase [1], or just roll a dice.
> > > 
> > > Well underscores are originally what I had in my first patch [1]. Eric
> > > what do you think?
> > > 
> > 
> > Can we please keep the libvirt names consistent for virtio-scsi
> > parameters with what is already defined in qemu..?  (Paolo CC'ed)
> > 
> > From qemu/include/hw/virtio/virtio-scsi.h:
> > 
> > #define DEFINE_VIRTIO_SCSI_PROPERTIES(_state, _conf_field)                     \
> >     DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1),       \
> >     DEFINE_PROP_UINT32("max_sectors", _state, _conf_field.max_sectors, 0xFFFF),\
> >     DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128)
> > 
> > AFAICT, having different names between qemu + libvirt parameters can
> > only add confusion.
> 
> QEMU is not the only hypervisor that libvirt targets, so tieing libvirt
> names to QEMU names is a non-goal. We pick the names that make most sense
> in the context of libvirt.
> 

Not sure I follow..  virtio-scsi is specific to QEMU/KVM, and per the
comment in the original patch:

  'Currently it only supports attribute <code>queues</code> (<span class="since">1.0.5</span>, QEMU and KVM only)'

would seem to indicate the parameter names are only used in the context
of QEMU/KVM, no..?

If the virtio-scsi parameters are intended to be used across
hypervisors, then matching them to QEMU's own names doesn't really
matter.  But if they are specific to virtio-scsi and only used by
QEMU/KVM instances, then renaming them to something arbitrary to libvirt
is pointless and confusing.

--nab