Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
Quoting Brendan Higgins (2019-08-13 02:12:54) > On Tue, Aug 13, 2019 at 2:04 AM Brendan Higgins > wrote: > > > > On Mon, Aug 12, 2019 at 10:30 PM Stephen Boyd wrote: > > > > > > Quoting Brendan Higgins (2019-08-12 22:02:59) > > > > However, now that I added the kunit_resource_destroy, I thought it > > > > might be good to free the string_stream after I use it in each call to > > > > kunit_assert->format(...) in which case I will be using this logic. > > > > > > > > So I think the right thing to do is to expose string_stream_destroy so > > > > kunit_do_assert can clean up when it's done, and then demote > > > > string_stream_clear to static. Sound good? > > > > > > Ok, sure. I don't really see how clearing it explicitly when the > > > assertion prints vs. never allocating it to begin with is really any > > > different. Maybe I've missed something though. > > > > It's for the case that we *do* print something out. Once we are doing > > printing, we don't want the fragments anymore. > > Oops, sorry fat fingered: s/doing/done Yes, but when we print something out we've run into some sort of problem and then the test is over. So freeing the memory when it fails vs. when the test is over seems like a minor difference. Or is it also used to print other informational messages while the test is running? I'm not particularly worried here, just trying to see if less code is possible. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
On Tue, Aug 13, 2019 at 2:04 AM Brendan Higgins wrote: > > On Mon, Aug 12, 2019 at 10:30 PM Stephen Boyd wrote: > > > > Quoting Brendan Higgins (2019-08-12 22:02:59) > > > On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd wrote: > > > > > > > > Quoting Brendan Higgins (2019-08-12 17:41:05) > > > > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd wrote: > > > > > > > > > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > > > > > devres_destroy) and use kunit_kfree here? > > > > > > > > > > > > > > > > > > > Yes, or drop the API entirely? Does anything need this > > > > > > functionality? > > > > > > > > > > Drop the kunit_resource API? I would strongly prefer not to. > > > > > > > > No. I mean this API, string_stream_clear(). Does anything use it? > > > > > > Oh, right. No. > > > > > > However, now that I added the kunit_resource_destroy, I thought it > > > might be good to free the string_stream after I use it in each call to > > > kunit_assert->format(...) in which case I will be using this logic. > > > > > > So I think the right thing to do is to expose string_stream_destroy so > > > kunit_do_assert can clean up when it's done, and then demote > > > string_stream_clear to static. Sound good? > > > > Ok, sure. I don't really see how clearing it explicitly when the > > assertion prints vs. never allocating it to begin with is really any > > different. Maybe I've missed something though. > > It's for the case that we *do* print something out. Once we are doing > printing, we don't want the fragments anymore. Oops, sorry fat fingered: s/doing/done ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
On Mon, Aug 12, 2019 at 10:30 PM Stephen Boyd wrote: > > Quoting Brendan Higgins (2019-08-12 22:02:59) > > On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd wrote: > > > > > > Quoting Brendan Higgins (2019-08-12 17:41:05) > > > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd wrote: > > > > > > > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > > > > devres_destroy) and use kunit_kfree here? > > > > > > > > > > > > > > > > Yes, or drop the API entirely? Does anything need this functionality? > > > > > > > > Drop the kunit_resource API? I would strongly prefer not to. > > > > > > No. I mean this API, string_stream_clear(). Does anything use it? > > > > Oh, right. No. > > > > However, now that I added the kunit_resource_destroy, I thought it > > might be good to free the string_stream after I use it in each call to > > kunit_assert->format(...) in which case I will be using this logic. > > > > So I think the right thing to do is to expose string_stream_destroy so > > kunit_do_assert can clean up when it's done, and then demote > > string_stream_clear to static. Sound good? > > Ok, sure. I don't really see how clearing it explicitly when the > assertion prints vs. never allocating it to begin with is really any > different. Maybe I've missed something though. It's for the case that we *do* print something out. Once we are doing printing, we don't want the fragments anymore. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
Quoting Brendan Higgins (2019-08-12 22:02:59) > On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd wrote: > > > > Quoting Brendan Higgins (2019-08-12 17:41:05) > > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd wrote: > > > > > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > > > devres_destroy) and use kunit_kfree here? > > > > > > > > > > > > > Yes, or drop the API entirely? Does anything need this functionality? > > > > > > Drop the kunit_resource API? I would strongly prefer not to. > > > > No. I mean this API, string_stream_clear(). Does anything use it? > > Oh, right. No. > > However, now that I added the kunit_resource_destroy, I thought it > might be good to free the string_stream after I use it in each call to > kunit_assert->format(...) in which case I will be using this logic. > > So I think the right thing to do is to expose string_stream_destroy so > kunit_do_assert can clean up when it's done, and then demote > string_stream_clear to static. Sound good? Ok, sure. I don't really see how clearing it explicitly when the assertion prints vs. never allocating it to begin with is really any different. Maybe I've missed something though. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd wrote: > > Quoting Brendan Higgins (2019-08-12 17:41:05) > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd wrote: > > > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > > devres_destroy) and use kunit_kfree here? > > > > > > > > > > Yes, or drop the API entirely? Does anything need this functionality? > > > > Drop the kunit_resource API? I would strongly prefer not to. > > No. I mean this API, string_stream_clear(). Does anything use it? Oh, right. No. However, now that I added the kunit_resource_destroy, I thought it might be good to free the string_stream after I use it in each call to kunit_assert->format(...) in which case I will be using this logic. So I think the right thing to do is to expose string_stream_destroy so kunit_do_assert can clean up when it's done, and then demote string_stream_clear to static. Sound good? ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
Quoting Brendan Higgins (2019-08-12 17:41:05) > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd wrote: > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > devres_destroy) and use kunit_kfree here? > > > > > > > Yes, or drop the API entirely? Does anything need this functionality? > > Drop the kunit_resource API? I would strongly prefer not to. No. I mean this API, string_stream_clear(). Does anything use it? ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd wrote:
>
> Quoting Brendan Higgins (2019-08-12 16:33:36)
> > On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote:
> > > Quoting Brendan Higgins (2019-08-12 11:24:06)
> > > > +void string_stream_clear(struct string_stream *stream)
> > > > +{
> > > > + struct string_stream_fragment *frag_container,
> > > > *frag_container_safe;
> > > > +
> > > > + spin_lock(&stream->lock);
> > > > + list_for_each_entry_safe(frag_container,
> > > > +frag_container_safe,
> > > > +&stream->fragments,
> > > > +node) {
> > > > + list_del(&frag_container->node);
> > >
> > > Shouldn't we free the allocation here? Otherwise, if some test is going
> > > to add, add, clear, add, it's going to leak until the test is over?
> >
> > So basically this means I should add a kunit_kfree and
> > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > devres_destroy) and use kunit_kfree here?
> >
>
> Yes, or drop the API entirely? Does anything need this functionality?
Drop the kunit_resource API? I would strongly prefer not to.
string_stream uses it; the expectation stuff uses it via string
stream; some of the tests in this patchset allocate memory as part of
the test setup that uses it. The intention is that we would provide a
kunit_res_* version of many (hopefully eventually most) common
resources required by tests and it would be used in the same way that
the devm_* stuff is.
Nevertheless, I am fine adding the kunit_resource_destroy, etc. I just
wanted to make sure I understood what you were asking.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
Quoting Brendan Higgins (2019-08-12 16:33:36)
> On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote:
> > Quoting Brendan Higgins (2019-08-12 11:24:06)
> > > +void string_stream_clear(struct string_stream *stream)
> > > +{
> > > + struct string_stream_fragment *frag_container,
> > > *frag_container_safe;
> > > +
> > > + spin_lock(&stream->lock);
> > > + list_for_each_entry_safe(frag_container,
> > > +frag_container_safe,
> > > +&stream->fragments,
> > > +node) {
> > > + list_del(&frag_container->node);
> >
> > Shouldn't we free the allocation here? Otherwise, if some test is going
> > to add, add, clear, add, it's going to leak until the test is over?
>
> So basically this means I should add a kunit_kfree and
> kunit_resource_destroy (respective equivalents to devm_kfree, and
> devres_destroy) and use kunit_kfree here?
>
Yes, or drop the API entirely? Does anything need this functionality?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote:
> Quoting Brendan Higgins (2019-08-12 11:24:06)
> > +void string_stream_clear(struct string_stream *stream)
> > +{
> > + struct string_stream_fragment *frag_container, *frag_container_safe;
> > +
> > + spin_lock(&stream->lock);
> > + list_for_each_entry_safe(frag_container,
> > +frag_container_safe,
> > +&stream->fragments,
> > +node) {
> > + list_del(&frag_container->node);
>
> Shouldn't we free the allocation here? Otherwise, if some test is going
> to add, add, clear, add, it's going to leak until the test is over?
So basically this means I should add a kunit_kfree and
kunit_resource_destroy (respective equivalents to devm_kfree, and
devres_destroy) and use kunit_kfree here?
> > + }
> > + stream->length = 0;
> > + spin_unlock(&stream->lock);
> > +}
> > +
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
Quoting Brendan Higgins (2019-08-12 11:24:06)
> +void string_stream_clear(struct string_stream *stream)
> +{
> + struct string_stream_fragment *frag_container, *frag_container_safe;
> +
> + spin_lock(&stream->lock);
> + list_for_each_entry_safe(frag_container,
> +frag_container_safe,
> +&stream->fragments,
> +node) {
> + list_del(&frag_container->node);
Shouldn't we free the allocation here? Otherwise, if some test is going
to add, add, clear, add, it's going to leak until the test is over?
> + }
> + stream->length = 0;
> + spin_unlock(&stream->lock);
> +}
> +
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
