On Mon, Apr 10, 2017 at 03:35:10PM +0200, Erik Skultety wrote:
> > > +void virNodeDevCapMdevFree(virNodeDevCapMdevPtr mdev)
> > > +{
> > > + if (!mdev)
> > > + return;
> > > +
> > > + VIR_FREE(mdev->type);
> > > + VIR_FREE(mdev->name);
> > > + VIR_FREE(mdev->description);
> > > + VIR_FREE(mdev->device_api);
> > > + VIR_FREE(mdev);
> > > +}
> > > +
>
> [...]
>
> > > +
> > > +typedef struct _virNodeDevCapMdev virNodeDevCapMdev;
> > > +typedef virNodeDevCapMdev *virNodeDevCapMdevPtr;
> > > +struct _virNodeDevCapMdev {
> > > + char *type;
> > > + char *name;
> > > + char *description;
> > > + char *device_api;
> > > + unsigned int available_instances;
> > > + unsigned int iommuGroupNumber;
> > > +};
> > > +
> >
> > Introducing this structure can be moved into the next patch, it's not
> > used here.
>
> It actually is, virNodeDevCapMdevFree uses is, and it also used in the snippet
> below. Anyhow, I usually try to introduce concepts, data types and other
The free function itself isn't used anywhere in this patch and also the
variable in struct is not used, that was my point.
> symbols first to make it a tiny bit easier for the reviewer, since this can
> be easily missed when actually implementing function bodies, or logic in
I think that data structures should be grouped with the logic itself, they
usually have no meaning without each other.
This patch introduces the mdev capability which makes sense to keep it in
a separate patch, but the capability itself doesn't require the structure,
it would be better to place it into the next patch that actually implements
the capability.
Pavel
> general. I have no problem with moving everything related to the structure to
> the following patch. Although in that case, if this patch would still be worth
> having as separate or more-or-less merge it with the next one completely. Let
> me know, what you find as the most plausible solution for you.
>
> Erik
>
> >
> > > typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev;
> > > typedef virNodeDevCapPCIDev *virNodeDevCapPCIDevPtr;
> > > struct _virNodeDevCapPCIDev {
> > > @@ -262,6 +274,7 @@ struct _virNodeDevCapData {
> > > virNodeDevCapStorage storage;
> > > virNodeDevCapSCSIGeneric sg;
> > > virNodeDevCapDRM drm;
> > > + virNodeDevCapMdev mdev;
> > > };
> > > };
> > >
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
