Re: [libvirt] [dbus PATCH 02/10] GetVcpus API method: Renamed to GetVcpusFlags

On Fri, Mar 23, 2018 at 03:43:45PM +0100, Pavel Hrdina wrote: > On Fri, Mar 23, 2018 at 02:25:25PM +0000, Daniel P. Berrangé wrote: > > On Fri, Mar 23, 2018 at 03:16:59PM +0100, Katerina Koukiou wrote: > > > Same for internal virtDBusDomainGetVcpus: > > > Renamed to virtDBusDomainGetVcpusFlags > > > > > > Following naming from libvirt API. > > > > > > Signed-off-by: Katerina Koukiou <kkoukiou(a)redhat.com&gt; > > > --- > > > data/org.libvirt.Domain.xml | 2 +- > > > src/domain.c | 17 ++++++++--------- > > > test/test_domain.py | 2 +- > > > 3 files changed, 10 insertions(+), 11 deletions(-) > > > > > > diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml > > > index 1ecf826..46cc8a7 100644 > > > --- a/data/org.libvirt.Domain.xml > > > +++ b/data/org.libvirt.Domain.xml > > > @@ -11,7 +11,7 @@ > > > <property name="Persistent" type="b" access="read"/> > > > <property name="State" type="s" access="read"/> > > > <property name="Autostart" type="b" access="read"/> > > > - <method name="GetVcpus"> > > > + <method name="GetVcpusFlags"> > > > > We only added Flags as a suffix in our C apis because we have no other > > way to fix it without breaking ABI compat. > > > > In some language bindings, however, we didn't preserve that naming, > > instead just adding 'flags' as an optional parameter. > > > > DBus doesn't have optional params, but since this is a green-field API, > > we don't have a backcompat problem to worry about. > > > > IOW, I suggest *not* adding "Flags" as a suffix to any of the DBus > > method names, even if you ultimately call a libvirt API named > > with a "Flags" suffix. > > I was thinking about not following the names exactly but on the other > hand it may leads into a confusion especially when we have the non-flags > version of the same API. > > I personally don't like the API names and I would gladly remove the > suffix from the D-Bus API names, but it may lead into a confusion about > which libvirt API is used under the hood. Sure, the API takes flags > so it will be probably the flags version, but we as libvirt developers > know this fact, on the other hand users might not realize that. > > If we decide not to follow the libvirt API names, we should probably > somehow document which libvirt API is used. In the Go binding I did this in the embedded API docs via link to the C API docs // See also https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainAttachDevice func (d *Domain) AttachDevice(xml string) error { cXml := C.CString(xml) defer C.free(unsafe.Pointer(cXml)) result := C.virDomainAttachDevice(d.ptr, cXml) if result == -1 { return GetLastError() } return nil } IIRC, the dbus introspection XML has something for docs comments where you could do the same. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|