Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

On Fri, Sep 29, 2017 at 3:49 PM, Michal Privoznik <mprivozn(a)redhat.com&gt; wrote: > On 09/29/2017 01:16 PM, Peter Krempa wrote: >> On Fri, Sep 29, 2017 at 12:57:29 +0200, Michal Privoznik wrote: >>> On 09/29/2017 09:52 AM, Peter Krempa wrote: >>>> On Fri, Sep 29, 2017 at 09:06:01 +0200, Michal Privoznik wrote: >>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1434451 >>>>> >>>>> It comes handy for management application to be able to have a >>>>> per-device label so that it can uniquely identify devices it >>>>> cares about. The advantage of this approach is that we don't have >>>>> to generate aliases at define time (non trivial amount of work >>>>> and problems). The only thing we do is parse the user supplied >>>>> tag and format it back. For instance: >>>>> >>>>> <disk type='block' device='disk'> >>>>> <driver name='qemu' type='raw'/> >>>>> <source dev='/dev/HostVG/QEMUGuest1'/> >>>>> <target dev='hda' bus='ide'/> >>>>> <alias user='myDisk0'/> >>>>> <address type='drive' controller='0' bus='0' target='0' > unit='0'/> >>>>> </disk> >>>>> >>>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>>>> --- >>>>> >>>>> An alternative approach to: >>>>> >>>>> https://www.redhat.com/archives/libvir-list/2017- > September/msg00765.html >>>>> >>>>> Honestly, I prefer this one as it's simpler and we don't have to care > about >>>>> devices changing their aliases on cold plug. I mean, on cold > (un-)plug we'd >>>>> have to regenerate the aliases so it might be hard to track certain > device. >>>>> But with this approach, it's no problem. >>>>> >>>>> Also, I'm not completely convinced about the name of @user attribute. > So I'm >>>>> open for suggestions. >>>> >>>> With this proposed design you need to make sure to document that the >>>> user alias is _not_ guaranteed to be unique and also that it can't be >>>> used to match the device in any way. >>>> >>> >>> Sure. I'll just add it at the end of the paragraph that describes >>> <alias/>. Err, hold on. There's none! But okay, I think I can find a >>> place to add it there. >>> >>> Even though my patch doesn't implement it (simply because I wanted to >>> have ACK on the design so I've saved it for follow up patches), I don't >>> quite see why we can't match user supplied aliases? >> >> I think it will introduce more problems than solve. You will need to >> make sure that it's unique even across hotplug and add lookup code for >> everything. Additionally you can't add that as a mandatory field when >> looking up, since some device classes allow lookup only with partial XML >> descriptions (for unplug/update). > > It can't be mandatory, that's the whole point. Sure. Well, in > DomainPostParse I can check for uniqueness pretty easily: just create an > empty list and start adding all strings provided by user. If the string > we're trying to add is already in the list we just error out. Sure, I'll > have to iterate over all devices, but that should be pretty easy since > we have virDomainDeviceInfoIterate(). All that's needed is to write the > callback function (= a few lines of code). Then, on hotplug goto 10. > IOW, if user alias is provided just check for its uniqueness. > >> >>> virsh domif-setlink fedora myNet down >>> >>> This has the great advantage of being ordering agnostic. You don't have >>> to check for the alias of the device you want to modify (as aliases can >>> change across different startups, right?). True, for that we would have >> >> Well, you've used a bad example for this. 'domif-setlink' uses target and >> mac address, both of which are stable and don't rely on ordering on >> startup. Same thing applies to disks. > > Oh right. domiftune is more like it. > >> >> The matching in virsh in this particular command is done by looking for >> the full element and then using that with the UpdateDevice() API, so the >> lookup is basically syntax sugar. >> >>> to make sure that the supplied aliases are unique per domain (which is >>> trivial to achieve). But apart from that I don't quite see why we >>> shouldn't/can't do it. >> >> Well, I don't think it's trivial. It's simpler than providing the alias >> which would be used with qemu, but you still have a zillion hotplug code >> paths which would need to check this. > > Well, it might be slightly tricky yes. But in general, the > virDomain*Find() would try to match the user alias first (if provided) > else continue with current behaviour. I'm not quite sure what you mean > by zillion code paths. > >> >>>> I think that users which know semantics of the current alias may be > very >>>> confused by putting user data with different semantics into the same >>>> field. >>>> >>> >>> Yep. As I say, I'm not happy with the name either. Any suggestion is >>> welcome. So a separate element then? Naming is hard. >> >> I'm voting for separate element in case there's consensus that this >> route is a good idea. >> > > Yeah, it may look a bit better. Any idea on the element name? > How about "label" or "userlabel"?