Re: [igt-dev] [PATCH i-g-t 2/4] Introduce device selection API

[Arkadiusz Hiler <arkadiusz.hiler@intel.com>]

On Thu, Oct 24, 2019 at 06:40:31PM +0200, Zbigniew Kempczyński wrote:
> On Thu, Oct 24, 2019 at 02:05:13PM +0300, Arkadiusz Hiler wrote:
> 
> <cut>
> 
> I've taken a quick look and I think this require change:
> 
> > +/**
> > + * igt_device_filter_set
> > + * @filter: filter that should be set globally
> > + *
> > + * Returns number of filters added to filter array. Can be greater than
> > + * 1 if @filters contains more than one filter separated by semicolon.
> > + */
> > +void igt_device_filter_set(const char *filter)
> > +{
> > +	if (!is_filter_valid(filter)) {
> > +		igt_warn("Invalid filter: %s\n", filter);
> > +		return;
> > +	}
> > +
> > +	device_filter = strdup(filter);
> > +}
> 
> Comment informs function returns number of filters but it returns void
> now. What we should also protect is to memleak if user passes different
> filters because we don't free previous device_filter string if already
> set.
> 
> Same in the header file.
> 
> > +
> > +/**
> > + * igt_device_filter_free
> > + *
> > + * Free the filter that we store internally, effectively insetting it.
> > + */
> > +void igt_device_filter_free(void)
> > +{
> > +	if (device_filter != NULL)
> > +		free(device_filter);
> > +}
> 
> I would also add nullyfiing device_filter here.
> 
> That's for brief look.
> 
> Thanks for reviewing and rewriting patches.

The following changes will be included in the next revision. Thanks!

diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
index 71d30587..c3c99001 100644
--- a/lib/igt_device_scan.c
+++ b/lib/igt_device_scan.c
@@ -1050,9 +1050,6 @@ static bool is_filter_valid(const char *fstr)
 /**
  * igt_device_filter_set
  * @filter: filter that should be set globally
- *
- * Returns number of filters added to filter array. Can be greater than
- * 1 if @filters contains more than one filter separated by semicolon.
  */
 void igt_device_filter_set(const char *filter)
 {
@@ -1061,18 +1058,23 @@ void igt_device_filter_set(const char *filter)
 		return;
 	}
 
+	if (device_filter != NULL)
+		free(device_filter);
+
 	device_filter = strdup(filter);
 }
 
 /**
  * igt_device_filter_free
  *
- * Free the filter that we store internally, effectively insetting it.
+ * Free the filter that we store internally, effectively unsetting it.
  */
 void igt_device_filter_free(void)
 {
 	if (device_filter != NULL)
 		free(device_filter);
+
+	device_filter = NULL;
 }
 
 /**
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev