From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH V4 22/24] xl: update domain configuration when we hotplug a device Date: Wed, 7 May 2014 11:16:03 +0100 Message-ID: <1399457763.3014.252.camel@kazak.uk.xensource.com> References: <1398949101-23320-1-git-send-email-wei.liu2@citrix.com> <1398949101-23320-23-git-send-email-wei.liu2@citrix.com> <1399391096.3014.181.camel@kazak.uk.xensource.com> <20140506155810.GT17067@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140506155810.GT17067@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: ian.jackson@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Tue, 2014-05-06 at 16:58 +0100, Wei Liu wrote: > > > + libxl_device_pci pcidev, *ppcidev; > > > > Likewise ppcidev here, perhaps meaning you would need to pass the type > > rather then an instance to the variable. > > > > > XLU_Config *config; > > > > > > libxl_device_pci_init(&pcidev); > > > @@ -3059,7 +3090,12 @@ static void pciattach(uint32_t domid, const char *bdf, const char *vs) > > > fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", bdf); > > > exit(2); > > > } > > > - libxl_device_pci_add(ctx, domid, &pcidev, 0); > > > + > > > + ADD_DEVICE(pci, domid, pcidev, &d_config, ppcidev, { > > > + ppcidev = ARRAY_EXTEND_INIT_NODEVID(d_config.pcidevs, > > > + d_config.num_pcidevs, > > > + libxl_device_pci_init); > > > > Do you really need to do a deep copy of pcidev into the new array slot? > > If you just memcpy the thing over and then do not dispose the original > > then the copy will simply take over those allocations, which is fine I > > think. > > > > The copy is done so that we know what user provides at the beginning, > i.e. this is the template that we need to keep. After the structure is > handed back by libxl the content might be altered already, say, all > "default" values replaced by libxl with some values that it deems > sensible. Then fixup will run to copy those bits we care about back to > the template. I see. (A shame, avoiding this deep copy stuff would have simplified all sorts of things!) BTW I think it's not so much the replacement of fields but the freeing of the old values which prevents the shallow copy. > > > + libxl_mac_copy(ctx, &pnic->mac, &nic.mac); > > > > Doesn't the copy handle this already? If not then why not? > > > > This is the fixup. If user doesn't supply uuid, libxl generates one for > this device and we really want to save it in our state. If user supplies > before hand, it just copies the original value back. > > The fixup is device dependent. You may find nic has different fixup and > disk has no fixup at all. I see. eventually we might want to provide this fixup as a libxl helper function, but having it here for now will let us bed it in first. Ian.