Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility

On Fri, 14 Jun 2019 17:06:15 +0200 Christophe de Dinechin <cdupontd(a)redhat.com&gt; wrote: > > On 14 Jun 2019, at 16:23, Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > > > On Fri, 14 Jun 2019 11:54:42 +0200 > > Christophe de Dinechin <cdupontd(a)redhat.com&gt; wrote:

> > Where is the parent/type ownership implied? > > I did not imply it, but I read some concern about ownership > on your part in "they need to guess that an mdev device > with the same parent and type is *theirs*.” (emphasis mine) > > I personally see no change on the “need to guess” implied > by the fact that you run uuidgen inside the script, so > that’s why I tried to guess what you meant. As I noted in the reply to the pull request, putting `uuidgen` inline was probably a bad example.

However, the difference is that the user has imposed the race on themselves if they invoke mdevctl like this, they've provided a uuid but they didn't record what it is. This is the user's problem. Pushing uuid selection into mdevctl makes it mdevctl's problem because the interface is fundamentally broken. > > The intended semantics are > > "try to create this type of device under this parent”. > > Agreed. Which is why I don’t see why trying to create > with some new UUID introduces any race (as long as > the script prints out that UUID, which I admit my patch > entirely failed to to) And that's the piece that makes it fundamentally broken. Beyond that, it seems unnecessary. I don't see this as the primary invocation of mdevctl and the functionality it adds is trivially accomplished in a wrapper, so what's the value? > >>> How do you resolve two instances of this happening in parallel and both > >>> coming to the same conclusion which is their device. If a user wants > >>> this sort of headache they can call mdevctl with `uuidgen` but I don't > >>> think we should encourage it further. > >> > >> I agree there is a race, but if anything, having a usage where you don’t > >> pass the UUID on the command line is a step in the right direction. > >> It leaves the door open for the create-mdev script to do smarter things, > >> like deferring the allocation of the mdevs to an entity that has slightly > >> more knowledge of the global system state than uuidgen. > > > > A user might (likely) require a specific uuid to match their VM > > configuration. I can only think of very niche use cases where a user > > doesn't care what uuid they get. > > They do care. But I typically copy-paste my UUIDs, and then > > 1. copy-pasting at the end is always faster than between > the command and other arguments (3-args case). > > 2. copy-pasting the output of the previous command is faster > than having one extra step where I need to copy the same thing twice > (2-args case). > > So to me, if the script is intended to be used by humans, my > proposal makes it slightly more comfortable to use. Nothing more. This is your preference, but I wouldn't call it universal. Specifying the uuid last seems backwards to me, we're creating an object so let's first name that object. We then specify where that object should be created and what type it has. This seems very logical to me, besides, it's also the exact same order we use when listing mdevs :P Clearly there's personal preference here, so let's not arbitrarily pick a different preference. If copy/paste order is more important to you then submit a patch to give mdevctl real argument processing so you can specify --uuid, --parent, --type in whatever order you want.