[PATCH v2 0/5] qemu_passt: Fix issues with PID file

Michal Privoznik posted 5 patches 1 year, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/cover.1676554196.git.mprivozn@redhat.com
src/qemu/qemu_passt.c | 61 +++++++++++++++++++++++++++++--------------
1 file changed, 41 insertions(+), 20 deletions(-)
[PATCH v2 0/5] qemu_passt: Fix issues with PID file
Posted by Michal Privoznik 1 year, 1 month ago
This is a v2 of:

https://listman.redhat.com/archives/libvir-list/2023-February/237731.html

diff to v1:
- Merged patches that were ACKed in v1,
- Dropped 4/4 from the original series (the one that sets --foreground),
  and implemented a different approach

Michal Prívozník (5):
  qemu_passt: Avoid double daemonizing passt
  qemu_passt: Report passt's error on failed start
  qemu_passt: Make passt report errors to stderr whenever possible
  qemu_passt: Deduplicate passt killing code
  qemu_passt: Let passt write the PID file

 src/qemu/qemu_passt.c | 61 +++++++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 20 deletions(-)

-- 
2.39.1

Re: [PATCH v2 0/5] qemu_passt: Fix issues with PID file
Posted by Laine Stump 1 year, 1 month ago
On 2/16/23 8:32 AM, Michal Privoznik wrote:
> This is a v2 of:
> 
> https://listman.redhat.com/archives/libvir-list/2023-February/237731.html
> 
> diff to v1:
> - Merged patches that were ACKed in v1,
> - Dropped 4/4 from the original series (the one that sets --foreground),
>    and implemented a different approach
> 
> Michal Prívozník (5):
>    qemu_passt: Avoid double daemonizing passt
>    qemu_passt: Report passt's error on failed start
>    qemu_passt: Make passt report errors to stderr whenever possible
>    qemu_passt: Deduplicate passt killing code
>    qemu_passt: Let passt write the PID file

This is everything that was in the patch I sent last week, with the 
following additions

1) adding NULLSTR() around the reference to errbuf in patch 2/5

2) adding "--stderr" to the commandline in patch 3/5 (which I found to 
be unnecessary in my testing - as Stefano says everything goes to stderr 
until passt has completed its init anyway)

3) the other bit of patch 3/5 which adds an extra message telling the 
user to look into the designated logfile for the error - this is 
unnecessary (and actually now counter-productive, as it forces you to 
look elsewhere for the error when you wouldn't have needed to) because 
of patches I've sent to passt.

4)  patch 4/5 that is a cleanup de-duplicating code

5) patch 5 changes additional code (that I didn't touch in my patch) to 
use virPidFileReadPath() instead of virPidFileReadPathIfLocked(), and 
virProcessKillPainfully() instead of the higher level 
virPidFileForceCleanupPath().

So it all seems fine (except the error reporting stuff), but why revert 
a patch only to push back the same changes in a deconstructed fashion 
plus some fixups, rather than just posting a followup or two?

> 
>   src/qemu/qemu_passt.c | 61 +++++++++++++++++++++++++++++--------------
>   1 file changed, 41 insertions(+), 20 deletions(-)
> 
Re: [PATCH v2 0/5] qemu_passt: Fix issues with PID file
Posted by Michal Prívozník 1 year, 1 month ago
On 2/16/23 17:35, Laine Stump wrote:
> On 2/16/23 8:32 AM, Michal Privoznik wrote:
>> This is a v2 of:
>>
>> https://listman.redhat.com/archives/libvir-list/2023-February/237731.html
>>
>> diff to v1:
>> - Merged patches that were ACKed in v1,
>> - Dropped 4/4 from the original series (the one that sets --foreground),
>>    and implemented a different approach
>>
>> Michal Prívozník (5):
>>    qemu_passt: Avoid double daemonizing passt
>>    qemu_passt: Report passt's error on failed start
>>    qemu_passt: Make passt report errors to stderr whenever possible
>>    qemu_passt: Deduplicate passt killing code
>>    qemu_passt: Let passt write the PID file
> 
> This is everything that was in the patch I sent last week, with the
> following additions
> 
> 1) adding NULLSTR() around the reference to errbuf in patch 2/5
> 
> 2) adding "--stderr" to the commandline in patch 3/5 (which I found to
> be unnecessary in my testing - as Stefano says everything goes to stderr
> until passt has completed its init anyway)
> 
> 3) the other bit of patch 3/5 which adds an extra message telling the
> user to look into the designated logfile for the error - this is
> unnecessary (and actually now counter-productive, as it forces you to
> look elsewhere for the error when you wouldn't have needed to) because
> of patches I've sent to passt.
> 
> 4)  patch 4/5 that is a cleanup de-duplicating code
> 
> 5) patch 5 changes additional code (that I didn't touch in my patch) to
> use virPidFileReadPath() instead of virPidFileReadPathIfLocked(), and
> virProcessKillPainfully() instead of the higher level
> virPidFileForceCleanupPath().
> 
> So it all seems fine (except the error reporting stuff), but why revert
> a patch only to push back the same changes in a deconstructed fashion
> plus some fixups, rather than just posting a followup or two?

Yeah, I realized this too and I'm sorry. My original intention was to
fix this in a completely different way (as my last patch from v1
demonstrates) and that was incompatible with yours.

Michal

Re: [PATCH v2 0/5] qemu_passt: Fix issues with PID file
Posted by Stefano Brivio 1 year, 1 month ago
Michal,

On Fri, 17 Feb 2023 13:51:42 +0100
Michal Prívozník <mprivozn@redhat.com> wrote:

> On 2/16/23 17:35, Laine Stump wrote:
> > On 2/16/23 8:32 AM, Michal Privoznik wrote:  
> >> This is a v2 of:
> >>
> >> https://listman.redhat.com/archives/libvir-list/2023-February/237731.html
> >>
> >> diff to v1:
> >> - Merged patches that were ACKed in v1,
> >> - Dropped 4/4 from the original series (the one that sets --foreground),
> >>    and implemented a different approach
> >>
> >> Michal Prívozník (5):
> >>    qemu_passt: Avoid double daemonizing passt
> >>    qemu_passt: Report passt's error on failed start
> >>    qemu_passt: Make passt report errors to stderr whenever possible
> >>    qemu_passt: Deduplicate passt killing code
> >>    qemu_passt: Let passt write the PID file  
> > 
> > This is everything that was in the patch I sent last week, with the
> > following additions
> > 
> > 1) adding NULLSTR() around the reference to errbuf in patch 2/5
> > 
> > 2) adding "--stderr" to the commandline in patch 3/5 (which I found to
> > be unnecessary in my testing - as Stefano says everything goes to stderr
> > until passt has completed its init anyway)
> > 
> > 3) the other bit of patch 3/5 which adds an extra message telling the
> > user to look into the designated logfile for the error - this is
> > unnecessary (and actually now counter-productive, as it forces you to
> > look elsewhere for the error when you wouldn't have needed to) because
> > of patches I've sent to passt.
> > 
> > 4)  patch 4/5 that is a cleanup de-duplicating code
> > 
> > 5) patch 5 changes additional code (that I didn't touch in my patch) to
> > use virPidFileReadPath() instead of virPidFileReadPathIfLocked(), and
> > virProcessKillPainfully() instead of the higher level
> > virPidFileForceCleanupPath().
> > 
> > So it all seems fine (except the error reporting stuff), but why revert
> > a patch only to push back the same changes in a deconstructed fashion
> > plus some fixups, rather than just posting a followup or two?  
> 
> Yeah, I realized this too and I'm sorry. My original intention was to
> fix this in a completely different way (as my last patch from v1
> demonstrates) and that was incompatible with yours.

How do you want to proceed? Laine's series to improve error reporting in
passt is included in:

- passt version 2023_02_16.4663ccc
- Debian and Ubuntu packages 0.0~git20230216.4663ccc, pending upload
- Fedora and CentOS Stream packages passt-0^20230216.g4663ccc-1
  (stable: fc38, fc39, eln126, testing: fc36, fc37)

I guess either an updated patch from Laine with your refinements or an
updated series from you would work...? Thanks,

-- 
Stefano
Re: [PATCH v2 0/5] qemu_passt: Fix issues with PID file
Posted by Stefano Brivio 1 year, 1 month ago
On Mon, 20 Feb 2023 09:38:05 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> Michal,
> 
> On Fri, 17 Feb 2023 13:51:42 +0100
> Michal Prívozník <mprivozn@redhat.com> wrote:
> 
> > On 2/16/23 17:35, Laine Stump wrote:  
> > > On 2/16/23 8:32 AM, Michal Privoznik wrote:    
> > >> This is a v2 of:
> > >>
> > >> https://listman.redhat.com/archives/libvir-list/2023-February/237731.html
> > >>
> > >> diff to v1:
> > >> - Merged patches that were ACKed in v1,
> > >> - Dropped 4/4 from the original series (the one that sets --foreground),
> > >>    and implemented a different approach
> > >>
> > >> Michal Prívozník (5):
> > >>    qemu_passt: Avoid double daemonizing passt
> > >>    qemu_passt: Report passt's error on failed start
> > >>    qemu_passt: Make passt report errors to stderr whenever possible
> > >>    qemu_passt: Deduplicate passt killing code
> > >>    qemu_passt: Let passt write the PID file    
> > > 
> > > This is everything that was in the patch I sent last week, with the
> > > following additions
> > > 
> > > 1) adding NULLSTR() around the reference to errbuf in patch 2/5
> > > 
> > > 2) adding "--stderr" to the commandline in patch 3/5 (which I found to
> > > be unnecessary in my testing - as Stefano says everything goes to stderr
> > > until passt has completed its init anyway)
> > > 
> > > 3) the other bit of patch 3/5 which adds an extra message telling the
> > > user to look into the designated logfile for the error - this is
> > > unnecessary (and actually now counter-productive, as it forces you to
> > > look elsewhere for the error when you wouldn't have needed to) because
> > > of patches I've sent to passt.
> > > 
> > > 4)  patch 4/5 that is a cleanup de-duplicating code
> > > 
> > > 5) patch 5 changes additional code (that I didn't touch in my patch) to
> > > use virPidFileReadPath() instead of virPidFileReadPathIfLocked(), and
> > > virProcessKillPainfully() instead of the higher level
> > > virPidFileForceCleanupPath().
> > > 
> > > So it all seems fine (except the error reporting stuff), but why revert
> > > a patch only to push back the same changes in a deconstructed fashion
> > > plus some fixups, rather than just posting a followup or two?    
> > 
> > Yeah, I realized this too and I'm sorry. My original intention was to
> > fix this in a completely different way (as my last patch from v1
> > demonstrates) and that was incompatible with yours.  
> 
> How do you want to proceed? Laine's series to improve error reporting in
> passt is included in:
> 
> - passt version 2023_02_16.4663ccc
> - Debian and Ubuntu packages 0.0~git20230216.4663ccc, pending upload
> - Fedora and CentOS Stream packages passt-0^20230216.g4663ccc-1
>   (stable: fc38, fc39, eln126, testing: fc36, fc37)
> 
> I guess either an updated patch from Laine with your refinements or an
> updated series from you would work...? Thanks,

Ah, I see you just pushed your series...

-- 
Stefano