[Libvir] [PATCH] Allow modifying existing block device, making virDomainAttachDevice modify the device if it already exists

[Daniel P. Berrange]

On Thu, Sep 06, 2007 at 11:59:08AM -0400, Daniel Veillard wrote:
> On Thu, Sep 06, 2007 at 11:45:39AM -0400, Hugh Brock wrote:
> > The attached patch adds code to xend_internal.c:xenDomainAttachDevice 
> > that checks to see if the device mentioned in the passed-in XML already 
> > exists, and if so calls op_device_configure to modify the device. It is 
> > particularly useful for connecting and disconnecting a hardware cdrom 
> > device to an FV guest.
> > 
> > I considered making new API a la virDomainModifyDevice, but decided 
> > overloading virDomainAttachDevice was cleaner (and didn't change 
> > existing API).
> > 
> > I have tested the patch on f7 with xen 3.1 and a windows 2000 guest. 
> > Since the patch merely wraps Xen's device_configure call, we're not 
> > adding much risk of breakage.
> 
>   Okay looks sensible. 
> > diff -ruN libvirt.orig/src/xend_internal.c libvirt/src/xend_internal.c
> > --- libvirt.orig/src/xend_internal.c	2007-09-06 11:28:05.000000000 -0400
> > +++ libvirt/src/xend_internal.c	2007-09-06 10:47:25.000000000 -0400
> > @@ -3087,6 +3087,7 @@
> >      char *sexpr, *conf, *str;
> >      int hvm = 0, ret;
> >      xenUnifiedPrivatePtr priv;
> > +    char class[8], ref[80];
> >  
> >      if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) {
> >          virXendError((domain ? domain->conn : NULL), VIR_ERR_INVALID_ARG,
> > @@ -3116,8 +3117,16 @@
> >          *(conf + strlen(conf) -1) = 0; /* suppress final ) */
> >      }
> >      else conf = sexpr;
> > -    ret = xend_op(domain->conn, domain->name, "op", "device_create",
> > -        "config", conf, NULL);
> > +    if (virDomainXMLDevID(domain, xml, class, ref)) {
> > +        /* device doesn't exist, define it */
> > +        ret = xend_op(domain->conn, domain->name, "op", "device_create",
> > +                      "config", conf, NULL);
> > +    } 
> > +    else {
> > +        /* device exists, attempt to modify it */
> > +        ret = xend_op(domain->conn, domain->name, "op", "device_configure", 
> > +                      "config", conf, "dev", ref, NULL);
> > +    }
> >      free(sexpr);
> >      return ret;
> >  }
> 
>   virDomainXMLDevID looks frightening to me since we write to an array of
> undisclosed size, but that is independant from the patch which looks fine to
> me.

Yep, this patch is fine, but we absolutely have to fix virDomainXMLDevID
as the value it writes into the pre-allocated 'ref' parameter is taken from
an XML attribute which can be an arbitrary size & can thus potentially 
overflow the '80' char buffer passed from xenDaemonDetachDevice or this
new call for changing CD media.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|