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

On Tue, Jul 01, 2014 at 06:51:46PM -0700, Nicholas A. Bellinger wrote: > Hi Mike & Co, > > > ---------- Forwarded message ---------- > > From: Mike Perez <thingee(a)gmail.com&gt; > > 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(a)redhat.com&gt; > > Cc: Eric Blake <eblake(a)redhat.com&gt;, libvir-list(a)redhat.com, > > berrange(a)redhat.com, &quot;Nab(a)daterainc.com&quot; <Nab(a)daterainc.com&gt; > > > > > > On Thu, Jun 5, 2014 at 6:57 AM, Ján Tomko <jtomko(a)redhat.com&gt; 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.