Re: Review Request 127770: Increase maximum string length in KSycoca database

[Jos van den Oever]

> On apr 28, 2016, 9:12 a.m., Milian Wolff wrote:
> > src/sycoca/ksycocautils.cpp, line 29
> > 
> >
> > as a follow-up cleanup I suggest to introduce an enum to hold this 
> > magic value

I can do that if the code stays. I made another rr to remove it: 
https://git.reviewboard.kde.org/r/127786/


- Jos


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/#review94946
---


On apr 28, 2016, 7:01 a.m., Jos van den Oever wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127770/
> ---
> 
> (Updated apr 28, 2016, 7:01 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> The code for reading and writing of strings in KSycoca is not symmetrical. 
> Strings of any length can be written, but only strings of less than 8192 
> bytes may be read. This limit is set in KSycocaUtilsPrivate::read. The limit 
> is probably there to avoid out-of-memory situations.
> 
> On my system I have a lot of XDG data dirs. The length of the environment 
> variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which 
> needs 8184 bytes. KBuildSycoca save that without complaint but complains when 
> reading it.
> 
> 
> The simplest solution here is to simply increase the magic number 8192 to 
> e.g. 65528. This is just a temporary buffer.
> 
> Or we just check the size of the whole cache file (e.g. < 100M) and remove 
> all other limits. That would simplify
> 
>KSycocaUtilsPrivate::read(*str, header.prefixes);
> 
> to
> 
>*str >> header.prefixes;
> 
> This patch chooses the first option.
> 
> 
> Diffs
> -
> 
>   src/sycoca/ksycocautils.cpp 1ba75e8 
> 
> Diff: https://git.reviewboard.kde.org/r/127770/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests on KService. All but one of the previously failing tests on 
> NixOS is now fixed.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127770: Increase maximum string length in KSycoca database

[Milian Wolff]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/#review94946
---




src/sycoca/ksycocautils.cpp (line 29)


as a follow-up cleanup I suggest to introduce an enum to hold this magic 
value


- Milian Wolff


On April 28, 2016, 7:01 a.m., Jos van den Oever wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127770/
> ---
> 
> (Updated April 28, 2016, 7:01 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> The code for reading and writing of strings in KSycoca is not symmetrical. 
> Strings of any length can be written, but only strings of less than 8192 
> bytes may be read. This limit is set in KSycocaUtilsPrivate::read. The limit 
> is probably there to avoid out-of-memory situations.
> 
> On my system I have a lot of XDG data dirs. The length of the environment 
> variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which 
> needs 8184 bytes. KBuildSycoca save that without complaint but complains when 
> reading it.
> 
> 
> The simplest solution here is to simply increase the magic number 8192 to 
> e.g. 65528. This is just a temporary buffer.
> 
> Or we just check the size of the whole cache file (e.g. < 100M) and remove 
> all other limits. That would simplify
> 
>KSycocaUtilsPrivate::read(*str, header.prefixes);
> 
> to
> 
>*str >> header.prefixes;
> 
> This patch chooses the first option.
> 
> 
> Diffs
> -
> 
>   src/sycoca/ksycocautils.cpp 1ba75e8 
> 
> Diff: https://git.reviewboard.kde.org/r/127770/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests on KService. All but one of the previously failing tests on 
> NixOS is now fixed.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127770: Increase maximum string length in KSycoca database

[Jos van den Oever]

> On apr 27, 2016, 8:36 p.m., David Faure wrote:
> > src/sycoca/ksycocautils.cpp, line 39
> > 
> >
> > Variable-length arrays are invalid C++.
> > 
> > gcc might accept it, but -pedantic won't, and other compilers might not.
> > 
> > See e.g. 
> > http://stackoverflow.com/questions/1887097/variable-length-arrays-in-c 
> > (which is about C++, the '+'s got removed from the URL)
> > 
> > You can use an enum value to factorize the 8192. I think "static const 
> > int" won't do, if I read http://en.cppreference.com/w/cpp/language/array 
> > correctly.
> 
> Albert Astals Cid wrote:
> I'm not sure if it's up to standard or not but both g++ and clang++ 
> compile https://paste.kde.org/pfrytc0no with -pendantic
> 
> I wouldn't call it "variable-length" since the length is known at compile 
> time.
> 
> Also 
> http://stackoverflow.com/questions/1327806/do-all-c-compilers-allow-using-a-static-const-int-class-member-variable-as-an
>  seems to say it's fine.

i chose to keep it simple and stick to literals


> On apr 27, 2016, 8:36 p.m., David Faure wrote:
> > src/sycoca/ksycocautils.cpp, line 27
> > 
> >
> > static ? But see next item.

i chose to keep it simple and stick to literals


- Jos


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/#review94914
---


On apr 27, 2016, 8:44 p.m., Jos van den Oever wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127770/
> ---
> 
> (Updated apr 27, 2016, 8:44 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> The code for reading and writing of strings in KSycoca is not symmetrical. 
> Strings of any length can be written, but only strings of less than 8192 
> bytes may be read. This limit is set in KSycocaUtilsPrivate::read. The limit 
> is probably there to avoid out-of-memory situations.
> 
> On my system I have a lot of XDG data dirs. The length of the environment 
> variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which 
> needs 8184 bytes. KBuildSycoca save that without complaint but complains when 
> reading it.
> 
> 
> The simplest solution here is to simply increase the magic number 8192 to 
> e.g. 65528. This is just a temporary buffer.
> 
> Or we just check the size of the whole cache file (e.g. < 100M) and remove 
> all other limits. That would simplify
> 
>KSycocaUtilsPrivate::read(*str, header.prefixes);
> 
> to
> 
>*str >> header.prefixes;
> 
> This patch chooses the first option.
> 
> 
> Diffs
> -
> 
>   src/sycoca/ksycocautils.cpp 1ba75e8 
> 
> Diff: https://git.reviewboard.kde.org/r/127770/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests on KService. All but one of the previously failing tests on 
> NixOS is now fixed.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127770: Increase maximum string length in KSycoca database

[Albert Astals Cid]

> On April 27, 2016, 8:36 p.m., David Faure wrote:
> > src/sycoca/ksycocautils.cpp, line 39
> > 
> >
> > Variable-length arrays are invalid C++.
> > 
> > gcc might accept it, but -pedantic won't, and other compilers might not.
> > 
> > See e.g. 
> > http://stackoverflow.com/questions/1887097/variable-length-arrays-in-c 
> > (which is about C++, the '+'s got removed from the URL)
> > 
> > You can use an enum value to factorize the 8192. I think "static const 
> > int" won't do, if I read http://en.cppreference.com/w/cpp/language/array 
> > correctly.

I'm not sure if it's up to standard or not but both g++ and clang++ compile 
https://paste.kde.org/pfrytc0no with -pendantic

I wouldn't call it "variable-length" since the length is known at compile time.

Also 
http://stackoverflow.com/questions/1327806/do-all-c-compilers-allow-using-a-static-const-int-class-member-variable-as-an
 seems to say it's fine.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/#review94914
---


On April 27, 2016, 8:44 p.m., Jos van den Oever wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127770/
> ---
> 
> (Updated April 27, 2016, 8:44 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> The code for reading and writing of strings in KSycoca is not symmetrical. 
> Strings of any length can be written, but only strings of less than 8192 
> bytes may be read. This limit is set in KSycocaUtilsPrivate::read. The limit 
> is probably there to avoid out-of-memory situations.
> 
> On my system I have a lot of XDG data dirs. The length of the environment 
> variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which 
> needs 8184 bytes. KBuildSycoca save that without complaint but complains when 
> reading it.
> 
> 
> The simplest solution here is to simply increase the magic number 8192 to 
> e.g. 65528. This is just a temporary buffer.
> 
> Or we just check the size of the whole cache file (e.g. < 100M) and remove 
> all other limits. That would simplify
> 
>KSycocaUtilsPrivate::read(*str, header.prefixes);
> 
> to
> 
>*str >> header.prefixes;
> 
> This patch chooses the first option.
> 
> 
> Diffs
> -
> 
>   src/sycoca/ksycocautils.cpp 1ba75e8 
> 
> Diff: https://git.reviewboard.kde.org/r/127770/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests on KService. All but one of the previously failing tests on 
> NixOS is now fixed.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127770: Increase maximum string length in KSycoca database

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/
---

(Updated apr 27, 2016, 8:44 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Use literals instead of variable to be on the safe side wrt compiler 
compatibility.


Repository: kservice


Description
---

The code for reading and writing of strings in KSycoca is not symmetrical. 
Strings of any length can be written, but only strings of less than 8192 bytes 
may be read. This limit is set in KSycocaUtilsPrivate::read. The limit is 
probably there to avoid out-of-memory situations.

On my system I have a lot of XDG data dirs. The length of the environment 
variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which needs 
8184 bytes. KBuildSycoca save that without complaint but complains when reading 
it.


The simplest solution here is to simply increase the magic number 8192 to e.g. 
65528. This is just a temporary buffer.

Or we just check the size of the whole cache file (e.g. < 100M) and remove all 
other limits. That would simplify

   KSycocaUtilsPrivate::read(*str, header.prefixes);

to

   *str >> header.prefixes;

This patch chooses the first option.


Diffs (updated)
-

  src/sycoca/ksycocautils.cpp 1ba75e8 

Diff: https://git.reviewboard.kde.org/r/127770/diff/


Testing
---

Ran unit tests on KService. All but one of the previously failing tests on 
NixOS is now fixed.


Thanks,

Jos van den Oever

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127770: Increase maximum string length in KSycoca database

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/#review94914
---



Very good find, thanks!


src/sycoca/ksycocautils.cpp (line 27)


static ? But see next item.



src/sycoca/ksycocautils.cpp (line 39)


Variable-length arrays are invalid C++.

gcc might accept it, but -pedantic won't, and other compilers might not.

See e.g. 
http://stackoverflow.com/questions/1887097/variable-length-arrays-in-c (which 
is about C++, the '+'s got removed from the URL)

You can use an enum value to factorize the 8192. I think "static const int" 
won't do, if I read http://en.cppreference.com/w/cpp/language/array correctly.


- David Faure


On April 27, 2016, 8 p.m., Jos van den Oever wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127770/
> ---
> 
> (Updated April 27, 2016, 8 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> The code for reading and writing of strings in KSycoca is not symmetrical. 
> Strings of any length can be written, but only strings of less than 8192 
> bytes may be read. This limit is set in KSycocaUtilsPrivate::read. The limit 
> is probably there to avoid out-of-memory situations.
> 
> On my system I have a lot of XDG data dirs. The length of the environment 
> variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which 
> needs 8184 bytes. KBuildSycoca save that without complaint but complains when 
> reading it.
> 
> 
> The simplest solution here is to simply increase the magic number 8192 to 
> e.g. 65528. This is just a temporary buffer.
> 
> Or we just check the size of the whole cache file (e.g. < 100M) and remove 
> all other limits. That would simplify
> 
>KSycocaUtilsPrivate::read(*str, header.prefixes);
> 
> to
> 
>*str >> header.prefixes;
> 
> This patch chooses the first option.
> 
> 
> Diffs
> -
> 
>   src/sycoca/ksycocautils.cpp 1ba75e8 
> 
> Diff: https://git.reviewboard.kde.org/r/127770/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests on KService. All but one of the previously failing tests on 
> NixOS is now fixed.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127770: Increase maximum string length in KSycoca database

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/
---

Review request for KDE Frameworks and David Faure.


Repository: kservice


Description
---

The code for reading and writing of strings in KSycoca is not symmetrical. 
Strings of any length can be written, but only strings of less than 8192 bytes 
may be read. This limit is set in KSycocaUtilsPrivate::read. The limit is 
probably there to avoid out-of-memory situations.

On my system I have a lot of XDG data dirs. The length of the environment 
variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which needs 
8184 bytes. KBuildSycoca save that without complaint but complains when reading 
it.


The simplest solution here is to simply increase the magic number 8192 to e.g. 
65528. This is just a temporary buffer.

Or we just check the size of the whole cache file (e.g. < 100M) and remove all 
other limits. That would simplify

   KSycocaUtilsPrivate::read(*str, header.prefixes);

to

   *str >> header.prefixes;

This patch chooses the first option.


Diffs
-

  src/sycoca/ksycocautils.cpp 1ba75e8 

Diff: https://git.reviewboard.kde.org/r/127770/diff/


Testing
---

Ran unit tests on KService. All but one of the previously failing tests on 
NixOS is now fixed.


Thanks,

Jos van den Oever

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel