Re: [Qemu-devel] [PATCH 4/5] Add generic drive hotplugging

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> On Tue, Aug 24, 2010 at 03:46:14PM +0200, Alexander Graf wrote:
>> Daniel P. Berrange wrote:
>> > On Tue, Aug 24, 2010 at 03:40:25PM +0200, Alexander Graf wrote:
>> >   
>> >> Daniel P. Berrange wrote:
>> >>     
>> >>> On Tue, Aug 24, 2010 at 12:45:19PM +0200, Alexander Graf wrote:
>> >>>       
>> >>>>> The key is that you should use  if=none for all cases. Here are two
>> >>>>> examples of how libvirt does it currently:
>> >>>>>
>> >>>>> VirtIO:
>> >>>>>
>> >>>>>   drive_add dummy 
>> >>>>> file=/var/lib/libvirt/images/data.img,if=none,id=drive-virtio-disk1,format=raw
>> >>>>>   device_add 
>> >>>>> virtio-blk-pci,bus=pci.0,addr=0x0,drive=drive-virtio-disk1,id=virtio-disk1'
>> >>>>>
>> >>>>> SCSI:
>> >>>>>
>> >>>>>   drive_add dummy 
>> >>>>> file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw'
>> >>>>>   device_add 
>> >>>>> scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>> >>>>>
>> >>>>> The 'dummy' value there can be absolutely anything you want.
>> >>>>> It is totaly ignored when QEMU sees if=none in 2nd arg.
>> >>>>>   
>> >>>>>       
>> >>>>>           
>> >>>> I'd be all for removing the pci-hotplug.c version of drive_add then. But
>> >>>> I think the IF_SCSI option there is to append a drive to an existing
>> >>>> SCSI bus, no?
>> >>>>     
>> >>>>         
>> >>> Actually this SCSI example I give above is appending a drive to an 
>> >>> existing
>> >>> bus (scsi0), in slot 1 (scsi-id=1).  To best of my knowledge there is no
>> >>> remaining use case that requires use of IF_SCSI, IF_IDE, etc. The IF_NONE
>> >>> approach can cope with all, modulo bugs that appear periodically with 
>> >>> code
>> >>> that mistakenly checks for a particular IF_XXX constant.
>> >>>
>> >>> If you wanted to also create a new SCSI bus, before creating the drive on
>> >>> it, you'd need to run three commands in total:
>> >>>
>> >>>   device_add lsi,id=scsi0,bus=pci.0,addr=0x7
>> >>>   drive_add dummy 
>> >>> file=/var/lib/libvirt/images/data.img,if=none,id=drive-scsi0-0-1,format=raw
>> >>>   device_add 
>> >>> scsi-disk,bus=scsi0.0,scsi-id=1,drive=drive-scsi0-0-1,id=scsi0-0-1
>> >>>   
>> >>>       
>> >> Nice - so we can just deprecate if=!none?
>> >>     
>> >
>> > In theory yes, but its not nice to tell users to switch everything over to
>> > use if=none, if we're going to deprecate that too in the next release when
>> > blockdev appears. Might as well just deprecate entire of drive_add/-drive
>> > at once.
>> >   
>> 
>> I guess I still fail to see the reason for blockdev when we force
>> drive_add to if=none...
>
> Markus can no doubt explain better than me, but off the top of my head
>  
>  - 'drive' has guest properties that should be against the device
>    not the drive which is for host properties (eg serial=, if=)
>  - 'file' is mangled to include protocol/format which means that 
>    it can't be unambigously parsed. (eg filenames containing :)
>
> Fixing those, particularly the latter, would breaks back-compat so we
> really need a new arg with sensible definition. This is what blockdev
> is intended todo (as well as a internal code cleanup)

Yes, -drive and drive_add are a tangled mess.  I decided to start with a
clean slate, because getting it right is difficult enough without the
historical baggage.

Perhaps the end result can be hammered into the drive_add mold somehow,
by adding a few new options and deprecating a few old ones.  Let's
decide when we get there.

The goal is clean separation of host and guest properties.  device_add
is for guest properties, blockdev_add is for host properties.  Just like
netdev_add, only for block instead of network.

We also want sufficient flexibility (or at least extensibility) to be
able to specify any useful host block driver configuration.  And all
that without the syntactic embarrassments that plague drive_add, such as
':' in filenames.