On Wed, May 03, 2017 at 05:35:47PM +0200, Pavel Hrdina wrote: > On Fri, Apr 28, 2017 at 09:46:32AM +0200, Erik Skultety wrote: > > virMediatedDeviceListAdd calls VIR_APPEND_ELEMENT which normally clears > > the element to be added, thus the pointer must not be used afterwards, > > but because the pointer here is passed by value, what gets cleared is > > a copy of the original pointer that got created on the stack > > automatically when calling the function. The fact that it works now is > > just a coincidence and a bug in virMediatedDeviceListAdd that needs to > > be fixed in a separate commit. Therefore, use a local variable to hold > > data, rather than accessing the pointer later. > > I don't thing that this patch is required. What happens here is that > in this function we get a pointer to an existing memory that stores > virMediatedDevice, if the mdev is not used we mark it as used and store > that pointer to *dst* list. That's what virMediatedDeviceListAdd does, > it only stores the pointer to an allocated into the list so it doesn't > clear the real data, it only operates with pointers. This means that > the memory addressed by local *mdev* variable still points to a > valid memory and everything is OK, there is no issue at all apart > from a fact, that setting the pointer in virMediatedDeviceListAdd > is pointless. > > Pavel
After reading the second patch, this one will be required, however I would change the commit message because there is no issue right now, this change is only necessary for the following patch. Pavel > > > Signed-off-by: Erik Skultety <eskul...@redhat.com> > > --- > > src/util/virmdev.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/src/util/virmdev.c b/src/util/virmdev.c > > index 2a1ade739..c1499d238 100644 > > --- a/src/util/virmdev.c > > +++ b/src/util/virmdev.c > > @@ -447,20 +447,21 @@ > > virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst, > > virObjectLock(dst); > > for (i = 0; i < count; i++) { > > virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i); > > + const char *mdev_path = mdev->path; > > > > if (virMediatedDeviceIsUsed(mdev, dst) || > > virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0) > > goto cleanup; > > > > /* Copy mdev references to the driver list: > > - * - caller is responsible for NOT freeing devices in @list on > > success > > + * - caller is responsible for NOT freeing devices in @src on > > success > > * - we're responsible for performing a rollback on failure > > */ > > if (virMediatedDeviceListAdd(dst, mdev) < 0) > > goto rollback; > > > > VIR_DEBUG("'%s' added to list of active mediated devices used by > > '%s'", > > - mdev->path, domname); > > + mdev_path, domname); > > } > > > > ret = 0; > > -- > > 2.12.2 > > > > -- > > libvir-list mailing list > > libvir-list@redhat.com > > https://www.redhat.com/mailman/listinfo/libvir-list > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list