Re: [libvirt] [PATCH] xen: Prevent updating device when attaching a device

On 02/08/2011 04:18 AM, Osier Yang wrote: > When attaching a device that already exists, xend driver updates > the device with "device_configure", it causes problems (e.g. for > disk device, 'device_configure' only can be used to update device > like CDROM), on the other hand, we provide additional API > (virDomainUpdateDevice) to update device, this fix is to raise up > errors instead of updating the existed device. > > * src/xen/xend_internal.c > --- > src/xen/xend_internal.c | 34 +++++++++++++++++++++++++++++----- > 1 files changed, 29 insertions(+), 5 deletions(-) The code makes sense to me. However, I'm worried that you've introduced a regression in behavior, so you might want to wait for a second ACK. > @@ -4065,17 +4090,16 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml, > /* device doesn't exist, define it */ > ret = xend_op(domain->conn, domain->name, "op", "device_create", > "config", sexpr, NULL); > - } > - else { > - /* device exists, attempt to modify it */ > - ret = xend_op(domain->conn, domain->name, "op", "device_configure", > - "config", sexpr, "dev", ref, NULL); This code dates back to Sep 2007 (commit fcf1b59), which was prior to the creation of virDomainUpdateDevice (Mar 2010, commit 46a2ea3). That is, any client that wanted to change CDROM had to use AttachDevice between 2007 and Mar 2010, even though AttachDevice would corrupt a non-CDROM device. Can you rework the patch to decide between device_configure (for CDROM) vs. error (for all other types), instead of blindly returning error, so that existing uses of AttachDevice for CDROM alterations that pre-dated UpdateDevice will still work?