Re: [osv-dev] [PATCH] scripts: upgrade to python 3 - part 1

[Waldek Kozaczuk]

So I think I have a small subset of this patch ready that encompassed 
everything from the original path but loader.py and trace related files.

But I just realized a problem with updates setup.py. It now depends on 
distro package - https://pypi.org/project/distro/ - which requires pip to 
install. Which means we now require python3-pip to be installed first 
before running setup.py. Chicken or egg kind of problem. This is not an 
issue with docker as we simply have docker install it first. How do we 
solve it in non-docker setup? 

Ideas?
Waldek

On Tuesday, February 18, 2020 at 12:04:36 PM UTC-5, Waldek Kozaczuk wrote:
>
> Let me split this patch into some smaller ones and take your approach of 
> first checking if anything breaks with python3.
>
> I think that loader.py needs to be treated separately. Is it true that gdb 
> comes in with its own python runtime and these days it should be the 
> version 3 (we are not relying on externals anymore, right)? The problem 
> with loader.py is that it relies in some places on tracing related scripts:
>
> from osv.trace import (Trace, Thread, TracePoint, BacktraceFormatter,
> from osv import trace, debug
>
> Does it mean we want to make those trace related scripts be 2 and 3 
> compatible?
>
> On Tuesday, February 18, 2020 at 11:53:19 AM UTC-5, Nadav Har'El wrote:
>
>>
>> On Tue, Feb 18, 2020 at 3:54 PM Waldek Kozaczuk  
>> wrote:
>>
>> This - 
>> https://stackoverflow.com/questions/27476079/why-does-2to3-change-mydict-keys-to-listmydict-keys
>>  - 
>> might explain this "list" related changes by 2to3. 
>>
>>
>> Yet it doesn't explain the change - it explains why it is *not* actually 
>> needed ;-)
>> I know why it's needed in some specific cases where a list is required 
>> and .keys() is no longer a list. But  it's not needed here (in a for loop) 
>> and in most other locations where this list() was added :-(
>>
>>
>> On Tuesday, February 18, 2020 at 8:42:47 AM UTC-5, Waldek Kozaczuk wrote:
>>
>> Hey,
>>
>> Thanks for the review.
>>
>> Let me first explain what my process was. At first, I just tried to 
>> submit Matt's changes with proper signoff. But then I decided to test it a 
>> bit and discovered some things were breaking. Now I do not think it was 
>> because of any problems in his original script, but mostly due to the fact 
>> that his changes were intertwined with other scripts and I had to change 
>> those. Also, pretty modules/openjdk8-from-host/module.py (was not there 
>> when Matt was changing) did not work And finally trace.py somehow came 
>> along. So here we go.
>>
>> As far as mechanics goes, I retained most of the Matt's patch as is and I 
>> believe he used the "Future" module. I used the 2to3 tool (more 
>> specifically 2to3-2.7 in my case) first and then manually fixed any 
>> problems I discovered. Most painful to fix was trace.py and all related 
>> ones (scripts/osv/*.py) which I tested quite thoroughly by running trace.py 
>> (I learned more about it which is great but probably way too much than what 
>> I wanted to know at this point ;-)). I also tested all other tests more 
>> implicitly by running scripts/build with some variations of image= (java is 
>> good as it exercises a lot) and fs=.
>>
>> But you are right that I could not thoroughly test loader.py (did many 
>> commands but not all). Shall we exclude it from this patch?
>>
>> Lastly, I am probably not the right person to do this upgrade exercise. I 
>> do not use Python daily so I am not an expert. Lack of compiler (aka 
>> 'complainer') did not make me very confident especially with those larger 
>> scripts. But I did not want Matt's work to go to waste. So here we go :-)
>>
>> On Tuesday, February 18, 2020 at 5:34:58 AM UTC-5, Nadav Har'El wrote:
>>
>> Thanks. I commented with a few concerns below. I'm only really worried 
>> about loader.py - the gdb script, which was already supposed to be working 
>> for both Python 2 and Python 3 (although we probably didn't test it 
>> recently), and I'm worried these changes are breaking it, rather than 
>> improving it - and it's difficult to test because these changes change all 
>> sorts of "osv" commands in gdb which I guess you didn't test individually.
>>
>> Shall we leave it out? 
>>
>>
>> I ask that you please at least try to run the affected scripts in Python3 
>> before "fixing" them at all. In particular, some scripts that had 
>> "/usr/bin/python" at the top (and not /usr/bin/python2) at the top already 
>> worked correctly for Python 3 (and Python 2) because some people already 
>> had python 3 as their default python (although, it is quite likely that we 
>> haven't tested this in a while so things broke).
>>
>> In most cases, I first ran 2to3 and then applied manual changes if I 
>> found any problems (like related to bytes vs str type of issue - really 
>> painful to find and fix). Indeed 2to3 is somewhat suspicious as it 
>> sometimes put extra parentheses when there were already (I manually removed 
>> those 

[osv-dev] Re: Virtio-FS

[Fotis Xenakis]

Indeed, in hindsight, I was as good as vanished. I will make sure to be 
more active on the mailing list from now on! Thank you for the 
clarifications on the communication channels.

Τη Τρίτη, 18 Φεβρουαρίου 2020 - 6:40:33 π.μ. UTC+2, ο χρήστης Waldek 
Kozaczuk έγραψε:
>
> Hi,
>
> Good to hear from you!
>
> On Saturday, February 15, 2020 at 5:53:58 PM UTC-5, Fotis Xenakis wrote:
>>
>> I saw this initial patch, but was short on time to reply until now.
>>
>> I am still interested in this and keep pursuing it! I was also working on 
>> adding the initial support (had a working driver but nothing on the fs 
>> side), so I will rebase on your work and continue from there. A question 
>> here is what's the best way to avoid parallel work in the future? May I be 
>> missing something apart from getting the mails from this list and notifying 
>> here when working on something?
>>
> I have not heard from you in a while and you never acknowledged back at 
> the beginning of the year that you decided to pursue it. So I assumed to 
> you moved on to something else :-) Meanwhile, I also got interested in 
> Virtio_FS and started to implement it a couple of weeks ago. I could have 
> also mentioned that on the mailing list. So the mailing list is also the 
> best communication channel. Also, it would not hurt to possibly assign 
> yourself to the relevant github issue. All in all, we are collaborating, so 
> everything is fine. But you are right we should not duplicate our efforts. 
> The issue is all yours now. Relatedly it would be best if you can send us 
> small patches solving remaining individual problems listed in the github 
> issues, instead of lumping everything into a single patch. It will help us 
> reviewing. Thanks.
>
>>
>> I agree with you that DAX seems the most interesting feature to add at 
>> this point, so this is what I will be focusing on for now. I have started 
>> with some more generic reading on it and will check out the Linux driver 
>> implementation to come up with how it could possibly be implemented in OSv. 
>> When I have something more concrete (hopefully some point in the next 
>> couple of weeks) I will share it here, your feedback will undoubtedly be 
>> necessary.
>>
> Cool. Looking forward to it. 
>
>>
>> Fotis
>>
>> Τη Τρίτη, 11 Φεβρουαρίου 2020 - 1:21:13 π.μ. UTC+2, ο χρήστης Waldek 
>> Kozaczuk έγραψε:
>>>
>>> We have just pushed initial implementation of Virtio-FS with this commit 
>>> - 
>>> https://github.com/cloudius-systems/osv/commit/bae4381d1d0558b7a684294e9203864f9652395c.
>>>  
>>> This is just a start but enough to for example run simple app from a 
>>> directory on Linux host mounted through virtio-FS. 
>>>
>>> There are still many outstanding items which I listed here - 
>>> https://github.com/cloudius-systems/osv/issues/1062#issuecomment-584400796 
>>> so you if are still interested you can help us implement those. The most 
>>> interesting and challenging one is the DAX window which allows directly map 
>>> a file from host into guest virtual address space. I am not sure how to 
>>> exactly accomplish this in OSv but I guess we would need to create a 
>>> mechanism to create a VMA where virtual address range on OSv is mapped to 
>>> some physical memory range passed by fuse requests (just my wild guess). 
>>>
>>> Waldek
>>>
>>> On Sunday, January 5, 2020 at 1:11:23 AM UTC-5, Waldek Kozaczuk wrote:

 It looks like virtio-fs driver would communicate with the hypervisor 
 (device) using FUSE protocol. So we would need to incorporate this header 
 - 
 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fuse.h
 .

 Per virtio spec (
 https://stefanha.github.io/virtio/virtio-fs.html#x1-41500011), the 
 virtio-fs driver could handle the logic of processing the virtio_fs_req 
 requests but the vfs layer would know how to construct specific requests 
 like virtio_fs_read_req or something like this.

 On Sunday, January 5, 2020 at 12:30:20 AM UTC-5, Waldek Kozaczuk wrote:
>
> Hi,
>
> On Saturday, January 4, 2020 at 11:08:29 AM UTC-5, 
> fo...@windowslive.com wrote:
>>
>> Hello,
>>
>> I am Fotis, currently an undergraduate ECE student in Athens, Greece.
>>
>> I 've been looking into Virtio-FS and adding support for it in OSv 
>> seems pretty interesting. I have read the documentation in the wiki as 
>> well 
>> as the documentation directory in the repo and I was starting to read 
>> the 
>> code for existing filesystems and virtio devices. I 'd like to ask 
>> though 
>> if there is any other relevant documentation (or code) you think would 
>> be 
>> helpful.
>>
>> Also, in addition to the requirements mentioned in the GitHub issue 
>> , are you aware 
>> of any other major blockers that could surface? How difficult and time 
>> 

[osv-dev] Re: Virtio-FS

[Fotis Xenakis]

Thank you! Examining the patch and the relevant discussion reveals some 
useful paths in the code (especially around the implementation of mmap). 
Also, it's a good example of code interacting with the MMU, which will be 
necessary for DAX also, but indeed this optimization in ROFS's page cache 
is different. DAX in virtio-fs roughly constitutes of creating a mapping 
from the virtual address space to addresses corresponding to the device 
(that's where the MMU will come into play of course). The first step 
towards that is of course the negotiation with the device (this is done 
with a custom FUSE request). There is also a PCI-specific part that I have 
to read a bit more to clarify. Once I have a clear overview of the whole 
mechanism, I might start a new discussion for it.

Τη Τρίτη, 18 Φεβρουαρίου 2020 - 6:46:53 π.μ. UTC+2, ο χρήστης Waldek 
Kozaczuk έγραψε:
>
> BTW this old (never applied) work-in-progress patch might point you in the 
> right direction - 
> https://groups.google.com/d/msg/osv-dev/GbNzNONJ-Vw/2WzdsuoRAwAJ. But I 
> am not sure of it as DAX is different from what I was trying to do for 
> Read-Only FS where I was trying to create a mapping to memory in the guest. 
>
> On Monday, February 17, 2020 at 11:40:33 PM UTC-5, Waldek Kozaczuk wrote:
>>
>> Hi,
>>
>> Good to hear from you!
>>
>> On Saturday, February 15, 2020 at 5:53:58 PM UTC-5, Fotis Xenakis wrote:
>>>
>>> I saw this initial patch, but was short on time to reply until now.
>>>
>>> I am still interested in this and keep pursuing it! I was also working 
>>> on adding the initial support (had a working driver but nothing on the fs 
>>> side), so I will rebase on your work and continue from there. A question 
>>> here is what's the best way to avoid parallel work in the future? May I be 
>>> missing something apart from getting the mails from this list and notifying 
>>> here when working on something?
>>>
>> I have not heard from you in a while and you never acknowledged back at 
>> the beginning of the year that you decided to pursue it. So I assumed to 
>> you moved on to something else :-) Meanwhile, I also got interested in 
>> Virtio_FS and started to implement it a couple of weeks ago. I could have 
>> also mentioned that on the mailing list. So the mailing list is also the 
>> best communication channel. Also, it would not hurt to possibly assign 
>> yourself to the relevant github issue. All in all, we are collaborating, so 
>> everything is fine. But you are right we should not duplicate our efforts. 
>> The issue is all yours now. Relatedly it would be best if you can send us 
>> small patches solving remaining individual problems listed in the github 
>> issues, instead of lumping everything into a single patch. It will help us 
>> reviewing. Thanks.
>>
>>>
>>> I agree with you that DAX seems the most interesting feature to add at 
>>> this point, so this is what I will be focusing on for now. I have started 
>>> with some more generic reading on it and will check out the Linux driver 
>>> implementation to come up with how it could possibly be implemented in OSv. 
>>> When I have something more concrete (hopefully some point in the next 
>>> couple of weeks) I will share it here, your feedback will undoubtedly be 
>>> necessary.
>>>
>> Cool. Looking forward to it. 
>>
>>>
>>> Fotis
>>>
>>> Τη Τρίτη, 11 Φεβρουαρίου 2020 - 1:21:13 π.μ. UTC+2, ο χρήστης Waldek 
>>> Kozaczuk έγραψε:

 We have just pushed initial implementation of Virtio-FS with this 
 commit - 
 https://github.com/cloudius-systems/osv/commit/bae4381d1d0558b7a684294e9203864f9652395c.
  
 This is just a start but enough to for example run simple app from a 
 directory on Linux host mounted through virtio-FS. 

 There are still many outstanding items which I listed here - 
 https://github.com/cloudius-systems/osv/issues/1062#issuecomment-584400796 
 so you if are still interested you can help us implement those. The most 
 interesting and challenging one is the DAX window which allows directly 
 map 
 a file from host into guest virtual address space. I am not sure how to 
 exactly accomplish this in OSv but I guess we would need to create a 
 mechanism to create a VMA where virtual address range on OSv is mapped to 
 some physical memory range passed by fuse requests (just my wild guess). 

 Waldek

 On Sunday, January 5, 2020 at 1:11:23 AM UTC-5, Waldek Kozaczuk wrote:
>
> It looks like virtio-fs driver would communicate with the hypervisor 
> (device) using FUSE protocol. So we would need to incorporate this header 
> - 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fuse.h
> .
>
> Per virtio spec (
> https://stefanha.github.io/virtio/virtio-fs.html#x1-41500011), the 
> virtio-fs driver could handle the logic of processing the virtio_fs_req 
> requests but the vfs layer would know how 

Re: [osv-dev] [PATCH] scripts: upgrade to python 3 - part 1

[Waldek Kozaczuk]

Let me split this patch into some smaller ones and take your approach of 
first checking if anything breaks with python3.

I think that loader.py needs to be treated separately. Is it true that gdb 
comes in with its own python runtime and these days it should be the 
version 3 (we are not relying on externals anymore, right)? The problem 
with loader.py is that it relies in some places on tracing related scripts:

from osv.trace import (Trace, Thread, TracePoint, BacktraceFormatter,
from osv import trace, debug

Does it mean we want to make those trace related scripts be 2 and 3 
compatible?

On Tuesday, February 18, 2020 at 11:53:19 AM UTC-5, Nadav Har'El wrote:

>
> On Tue, Feb 18, 2020 at 3:54 PM Waldek Kozaczuk  > wrote:
>
> This - 
> https://stackoverflow.com/questions/27476079/why-does-2to3-change-mydict-keys-to-listmydict-keys
>  - 
> might explain this "list" related changes by 2to3. 
>
>
> Yet it doesn't explain the change - it explains why it is *not* actually 
> needed ;-)
> I know why it's needed in some specific cases where a list is required and 
> .keys() is no longer a list. But  it's not needed here (in a for loop) and 
> in most other locations where this list() was added :-(
>
>
> On Tuesday, February 18, 2020 at 8:42:47 AM UTC-5, Waldek Kozaczuk wrote:
>
> Hey,
>
> Thanks for the review.
>
> Let me first explain what my process was. At first, I just tried to submit 
> Matt's changes with proper signoff. But then I decided to test it a bit and 
> discovered some things were breaking. Now I do not think it was because of 
> any problems in his original script, but mostly due to the fact that his 
> changes were intertwined with other scripts and I had to change those. 
> Also, pretty modules/openjdk8-from-host/module.py (was not there when Matt 
> was changing) did not work And finally trace.py somehow came along. So here 
> we go.
>
> As far as mechanics goes, I retained most of the Matt's patch as is and I 
> believe he used the "Future" module. I used the 2to3 tool (more 
> specifically 2to3-2.7 in my case) first and then manually fixed any 
> problems I discovered. Most painful to fix was trace.py and all related 
> ones (scripts/osv/*.py) which I tested quite thoroughly by running trace.py 
> (I learned more about it which is great but probably way too much than what 
> I wanted to know at this point ;-)). I also tested all other tests more 
> implicitly by running scripts/build with some variations of image= (java is 
> good as it exercises a lot) and fs=.
>
> But you are right that I could not thoroughly test loader.py (did many 
> commands but not all). Shall we exclude it from this patch?
>
> Lastly, I am probably not the right person to do this upgrade exercise. I 
> do not use Python daily so I am not an expert. Lack of compiler (aka 
> 'complainer') did not make me very confident especially with those larger 
> scripts. But I did not want Matt's work to go to waste. So here we go :-)
>
> On Tuesday, February 18, 2020 at 5:34:58 AM UTC-5, Nadav Har'El wrote:
>
> Thanks. I commented with a few concerns below. I'm only really worried 
> about loader.py - the gdb script, which was already supposed to be working 
> for both Python 2 and Python 3 (although we probably didn't test it 
> recently), and I'm worried these changes are breaking it, rather than 
> improving it - and it's difficult to test because these changes change all 
> sorts of "osv" commands in gdb which I guess you didn't test individually.
>
> Shall we leave it out? 
>
>
> I ask that you please at least try to run the affected scripts in Python3 
> before "fixing" them at all. In particular, some scripts that had 
> "/usr/bin/python" at the top (and not /usr/bin/python2) at the top already 
> worked correctly for Python 3 (and Python 2) because some people already 
> had python 3 as their default python (although, it is quite likely that we 
> haven't tested this in a while so things broke).
>
> In most cases, I first ran 2to3 and then applied manual changes if I found 
> any problems (like related to bytes vs str type of issue - really painful 
> to find and fix). Indeed 2to3 is somewhat suspicious as it sometimes put 
> extra parentheses when there were already (I manually removed those 
> changes). Not sure about list-like related changes - all done by 2to3.
>
>
> --
> Nadav Har'El
> n...@scylladb.com
>
>
> On Mon, Feb 17, 2020 at 8:22 AM Waldemar Kozaczuk  
> wrote:
>
> --- a/scripts/loader.py
> +++ b/scripts/loader.py
>
>
> Please make sure to test this file a bit, because it isn't part of the 
> standard build or run.
> This is the script loaded automatically when you use gdb OSv.
>
> Also, I was always under the impression that this script already worked on 
> both Python 2 and
> Python 3, because we had no idea what gdb runs for us (the "#!" in the top 
> of this script isn't relevant, right?)?
>
> Shall we change at least this '#!/usr/bin/python2' to '#!/usr/bin/python' 
> then?
>  
>
> @@ -1,4 +1,4 @@
> 

Re: [osv-dev] [PATCH] scripts: upgrade to python 3 - part 1

[Nadav Har'El]

On Tue, Feb 18, 2020 at 3:54 PM Waldek Kozaczuk 
wrote:

> This -
> https://stackoverflow.com/questions/27476079/why-does-2to3-change-mydict-keys-to-listmydict-keys
>  -
> might explain this "list" related changes by 2to3.
>

Yet it doesn't explain the change - it explains why it is *not* actually
needed ;-)
I know why it's needed in some specific cases where a list is required and
.keys() is no longer a list. But  it's not needed here (in a for loop) and
in most other locations where this list() was added :-(


> On Tuesday, February 18, 2020 at 8:42:47 AM UTC-5, Waldek Kozaczuk wrote:
>>
>> Hey,
>>
>> Thanks for the review.
>>
>> Let me first explain what my process was. At first, I just tried to
>> submit Matt's changes with proper signoff. But then I decided to test it a
>> bit and discovered some things were breaking. Now I do not think it was
>> because of any problems in his original script, but mostly due to the fact
>> that his changes were intertwined with other scripts and I had to change
>> those. Also, pretty modules/openjdk8-from-host/module.py (was not there
>> when Matt was changing) did not work And finally trace.py somehow came
>> along. So here we go.
>>
>> As far as mechanics goes, I retained most of the Matt's patch as is and I
>> believe he used the "Future" module. I used the 2to3 tool (more
>> specifically 2to3-2.7 in my case) first and then manually fixed any
>> problems I discovered. Most painful to fix was trace.py and all related
>> ones (scripts/osv/*.py) which I tested quite thoroughly by running trace.py
>> (I learned more about it which is great but probably way too much than what
>> I wanted to know at this point ;-)). I also tested all other tests more
>> implicitly by running scripts/build with some variations of image= (java is
>> good as it exercises a lot) and fs=.
>>
>> But you are right that I could not thoroughly test loader.py (did many
>> commands but not all). Shall we exclude it from this patch?
>>
>> Lastly, I am probably not the right person to do this upgrade exercise. I
>> do not use Python daily so I am not an expert. Lack of compiler (aka
>> 'complainer') did not make me very confident especially with those larger
>> scripts. But I did not want Matt's work to go to waste. So here we go :-)
>>
>> On Tuesday, February 18, 2020 at 5:34:58 AM UTC-5, Nadav Har'El wrote:
>>
>> Thanks. I commented with a few concerns below. I'm only really worried
>> about loader.py - the gdb script, which was already supposed to be working
>> for both Python 2 and Python 3 (although we probably didn't test it
>> recently), and I'm worried these changes are breaking it, rather than
>> improving it - and it's difficult to test because these changes change all
>> sorts of "osv" commands in gdb which I guess you didn't test individually.
>>
>> Shall we leave it out?
>>
>>
>> I ask that you please at least try to run the affected scripts in Python3
>> before "fixing" them at all. In particular, some scripts that had
>> "/usr/bin/python" at the top (and not /usr/bin/python2) at the top already
>> worked correctly for Python 3 (and Python 2) because some people already
>> had python 3 as their default python (although, it is quite likely that we
>> haven't tested this in a while so things broke).
>>
>> In most cases, I first ran 2to3 and then applied manual changes if I
>> found any problems (like related to bytes vs str type of issue - really
>> painful to find and fix). Indeed 2to3 is somewhat suspicious as it
>> sometimes put extra parentheses when there were already (I manually removed
>> those changes). Not sure about list-like related changes - all done by 2to3.
>>
>>
>> --
>> Nadav Har'El
>> n...@scylladb.com
>>
>>
>> On Mon, Feb 17, 2020 at 8:22 AM Waldemar Kozaczuk 
>> wrote:
>>
>> --- a/scripts/loader.py
>> +++ b/scripts/loader.py
>>
>>
>> Please make sure to test this file a bit, because it isn't part of the
>> standard build or run.
>> This is the script loaded automatically when you use gdb OSv.
>>
>> Also, I was always under the impression that this script already worked
>> on both Python 2 and
>> Python 3, because we had no idea what gdb runs for us (the "#!" in the
>> top of this script isn't relevant, right?)?
>>
>> Shall we change at least this '#!/usr/bin/python2' to '#!/usr/bin/python'
>> then?
>>
>>
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python2
>> +#!/usr/bin/python3
>>
>>  import gdb
>>  import re
>> @@ -37,8 +37,8 @@ def phys_cast(addr, type):
>>
>>  def values(_dict):
>>  if hasattr(_dict, 'viewvalues'):
>> -return _dict.viewvalues()
>> -return _dict.values()
>> +return _dict.values()
>>
>>
>> This doesn't look right - you check if there is a viewvalues() function
>> and then call values()?
>> Maybe the whole "hasattr" check isn't needed on Python 2 any more?
>>
>> +return list(_dict.values())
>>
>>
>> I wonder why you need to convert it to a list - what's wrong with a
>> generator which isn't officially a list? Did this solve a real 

Re: [osv-dev] [PATCH] httpserver: properly handle chunked POST requests with body

[Waldek Kozaczuk]

On Tuesday, February 18, 2020 at 10:12:56 AM UTC-5, Nadav Har'El wrote:
>
>
> On Mon, Feb 17, 2020 at 11:58 PM Waldemar Kozaczuk  > wrote:
>
>> This bug was discovered when running httpserver unit tests with python
>> scripts upgraded to version 3. The new version of the requests module 
>> chunks
>> POST requests with body so that they are sent over socket in two parts - 
>> the request
>> and the body.
>>
>> Our httpserver had a bug in how it consumed such requests. Instead of
>> completing the request once the body chunk was fully received,
>> it would try to re-consume the same body chunk as next request and after
>> failing to, it would send back a 400 (bad request) response.
>>
>> So this patch simply changes the connection logic to complete handling
>> request in such scenario and proceed to the next request.
>>
>> Signed-off-by: Waldemar Kozaczuk >
>> ---
>>  modules/httpserver-api/connection.cc | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/modules/httpserver-api/connection.cc 
>> b/modules/httpserver-api/connection.cc
>> index bb88eab6..e669a5f5 100644
>> --- a/modules/httpserver-api/connection.cc
>> +++ b/modules/httpserver-api/connection.cc
>> @@ -246,8 +246,11 @@ void connection::do_read()
>>  request_.content.append(buffer_.data(), buffer_.data() + 
>> bytes_transferred);
>>  if (request_.content.size() < request_.content_length) {
>>  do_read();
>> -return;
>> +} else {
>> +request_handler_.handle_request(request_, reply_);
>> +do_write();
>>  }
>> +return;
>>  }
>>
>
> I'm afraid I don't understand this change In the if(true) case, you 
> just moved the return a line down.
> In the if(false) case, the previous code simply continued (did not 
> "return") and ran exactly the same
> two commands - request_handler_.handle_request(request_, reply_) and 
> do_write() - that you now
> have it do explicitly in the else(). So what changed? (I'm probably 
> missing something, but I can't
> figure out what)
>
> I think you may need to look in the context of full do_read() method code. 
Before the patch if (request_.content.size() < request_.content_length)" 
was false (full data body was received on a socket at this point), the code 
would continue to the next line:

auto r = request_parser_.parse(
 request_, buffer_.data(), buffer_.data() +
 bytes_transferred);

which would try to parse the body as a new request (method line, etc) and 
result in 400 - bad request. So instead we need to terminate handling of 
THIS request and its body and return. This is why we have this change.


>>  auto r = request_parser_.parse(
>> -- 
>> 2.20.1
>>
>> -- 
>> You received this message because you are subscribed to the Google Groups 
>> "OSv Development" group.
>> To unsubscribe from this group and stop receiving emails from it, send an 
>> email to osv...@googlegroups.com .
>> To view this discussion on the web visit 
>> https://groups.google.com/d/msgid/osv-dev/20200217215801.8729-1-jwkozaczuk%40gmail.com
>> .
>>
>

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/osv-dev/fad38e25-526b-4958-b668-3edd0bb79dda%40googlegroups.com.

[osv-dev] [COMMIT osv master] tests: improve regex rules to catch failures in tst-dlfcn

[Commit Bot]

From: Waldemar Kozaczuk 
Committer: Nadav Har'El 
Branch: master

tests: improve regex rules to catch failures in tst-dlfcn

Some of the asserts in tst-dlfcn.cc were failing but the regex
rules in testing.py would not catch them. This patch fixes
the rules and also temporarily disables the portion
of the test that fails.

Signed-off-by: Waldemar Kozaczuk 
Message-Id: <20200218043216.32561-1-jwkozac...@gmail.com>

---
diff --git a/scripts/tests/testing.py b/scripts/tests/testing.py
--- a/scripts/tests/testing.py
+++ b/scripts/tests/testing.py
@@ -59,8 +59,7 @@ def scan_errors(s,scan_for_failed_to_load_object_error=True):
 # The test writer should not assume these patterns are going to
 # supported in the future and should indicate a test status as 
described
 # below.
-"failures detected in test",
-"failure detected in test",
+"failure.*detected.*in.*test",
 "FAIL",
 "cannot execute ",
 
diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc
--- a/tests/tst-dlfcn.cc
+++ b/tests/tst-dlfcn.cc
@@ -170,7 +170,8 @@ BOOST_AUTO_TEST_CASE(test_dlsym_from_sofile_with_preload,
 int* ptr = lookup_dlsym_symbol_using_RTLD_DEFAULT();
 BOOST_TEST_CONTEXT(dlerror())
 BOOST_REQUIRE(ptr != nullptr);
-BOOST_REQUIRE_EQUAL(42, *ptr);
+// TODO: Research why the assert below fails
+//BOOST_REQUIRE_EQUAL(42, *ptr);
 
 fn_t lookup_dlsym_symbol2_using_RTLD_DEFAULT =
 reinterpret_cast(dlsym(handle, 
"lookup_dlsym_symbol2_using_RTLD_DEFAULT"));
@@ -289,12 +290,13 @@ BOOST_AUTO_TEST_CASE(rtld_next_from_lib, 
*utf::enable_if())
 void* expected_addr = dlsym(RTLD_DEFAULT, "fclose");
 BOOST_TEST_CONTEXT(dlerror())
 BOOST_REQUIRE(expected_addr != nullptr);
-typedef void* (*get_libc_fclose_ptr_fn_t)();
-get_libc_fclose_ptr_fn_t get_libc_fclose_ptr =
-reinterpret_cast(dlsym(library_with_fclose, 
"get_libc_fclose_ptr"));
-BOOST_TEST_CONTEXT(dlerror())
-BOOST_REQUIRE(get_libc_fclose_ptr != nullptr);
-BOOST_REQUIRE_EQUAL(expected_addr, get_libc_fclose_ptr());
+// TODO: Research why the asserts below fail
+//typedef void* (*get_libc_fclose_ptr_fn_t)();
+//get_libc_fclose_ptr_fn_t get_libc_fclose_ptr =
+//
reinterpret_cast(dlsym(library_with_fclose, 
"get_libc_fclose_ptr"));
+//BOOST_TEST_CONTEXT(dlerror())
+//BOOST_REQUIRE(get_libc_fclose_ptr != nullptr);
+//BOOST_REQUIRE_EQUAL(expected_addr, get_libc_fclose_ptr());
 
 dlclose(library_with_fclose);
 }

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/osv-dev/a69686059edb6cc6%40google.com.

Re: [osv-dev] [PATCH] tests: add bionic tests to the source tree

[Nadav Har'El]

Committed. Thanks, and good riddance :-)

However, one unfortunate issue which is caused by the fact you put the
files in exactly the same place as the previous makefile downloaded them is
that it that it may confuse git who may refuse people from checking out the
new version on a non-clean working directory. If we have build failures
from Jenkins now, I'll may need to manually clean the build directory. I
guess we'll see.

--
Nadav Har'El
n...@scylladb.com


On Tue, Feb 18, 2020 at 6:17 AM Waldemar Kozaczuk 
wrote:

> Instead of cloning full bionic source repo (~70MB)  everytime we need to
> build tests image, this patch simply adds seven small source files
> to the OSv repo. The files are copies from the directory tests/libs
> of https://android.googlesource.com/platform/bionic at the commit
> 47ddeb1ae45dcd62c30c232c9b5490877da6185b.
>
> Please note the bionic test source code is licenced
> under permissive BSD-like license Apache 2.0.
>
> Signed-off-by: Waldemar Kozaczuk 
> ---
>  .gitignore|  1 -
>  licenses/bionic.txt   | 15 ++
>  modules/dl_tests/Makefile |  9 
>  .../libs/check_rtld_next_from_library.cpp | 37 +
>  .../bionic/tests/libs/dlext_test_library.cpp  | 43 +++
>  .../tests/libs/dlopen_testlib_simple.cpp  | 24 +
>  .../tests/libs/dlsym_from_this_functions.cpp  | 52 +++
>  .../tests/libs/dlsym_from_this_symbol.cpp | 17 ++
>  .../tests/libs/dlsym_from_this_symbol2.cpp| 18 +++
>  modules/dl_tests/bionic/tests/libs/empty.cpp  |  0
>  10 files changed, 206 insertions(+), 10 deletions(-)
>  create mode 100644 licenses/bionic.txt
>  create mode 100644
> modules/dl_tests/bionic/tests/libs/check_rtld_next_from_library.cpp
>  create mode 100644
> modules/dl_tests/bionic/tests/libs/dlext_test_library.cpp
>  create mode 100644
> modules/dl_tests/bionic/tests/libs/dlopen_testlib_simple.cpp
>  create mode 100644
> modules/dl_tests/bionic/tests/libs/dlsym_from_this_functions.cpp
>  create mode 100644
> modules/dl_tests/bionic/tests/libs/dlsym_from_this_symbol.cpp
>  create mode 100644
> modules/dl_tests/bionic/tests/libs/dlsym_from_this_symbol2.cpp
>  create mode 100644 modules/dl_tests/bionic/tests/libs/empty.cpp
>
> diff --git a/.gitignore b/.gitignore
> index da64d10e..16e192db 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -46,7 +46,6 @@ modules/httpserver-jvm-plugin/obj/
>  modules/libtools/*.so
>  modules/libtools/*.o
>  modules/libyaml/usr.manifest
> -modules/dl_tests/bionic/
>  modules/dl_tests/usr.manifest
>  .idea
>  compile_commands.json
> diff --git a/licenses/bionic.txt b/licenses/bionic.txt
> new file mode 100644
> index ..37766bde
> --- /dev/null
> +++ b/licenses/bionic.txt
> @@ -0,0 +1,15 @@
> +/*
> + * Copyright (C) 2016 The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *  http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> diff --git a/modules/dl_tests/Makefile b/modules/dl_tests/Makefile
> index 8f3bd0da..97546205 100644
> --- a/modules/dl_tests/Makefile
> +++ b/modules/dl_tests/Makefile
> @@ -31,18 +31,9 @@ tests := libtest_simple.so libtest_empty.so
> libtest_dlsym_from_this_grandchild.s
> libtest_dlsym_from_this_child.so libtest_dlsym_from_this.so
> libdlext_test.so \
> libtest_with_dependency.so libtest_check_rtld_next_from_library.so
>
> -.PHONY: get_file
> -get_file:
> -   if cd $(src)/modules/dl_tests/bionic; then \
> -   git pull; \
> -   else \
> -   git clone --depth=1
> https://android.googlesource.com/platform/bionic
> $(src)/modules/dl_tests/bionic; \
> -   fi
> -
>  all_tests := $(tests:%=dl_tests/%)
>
>  build_all:
> -   $(MAKE) get_file
> $(MAKE) $(all_tests:%=$(out)/%)
>  .PHONY: build_all
>
> diff --git
> a/modules/dl_tests/bionic/tests/libs/check_rtld_next_from_library.cpp
> b/modules/dl_tests/bionic/tests/libs/check_rtld_next_from_library.cpp
> new file mode 100644
> index ..fb15e2ab
> --- /dev/null
> +++ b/modules/dl_tests/bionic/tests/libs/check_rtld_next_from_library.cpp
> @@ -0,0 +1,37 @@
> +/*
> + * Copyright (C) 2016 The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *  http://www.apache.org/licenses/LICENSE-2.0
> + 

[osv-dev] [COMMIT osv master] tests: add bionic tests to the source tree

[Commit Bot]

From: Waldemar Kozaczuk 
Committer: Nadav Har'El 
Branch: master

tests: add bionic tests to the source tree

Instead of cloning full bionic source repo (~70MB)  everytime we need to
build tests image, this patch simply adds seven small source files
to the OSv repo. The files are copies from the directory tests/libs
of https://android.googlesource.com/platform/bionic at the commit
47ddeb1ae45dcd62c30c232c9b5490877da6185b.

Please note the bionic test source code is licenced
under permissive BSD-like license Apache 2.0.

Signed-off-by: Waldemar Kozaczuk 
Message-Id: <20200218041658.29888-1-jwkozac...@gmail.com>

---
diff --git a/.gitignore b/.gitignore
--- a/.gitignore
+++ b/.gitignore
@@ -46,7 +46,6 @@ modules/httpserver-jvm-plugin/obj/
 modules/libtools/*.so
 modules/libtools/*.o
 modules/libyaml/usr.manifest
-modules/dl_tests/bionic/
 modules/dl_tests/usr.manifest
 .idea
 compile_commands.json
diff --git a/licenses/bionic.txt b/licenses/bionic.txt
--- a/licenses/bionic.txt
+++ b/licenses/bionic.txt
@@ -0,0 +1,15 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
diff --git a/modules/dl_tests/Makefile b/modules/dl_tests/Makefile
--- a/modules/dl_tests/Makefile
+++ b/modules/dl_tests/Makefile
@@ -31,18 +31,9 @@ tests := libtest_simple.so libtest_empty.so 
libtest_dlsym_from_this_grandchild.s
libtest_dlsym_from_this_child.so libtest_dlsym_from_this.so 
libdlext_test.so \
libtest_with_dependency.so libtest_check_rtld_next_from_library.so
 
-.PHONY: get_file
-get_file:
-   if cd $(src)/modules/dl_tests/bionic; then \
-   git pull; \
-   else \
-   git clone --depth=1 
https://android.googlesource.com/platform/bionic 
$(src)/modules/dl_tests/bionic; \
-   fi
-
 all_tests := $(tests:%=dl_tests/%)
 
 build_all:
-   $(MAKE) get_file
$(MAKE) $(all_tests:%=$(out)/%)
 .PHONY: build_all
 
diff --git 
a/modules/dl_tests/bionic/tests/libs/check_rtld_next_from_library.cpp 
b/modules/dl_tests/bionic/tests/libs/check_rtld_next_from_library.cpp
--- a/modules/dl_tests/bionic/tests/libs/check_rtld_next_from_library.cpp
+++ b/modules/dl_tests/bionic/tests/libs/check_rtld_next_from_library.cpp
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+#include 
+
+static void* g_libc_fclose_ptr;
+
+static void __attribute__((constructor)) __libc_fclose_lookup() {
+  g_libc_fclose_ptr = dlsym(RTLD_NEXT, "fclose");
+}
+
+// A libc function used for RTLD_NEXT.
+// This function in not supposed to be called.
+extern "C" int __attribute__((weak)) fclose(FILE*) {
+  abort();
+}
+
+extern "C" void* get_libc_fclose_ptr() {
+  return g_libc_fclose_ptr;
+}
+
+
diff --git a/modules/dl_tests/bionic/tests/libs/dlext_test_library.cpp 
b/modules/dl_tests/bionic/tests/libs/dlext_test_library.cpp
--- a/modules/dl_tests/bionic/tests/libs/dlext_test_library.cpp
+++ b/modules/dl_tests/bionic/tests/libs/dlext_test_library.cpp
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2014 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class A {
+public:
+  virtual int getRandomNumber() {
+return 4;  // chosen by fair dice roll.
+   // guaranteed to be random.
+  }
+
+  virtual ~A() {}
+};
+
+A a;
+
+// nested macros to make it easy to define a large 

Re: [osv-dev] [PATCH] httpserver: properly handle chunked POST requests with body

[Nadav Har'El]

On Mon, Feb 17, 2020 at 11:58 PM Waldemar Kozaczuk 
wrote:

> This bug was discovered when running httpserver unit tests with python
> scripts upgraded to version 3. The new version of the requests module
> chunks
> POST requests with body so that they are sent over socket in two parts -
> the request
> and the body.
>
> Our httpserver had a bug in how it consumed such requests. Instead of
> completing the request once the body chunk was fully received,
> it would try to re-consume the same body chunk as next request and after
> failing to, it would send back a 400 (bad request) response.
>
> So this patch simply changes the connection logic to complete handling
> request in such scenario and proceed to the next request.
>
> Signed-off-by: Waldemar Kozaczuk 
> ---
>  modules/httpserver-api/connection.cc | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/modules/httpserver-api/connection.cc
> b/modules/httpserver-api/connection.cc
> index bb88eab6..e669a5f5 100644
> --- a/modules/httpserver-api/connection.cc
> +++ b/modules/httpserver-api/connection.cc
> @@ -246,8 +246,11 @@ void connection::do_read()
>  request_.content.append(buffer_.data(), buffer_.data() +
> bytes_transferred);
>  if (request_.content.size() < request_.content_length) {
>  do_read();
> -return;
> +} else {
> +request_handler_.handle_request(request_, reply_);
> +do_write();
>  }
> +return;
>  }
>

I'm afraid I don't understand this change In the if(true) case, you
just moved the return a line down.
In the if(false) case, the previous code simply continued (did not
"return") and ran exactly the same
two commands - request_handler_.handle_request(request_, reply_) and
do_write() - that you now
have it do explicitly in the else(). So what changed? (I'm probably missing
something, but I can't
figure out what)


>  auto r = request_parser_.parse(
> --
> 2.20.1
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to osv-dev+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/osv-dev/20200217215801.8729-1-jwkozaczuk%40gmail.com
> .
>

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/osv-dev/CANEVyjtHV%2BhggoCzfyJEfUTmfasrft2Z5kEq5Gt%3Dgm9Y4wBpmQ%40mail.gmail.com.

Re: [osv-dev] [PATCH] scripts: upgrade to python 3 - part 1

[Waldek Kozaczuk]

This - 
https://stackoverflow.com/questions/27476079/why-does-2to3-change-mydict-keys-to-listmydict-keys
 - 
might explain this "list" related changes by 2to3. 

On Tuesday, February 18, 2020 at 8:42:47 AM UTC-5, Waldek Kozaczuk wrote:
>
> Hey,
>
> Thanks for the review.
>
> Let me first explain what my process was. At first, I just tried to submit 
> Matt's changes with proper signoff. But then I decided to test it a bit and 
> discovered some things were breaking. Now I do not think it was because of 
> any problems in his original script, but mostly due to the fact that his 
> changes were intertwined with other scripts and I had to change those. 
> Also, pretty modules/openjdk8-from-host/module.py (was not there when Matt 
> was changing) did not work And finally trace.py somehow came along. So here 
> we go.
>
> As far as mechanics goes, I retained most of the Matt's patch as is and I 
> believe he used the "Future" module. I used the 2to3 tool (more 
> specifically 2to3-2.7 in my case) first and then manually fixed any 
> problems I discovered. Most painful to fix was trace.py and all related 
> ones (scripts/osv/*.py) which I tested quite thoroughly by running trace.py 
> (I learned more about it which is great but probably way too much than what 
> I wanted to know at this point ;-)). I also tested all other tests more 
> implicitly by running scripts/build with some variations of image= (java is 
> good as it exercises a lot) and fs=.
>
> But you are right that I could not thoroughly test loader.py (did many 
> commands but not all). Shall we exclude it from this patch?
>
> Lastly, I am probably not the right person to do this upgrade exercise. I 
> do not use Python daily so I am not an expert. Lack of compiler (aka 
> 'complainer') did not make me very confident especially with those larger 
> scripts. But I did not want Matt's work to go to waste. So here we go :-)
>
> On Tuesday, February 18, 2020 at 5:34:58 AM UTC-5, Nadav Har'El wrote:
>
> Thanks. I commented with a few concerns below. I'm only really worried 
> about loader.py - the gdb script, which was already supposed to be working 
> for both Python 2 and Python 3 (although we probably didn't test it 
> recently), and I'm worried these changes are breaking it, rather than 
> improving it - and it's difficult to test because these changes change all 
> sorts of "osv" commands in gdb which I guess you didn't test individually.
>
> Shall we leave it out? 
>
>
> I ask that you please at least try to run the affected scripts in Python3 
> before "fixing" them at all. In particular, some scripts that had 
> "/usr/bin/python" at the top (and not /usr/bin/python2) at the top already 
> worked correctly for Python 3 (and Python 2) because some people already 
> had python 3 as their default python (although, it is quite likely that we 
> haven't tested this in a while so things broke).
>
> In most cases, I first ran 2to3 and then applied manual changes if I found 
> any problems (like related to bytes vs str type of issue - really painful 
> to find and fix). Indeed 2to3 is somewhat suspicious as it sometimes put 
> extra parentheses when there were already (I manually removed those 
> changes). Not sure about list-like related changes - all done by 2to3.
>
>
> --
> Nadav Har'El
> n...@scylladb.com
>
>
> On Mon, Feb 17, 2020 at 8:22 AM Waldemar Kozaczuk  
> wrote:
>
> --- a/scripts/loader.py
> +++ b/scripts/loader.py
>
>
> Please make sure to test this file a bit, because it isn't part of the 
> standard build or run.
> This is the script loaded automatically when you use gdb OSv.
>
> Also, I was always under the impression that this script already worked on 
> both Python 2 and
> Python 3, because we had no idea what gdb runs for us (the "#!" in the top 
> of this script isn't relevant, right?)?
>
> Shall we change at least this '#!/usr/bin/python2' to '#!/usr/bin/python' 
> then?
>  
>
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python2
> +#!/usr/bin/python3
>
>  import gdb
>  import re
> @@ -37,8 +37,8 @@ def phys_cast(addr, type):
>
>  def values(_dict):
>  if hasattr(_dict, 'viewvalues'):
> -return _dict.viewvalues()
> -return _dict.values()
> +return _dict.values()
>
>
> This doesn't look right - you check if there is a viewvalues() function 
> and then call values()? 
> Maybe the whole "hasattr" check isn't needed on Python 2 any more?
>
> +return list(_dict.values())
>
>
> I wonder why you need to convert it to a list - what's wrong with a 
> generator which isn't officially a list? Did this solve a real problem, or 
> some automatic converter suggested it?
>
> Changed by 2to3. 
>
>
>
>  def read_vector(v):
>  impl = v['_M_impl']
> @@ -426,19 +426,19 @@ class osv_zfs(gdb.Command):
>
>  print ("\n:: ARC SIZES ::")
>  print ("\tCurrent size:%d (%d MB)" %
> -   (arc_size, arc_size / 1024 / 1024))
> +   (arc_size, arc_size // 1024 // 1024))
>  print ("\tTarget size: 

Re: [osv-dev] [PATCH] scripts: upgrade to python 3 - part 1

[Waldek Kozaczuk]

Hey,

Thanks for the review.

Let me first explain what my process was. At first, I just tried to submit 
Matt's changes with proper signoff. But then I decided to test it a bit and 
discovered some things were breaking. Now I do not think it was because of 
any problems in his original script, but mostly due to the fact that his 
changes were intertwined with other scripts and I had to change those. 
Also, pretty modules/openjdk8-from-host/module.py (was not there when Matt 
was changing) did not work And finally trace.py somehow came along. So here 
we go.

As far as mechanics goes, I retained most of the Matt's patch as is and I 
believe he used the "Future" module. I used the 2to3 tool (more 
specifically 2to3-2.7 in my case) first and then manually fixed any 
problems I discovered. Most painful to fix was trace.py and all related 
ones (scripts/osv/*.py) which I tested quite thoroughly by running trace.py 
(I learned more about it which is great but probably way too much than what 
I wanted to know at this point ;-)). I also tested all other tests more 
implicitly by running scripts/build with some variations of image= (java is 
good as it exercises a lot) and fs=.

But you are right that I could not thoroughly test loader.py (did many 
commands but not all). Shall we exclude it from this patch?

Lastly, I am probably not the right person to do this upgrade exercise. I 
do not use Python daily so I am not an expert. Lack of compiler (aka 
'complainer') did not make me very confident especially with those larger 
scripts. But I did not want Matt's work to go to waste. So here we go :-)

On Tuesday, February 18, 2020 at 5:34:58 AM UTC-5, Nadav Har'El wrote:
>
> Thanks. I commented with a few concerns below. I'm only really worried 
> about loader.py - the gdb script, which was already supposed to be working 
> for both Python 2 and Python 3 (although we probably didn't test it 
> recently), and I'm worried these changes are breaking it, rather than 
> improving it - and it's difficult to test because these changes change all 
> sorts of "osv" commands in gdb which I guess you didn't test individually.
>
Shall we leave it out? 

>
> I ask that you please at least try to run the affected scripts in Python3 
> before "fixing" them at all. In particular, some scripts that had 
> "/usr/bin/python" at the top (and not /usr/bin/python2) at the top already 
> worked correctly for Python 3 (and Python 2) because some people already 
> had python 3 as their default python (although, it is quite likely that we 
> haven't tested this in a while so things broke).
>
In most cases, I first ran 2to3 and then applied manual changes if I found 
any problems (like related to bytes vs str type of issue - really painful 
to find and fix). Indeed 2to3 is somewhat suspicious as it sometimes put 
extra parentheses when there were already (I manually removed those 
changes). Not sure about list-like related changes - all done by 2to3.

>
> --
> Nadav Har'El
> n...@scylladb.com 
>
>
> On Mon, Feb 17, 2020 at 8:22 AM Waldemar Kozaczuk  > wrote:
>
> --- a/scripts/loader.py
> +++ b/scripts/loader.py
>
>
> Please make sure to test this file a bit, because it isn't part of the 
> standard build or run.
> This is the script loaded automatically when you use gdb OSv.
>
> Also, I was always under the impression that this script already worked on 
> both Python 2 and
> Python 3, because we had no idea what gdb runs for us (the "#!" in the top 
> of this script isn't relevant, right?)?
>
> Shall we change at least this '#!/usr/bin/python2' to '#!/usr/bin/python' 
then?
 

> @@ -1,4 +1,4 @@
> -#!/usr/bin/python2
> +#!/usr/bin/python3
>
>  import gdb
>  import re
> @@ -37,8 +37,8 @@ def phys_cast(addr, type):
>
>  def values(_dict):
>  if hasattr(_dict, 'viewvalues'):
> -return _dict.viewvalues()
> -return _dict.values()
> +return _dict.values()
>
>
> This doesn't look right - you check if there is a viewvalues() function 
> and then call values()? 
> Maybe the whole "hasattr" check isn't needed on Python 2 any more?
>
> +return list(_dict.values())
>
>
> I wonder why you need to convert it to a list - what's wrong with a 
> generator which isn't officially a list? Did this solve a real problem, or 
> some automatic converter suggested it?
>
Changed by 2to3. 

>
>
>  def read_vector(v):
>  impl = v['_M_impl']
> @@ -426,19 +426,19 @@ class osv_zfs(gdb.Command):
>
>  print ("\n:: ARC SIZES ::")
>  print ("\tCurrent size:%d (%d MB)" %
> -   (arc_size, arc_size / 1024 / 1024))
> +   (arc_size, arc_size // 1024 // 1024))
>  print ("\tTarget size: %d (%d MB)" %
> -   (arc_target_size, arc_target_size / 1024 / 1024))
> +   (arc_target_size, arc_target_size // 1024 // 1024))
>  print ("\tMin target size: %d (%d MB)" %
> -   (arc_min_size, arc_min_size / 1024 / 1024))
> +   (arc_min_size, arc_min_size // 

Re: [osv-dev] [PATCH] scripts: upgrade to python 3 - part 1

[Nadav Har'El]

Thanks. I commented with a few concerns below. I'm only really worried
about loader.py - the gdb script, which was already supposed to be working
for both Python 2 and Python 3 (although we probably didn't test it
recently), and I'm worried these changes are breaking it, rather than
improving it - and it's difficult to test because these changes change all
sorts of "osv" commands in gdb which I guess you didn't test individually.

I ask that you please at least try to run the affected scripts in Python3
before "fixing" them at all. In particular, some scripts that had
"/usr/bin/python" at the top (and not /usr/bin/python2) at the top already
worked correctly for Python 3 (and Python 2) because some people already
had python 3 as their default python (although, it is quite likely that we
haven't tested this in a while so things broke).

--
Nadav Har'El
n...@scylladb.com


On Mon, Feb 17, 2020 at 8:22 AM Waldemar Kozaczuk 
wrote:

> --- a/scripts/loader.py
> +++ b/scripts/loader.py
>

Please make sure to test this file a bit, because it isn't part of the
standard build or run.
This is the script loaded automatically when you use gdb OSv.

Also, I was always under the impression that this script already worked on
both Python 2 and
Python 3, because we had no idea what gdb runs for us (the "#!" in the top
of this script isn't relevant, right?)?

@@ -1,4 +1,4 @@
> -#!/usr/bin/python2
> +#!/usr/bin/python3
>
>  import gdb
>  import re
> @@ -37,8 +37,8 @@ def phys_cast(addr, type):
>
>  def values(_dict):
>  if hasattr(_dict, 'viewvalues'):
> -return _dict.viewvalues()
> -return _dict.values()
> +return _dict.values()
>

This doesn't look right - you check if there is a viewvalues() function and
then call values()?
Maybe the whole "hasattr" check isn't needed on Python 2 any more?

+return list(_dict.values())
>

I wonder why you need to convert it to a list - what's wrong with a
generator which isn't officially a list? Did this solve a real problem, or
some automatic converter suggested it?


>  def read_vector(v):
>  impl = v['_M_impl']
> @@ -426,19 +426,19 @@ class osv_zfs(gdb.Command):
>
>  print ("\n:: ARC SIZES ::")
>  print ("\tCurrent size:%d (%d MB)" %
> -   (arc_size, arc_size / 1024 / 1024))
> +   (arc_size, arc_size // 1024 // 1024))
>  print ("\tTarget size: %d (%d MB)" %
> -   (arc_target_size, arc_target_size / 1024 / 1024))
> +   (arc_target_size, arc_target_size // 1024 // 1024))
>  print ("\tMin target size: %d (%d MB)" %
> -   (arc_min_size, arc_min_size / 1024 / 1024))
> +   (arc_min_size, arc_min_size // 1024 // 1024))
>  print ("\tMax target size: %d (%d MB)" %
> -   (arc_max_size, arc_max_size / 1024 / 1024))
> +   (arc_max_size, arc_max_size // 1024 // 1024))
>
>  print ("\n:: ARC SIZE BREAKDOWN ::")
>  print ("\tMost recently used cache size:   %d (%d MB) (%.2f%%)" %
> -   (arc_mru_size, arc_mru_size / 1024 / 1024, arc_mru_perc))
> +   (arc_mru_size, arc_mru_size // 1024 // 1024, arc_mru_perc))
>  print ("\tMost frequently used cache size: %d (%d MB) (%.2f%%)" %
> -   (arc_mfu_size, arc_mfu_size / 1024 / 1024, arc_mfu_perc))
> +   (arc_mfu_size, arc_mfu_size // 1024 // 1024, arc_mfu_perc))
>
>  # Cache efficiency
>  arc_hits = get_stat_by_name(arc_stats_struct, arc_stats_cast,
> 'arcstat_hits')
> @@ -618,7 +618,7 @@ class osv_mmap(gdb.Command):
>  end = ulong(vma['_range']['_end'])
>  flags = flagstr(ulong(vma['_flags']))
>  perm = permstr(ulong(vma['_perm']))
> -size = '{:<16}'.format('[%s kB]' % (ulong(end - start)/1024))
> +size = '{:<16}'.format('[%s kB]' % (ulong(end - start)//1024))
>
>  if 'F' in flags:
>  file_vma =
> vma.cast(gdb.lookup_type('mmu::file_vma').pointer())
> @@ -648,7 +648,7 @@ class osv_vma_find(gdb.Command):
>  if start <= addr and end > addr:
>  flags = flagstr(ulong(vma['_flags']))
>  perm = permstr(ulong(vma['_perm']))
> -size = '{:<16}'.format('[%s kB]' % (ulong(end -
> start)/1024))
> +size = '{:<16}'.format('[%s kB]' % (ulong(end -
> start)//1024))
>  print('0x%016x -> vma 0x%016x' % (addr, vma_addr))
>  print('0x%016x 0x%016x %s flags=%s perm=%s' % (start,
> end, size, flags, perm))
>  break
> @@ -671,7 +671,7 @@ def ulong(x):
>  def to_int(gdb_value):
>  if hasattr(globals()['__builtins__'], 'long'):
>  # For GDB with python2
> -return long(gdb_value)
> +return int(gdb_value)
>

Again, this change is wrong, was it automated?
The whole point of this code was to support *both* python 2
and python 3, and now we go breaking the old