| Message ID | 20190307154316.19194-5-ykaradzhov@vmware.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Various minor modifications and fixes toward KS 1.0 | expand |
On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote: > > const char *lib = plugin.toStdString().c_str(); > > This line is a bad idea because the returned array may (will) be > invalidated when the destructor of std::string is called. > > Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com> > --- > kernel-shark/src/KsUtils.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp > index 34b2e2d..d7b1753 100644 > --- a/kernel-shark/src/KsUtils.cpp > +++ b/kernel-shark/src/KsUtils.cpp > @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx) > > auto lamRegUser = [&kshark_ctx](const QString &plugin) > { > - const char *lib = plugin.toStdString().c_str(); > + const char *lib = plugin.toLocal8Bit().data(); > kshark_register_plugin(kshark_ctx, lib); > }; > > @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx) > > auto lamUregUser = [&kshark_ctx](const QString &plugin) > { > - const char *lib = plugin.toStdString().c_str(); > + const char *lib = plugin.toLocal8Bit().data(); > kshark_unregister_plugin(kshark_ctx, lib); > }; Doesn't the new version have the same problem with the temporary QByteArray? How about saving the data in a local std::string/QByteArray variable? std::string lib = plugin.toStdString(); kshark_register_plugin(kshark_ctx, lib.c_str()); Thanks! -- Slavomir Kaslev
On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote: > On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote: >> >> const char *lib = plugin.toStdString().c_str(); >> >> This line is a bad idea because the returned array may (will) be >> invalidated when the destructor of std::string is called. >> >> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com> >> --- >> kernel-shark/src/KsUtils.cpp | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp >> index 34b2e2d..d7b1753 100644 >> --- a/kernel-shark/src/KsUtils.cpp >> +++ b/kernel-shark/src/KsUtils.cpp >> @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx) >> >> auto lamRegUser = [&kshark_ctx](const QString &plugin) >> { >> - const char *lib = plugin.toStdString().c_str(); >> + const char *lib = plugin.toLocal8Bit().data(); >> kshark_register_plugin(kshark_ctx, lib); >> }; >> >> @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx) >> >> auto lamUregUser = [&kshark_ctx](const QString &plugin) >> { >> - const char *lib = plugin.toStdString().c_str(); >> + const char *lib = plugin.toLocal8Bit().data(); >> kshark_unregister_plugin(kshark_ctx, lib); >> }; > > Doesn't the new version have the same problem with the temporary QByteArray? > > How about saving the data in a local std::string/QByteArray variable? > > std::string lib = plugin.toStdString(); > kshark_register_plugin(kshark_ctx, lib.c_str()); > Hi Slavi, As far I can understand toStdString() will create a fresh std::string and this string will has its own copy of the characters. However, this copy gets out-of-scope as soon as we hit the semicolon of the line. My understanding was that toLocal8Bit().data() makes no copy so the array will go out-of-scope only when the QString is destroyed. But anyway, your solution looks cleaner and safer. I will send another version of the patch. Thanks a lot! Y. > Thanks! > > > -- > Slavomir Kaslev >
On Mon, Mar 11, 2019 at 3:20 PM Yordan Karadzhov (VMware) <y.karadz@gmail.com> wrote: > > > > On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote: > > On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote: > >> > >> const char *lib = plugin.toStdString().c_str(); > >> > >> This line is a bad idea because the returned array may (will) be > >> invalidated when the destructor of std::string is called. > >> > >> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com> > >> --- > >> kernel-shark/src/KsUtils.cpp | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp > >> index 34b2e2d..d7b1753 100644 > >> --- a/kernel-shark/src/KsUtils.cpp > >> +++ b/kernel-shark/src/KsUtils.cpp > >> @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx) > >> > >> auto lamRegUser = [&kshark_ctx](const QString &plugin) > >> { > >> - const char *lib = plugin.toStdString().c_str(); > >> + const char *lib = plugin.toLocal8Bit().data(); > >> kshark_register_plugin(kshark_ctx, lib); > >> }; > >> > >> @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx) > >> > >> auto lamUregUser = [&kshark_ctx](const QString &plugin) > >> { > >> - const char *lib = plugin.toStdString().c_str(); > >> + const char *lib = plugin.toLocal8Bit().data(); > >> kshark_unregister_plugin(kshark_ctx, lib); > >> }; > > > > Doesn't the new version have the same problem with the temporary QByteArray? > > > > How about saving the data in a local std::string/QByteArray variable? > > > > std::string lib = plugin.toStdString(); > > kshark_register_plugin(kshark_ctx, lib.c_str()); > > > > Hi Slavi, > > As far I can understand toStdString() will create a fresh std::string > and this string will has its own copy of the characters. However, this > copy gets out-of-scope as soon as we hit the semicolon of the line. > > My understanding was that toLocal8Bit().data() makes no copy so the > array will go out-of-scope only when the QString is destroyed. I did some digging into QString's implementation. From my reading of the code, toLocal8Bit() calls into qt_convert_to_latin1()[1] which does allocation, copying/transform-to-latin1 and returns the QByteArray. So it seems that toLocal8Bit() is still making a copy. [1] https://github.com/qt/qtbase/blob/5.12/src/corelib/tools/qstring.cpp#L5275 Cheers, -- Slavi > > But anyway, your solution looks cleaner and safer. > I will send another version of the patch. > Thanks a lot! > Y. > > > > Thanks! > > > > > > -- > > Slavomir Kaslev > >
On 11.03.19 г. 15:44 ч., Slavomir Kaslev wrote: > On Mon, Mar 11, 2019 at 3:20 PM Yordan Karadzhov (VMware) > <y.karadz@gmail.com> wrote: >> >> >> >> On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote: >>> On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote: >>>> >>>> const char *lib = plugin.toStdString().c_str(); >>>> >>>> This line is a bad idea because the returned array may (will) be >>>> invalidated when the destructor of std::string is called. >>>> >>>> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com> >>>> --- >>>> kernel-shark/src/KsUtils.cpp | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp >>>> index 34b2e2d..d7b1753 100644 >>>> --- a/kernel-shark/src/KsUtils.cpp >>>> +++ b/kernel-shark/src/KsUtils.cpp >>>> @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx) >>>> >>>> auto lamRegUser = [&kshark_ctx](const QString &plugin) >>>> { >>>> - const char *lib = plugin.toStdString().c_str(); >>>> + const char *lib = plugin.toLocal8Bit().data(); >>>> kshark_register_plugin(kshark_ctx, lib); >>>> }; >>>> >>>> @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx) >>>> >>>> auto lamUregUser = [&kshark_ctx](const QString &plugin) >>>> { >>>> - const char *lib = plugin.toStdString().c_str(); >>>> + const char *lib = plugin.toLocal8Bit().data(); >>>> kshark_unregister_plugin(kshark_ctx, lib); >>>> }; >>> >>> Doesn't the new version have the same problem with the temporary QByteArray? >>> >>> How about saving the data in a local std::string/QByteArray variable? >>> >>> std::string lib = plugin.toStdString(); >>> kshark_register_plugin(kshark_ctx, lib.c_str()); >>> >> >> Hi Slavi, >> >> As far I can understand toStdString() will create a fresh std::string >> and this string will has its own copy of the characters. However, this >> copy gets out-of-scope as soon as we hit the semicolon of the line. >> >> My understanding was that toLocal8Bit().data() makes no copy so the >> array will go out-of-scope only when the QString is destroyed. > > I did some digging into QString's implementation. From my reading of > the code, toLocal8Bit() calls into qt_convert_to_latin1()[1] which > does allocation, copying/transform-to-latin1 and returns the > QByteArray. So it seems that toLocal8Bit() is still making a copy. > > [1] https://github.com/qt/qtbase/blob/5.12/src/corelib/tools/qstring.cpp#L5275 > > Cheers, > Thanks a lot for investigating this! I think in this case it will be more appropriate if you sign the patch. cheers, Y. > -- Slavi > >> >> But anyway, your solution looks cleaner and safer. >> I will send another version of the patch. >> Thanks a lot! >> Y. >> >> >>> Thanks! >>> >>> >>> -- >>> Slavomir Kaslev >>> > > >
On March 11, 2019 10:55:57 AM PDT, "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > > >On 11.03.19 г. 15:44 ч., Slavomir Kaslev wrote: >> On Mon, Mar 11, 2019 at 3:20 PM Yordan Karadzhov (VMware) >> <y.karadz@gmail.com> wrote: >>> >>> >>> >>> On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote: >>>> On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov ><ykaradzhov@vmware.com> wrote: >>>>> >>>>> const char *lib = plugin.toStdString().c_str(); >>>>> >>>>> This line is a bad idea because the returned array may (will) be >>>>> invalidated when the destructor of std::string is called. >>>>> >>>>> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com> >>>>> --- >>>>> kernel-shark/src/KsUtils.cpp | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/kernel-shark/src/KsUtils.cpp >b/kernel-shark/src/KsUtils.cpp >>>>> index 34b2e2d..d7b1753 100644 >>>>> --- a/kernel-shark/src/KsUtils.cpp >>>>> +++ b/kernel-shark/src/KsUtils.cpp >>>>> @@ -439,7 +439,7 @@ void >KsPluginManager::registerFromList(kshark_context *kshark_ctx) >>>>> >>>>> auto lamRegUser = [&kshark_ctx](const QString &plugin) >>>>> { >>>>> - const char *lib = plugin.toStdString().c_str(); >>>>> + const char *lib = plugin.toLocal8Bit().data(); >>>>> kshark_register_plugin(kshark_ctx, lib); >>>>> }; >>>>> >>>>> @@ -474,7 +474,7 @@ void >KsPluginManager::unregisterFromList(kshark_context *kshark_ctx) >>>>> >>>>> auto lamUregUser = [&kshark_ctx](const QString &plugin) >>>>> { >>>>> - const char *lib = plugin.toStdString().c_str(); >>>>> + const char *lib = plugin.toLocal8Bit().data(); >>>>> kshark_unregister_plugin(kshark_ctx, lib); >>>>> }; >>>> >>>> Doesn't the new version have the same problem with the temporary >QByteArray? >>>> >>>> How about saving the data in a local std::string/QByteArray >variable? >>>> >>>> std::string lib = plugin.toStdString(); >>>> kshark_register_plugin(kshark_ctx, lib.c_str()); >>>> >>> >>> Hi Slavi, >>> >>> As far I can understand toStdString() will create a fresh >std::string >>> and this string will has its own copy of the characters. However, >this >>> copy gets out-of-scope as soon as we hit the semicolon of the line. >>> >>> My understanding was that toLocal8Bit().data() makes no copy so the >>> array will go out-of-scope only when the QString is destroyed. >> >> I did some digging into QString's implementation. From my reading of >> the code, toLocal8Bit() calls into qt_convert_to_latin1()[1] which >> does allocation, copying/transform-to-latin1 and returns the >> QByteArray. So it seems that toLocal8Bit() is still making a copy. >> >> [1] >https://github.com/qt/qtbase/blob/5.12/src/corelib/tools/qstring.cpp#L5275 >> >> Cheers, >> > >Thanks a lot for investigating this! >I think in this case it will be more appropriate if you sign the patch. >cheers, >Y. > >> -- Slavi >> >>> >>> But anyway, your solution looks cleaner and safer. >>> I will send another version of the patch. >>> Thanks a lot! Signing the patch means that the person either wrote the patch or handled it to be committed. I think you want Suggested-by: -- Steve >>> Y. >>> >>> >>>> Thanks! >>>> >>>> >>>> -- >>>> Slavomir Kaslev >>>> >> >> >>
On Mon, 11 Mar 2019 11:15:40 -0700 Steven Rostedt <rostedt@goodmis.org> wrote: > >Thanks a lot for investigating this! > >I think in this case it will be more appropriate if you sign the patch. > >cheers, > >Y. > > > >> -- Slavi > >> > >>> > >>> But anyway, your solution looks cleaner and safer. > >>> I will send another version of the patch. > >>> Thanks a lot! > > > Signing the patch means that the person either wrote the patch or handled it to be committed. > > I think you want Suggested-by: > BTW, I'm confused. Does this patch need to be applied or was there another version coming? -- Steve
On 26.03.19 г. 3:58 ч., Steven Rostedt wrote: > On Mon, 11 Mar 2019 11:15:40 -0700 > Steven Rostedt <rostedt@goodmis.org> wrote: > >>> Thanks a lot for investigating this! >>> I think in this case it will be more appropriate if you sign the patch. >>> cheers, >>> Y. >>> >>>> -- Slavi >>>> >>>>> >>>>> But anyway, your solution looks cleaner and safer. >>>>> I will send another version of the patch. >>>>> Thanks a lot! >> >> >> Signing the patch means that the person either wrote the patch or handled it to be committed. >> >> I think you want Suggested-by: >> > > BTW, I'm confused. Does this patch need to be applied or was there another version coming? > It is my fault. Looks like I forgot to send v3. The patch is coming. Thanks! Y. > -- Steve >
diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp index 34b2e2d..d7b1753 100644 --- a/kernel-shark/src/KsUtils.cpp +++ b/kernel-shark/src/KsUtils.cpp @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx) auto lamRegUser = [&kshark_ctx](const QString &plugin) { - const char *lib = plugin.toStdString().c_str(); + const char *lib = plugin.toLocal8Bit().data(); kshark_register_plugin(kshark_ctx, lib); }; @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx) auto lamUregUser = [&kshark_ctx](const QString &plugin) { - const char *lib = plugin.toStdString().c_str(); + const char *lib = plugin.toLocal8Bit().data(); kshark_unregister_plugin(kshark_ctx, lib); };
const char *lib = plugin.toStdString().c_str(); This line is a bad idea because the returned array may (will) be invalidated when the destructor of std::string is called. Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com> --- kernel-shark/src/KsUtils.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)