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 <eskultet@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/libvir-list
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
