Re: [libvirt] [PATCH 4/9] Public API Implementation

Daniel P. Berrange wrote: > On Tue, Jan 19, 2010 at 07:40:14PM +0000, Daniel P. Berrange wrote: > >> On Thu, Jan 14, 2010 at 10:42:41AM -0700, Jim Fehlig wrote: >> >>> Implementation of public API for virDomain{Attach,Detach}DeviceFlags. >>> --- >>> src/libvirt.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- >>> 1 files changed, 98 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/libvirt.c b/src/libvirt.c >>> index 1145561..77f76bc 100644 >>> --- a/src/libvirt.c >>> +++ b/src/libvirt.c >>> @@ -5129,7 +5129,6 @@ error: >>> int >>> virDomainAttachDevice(virDomainPtr domain, const char *xml) >>> { >>> - virConnectPtr conn; >>> DEBUG("domain=%p, xml=%s", domain, xml); >>> >>> virResetLastError(); >>> @@ -5147,17 +5146,63 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) >>> virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); >>> goto error; >>> } >>> + >>> + return virDomainAttachDeviceFlags(domain, xml, >>> + VIR_DOMAIN_DEVICE_MODIFY_LIVE); >>> + >>> +error: >>> + virDispatchError(domain->conn); >>> + return -1; >>> +} >>> >> This looks safe, but there's a subtle problem with changing the existing >> virDomainAttachDevice() entry point to call virDomainAttachDeviceFlags(). >> It will break compatability with old libvirtd, even though the old >> libvirtd supports the virDomainAttachDevice() code. Ah, yes - good catch. Thanks. >> So we need to keep >> the distinct paths in the public API & driver definitions. The eventual >> low level hypervisor drivers can of course just turn their existing >> impl into a thin wrapper to the new method.. >> > > There's one other option actually - we could put compatability code in > the remote driver client instead. Either, make it always invoke the old > RPC call if flags == VIR_DOMAIN_DEVICE_MODIFY_LIVE, or have it invoke > the new RPC call & fallback to the old RPC call if it gets an error > indicating the new one doesn't exist. > Do you prefer the latter option? After a quick look, I didn't spot any existing compatibility code in the remote driver client. The first option might be slightly better wrt maintenance.