Re: [libvirt] [PATCH v2 01/12] virstoragefile: Introduce virStoragePRDef

On 02/21/2018 01:11 PM, Michal Privoznik wrote: The design may have been agreed upon, but sometimes once a bit of work is done and the problem better understood the design can change. I still don't understand what the purpose of enabled='no' would be. Especially since when set to 'no', the <source> isn't formatted. I'd prefer to see a bit more wordy commit to describe the functionality and what it's used for... others, well I know, less is better because it's all self documenting anyway... In particular it's "confusing" what's done for when <source> is provided and what is done when it's not. See note below w/r/t managed='no' Since 4.2.0, the optional <code>reservations</code> sub-element of the storage <code>source</code> element can be used to enable persistent reservations for SCSI based disks (QEMU driver only). The element has one mandatory attribute <code>managed</code> set to either 'yes' or 'no'. When <code>managed</code> is 'yes', libvirt will create a helper process to manage the reservations and will check at restart for the helper process. When <code>managed</code> is 'no', libvirt will provide no management of the helper process. If libvirt is managing the helper process, then the <code>source</code> sub-element must be provided with mandatory attributes <code>type</code>, <code>path</code>, and <code>mode</code>. The <code>type</code> attribute currently only accepts 'unix' as an option and the <code>mode</code> attribute only accepts 'client'. The <code>path</code> attribute supplies the path to the UNIX socket for communication between libvirt and the helper process. FWIW: What I wrote above is backwards from the implementation for managed. At least it makes more sense to me. If libvirt is managing something, then it will do the work, if libvirt is not managing something then it doesn't do anything. Think of how hostdev's are managed or not. When managed, libvirt will detach and reattach. When not managed the user is responsible to perform the detach and reattach. BTW: While writing this I'm now wondering - so how does the managed='no' path actually find or communicate with the helper process? Maybe that answer comes later. Why does this hunk move to storagecommon.rng? Since it's going to be common between domain and storage (for vhostuser and persistent reservations), maybe it should move to basictypes.rng. Perhaps all of the following should be in basictypes.rng? Or some other new common rng file... Common between domain and storage where we seem to be having more and more crossover. The parse/format won't allow the mode=server value nor will it do any sort of reconnect... Personally, it's a bit "confusing" (at best) to have the Format routine be virStoragePRDefFormat Consider my thoughts above vis-a-vis managed. missing connection path (to be consistent) BTW: Seems pretty reasonable to add the same reconnect options that vhostuser has. I mean if the helper process dies does libvirt need to be restarted in order to recognize and restart? Or does it restart after some time period when it recognizes the helper process died? So if someone changes enabled from 'yes' to 'no', then if they had anything in the <source> buffer it'd be lost. So I see no reason for enabled As noted earlier this seems backwards to me. So no chance of this being 'server', thus what's the point? As noted earlier the managed attribute seems backwards. As I noted earlier this seems backwards. John