Re: [libvirt] [PATCH 1/5] tests: qemuxml2argvmock: Don't mock virCommandPassFD

2018-08-16 Thread Peter Krempa 
On Thu, Aug 16, 2018 at 12:08:12 +0200, Michal Privoznik wrote:
> On 08/14/2018 03:21 PM, Peter Krempa wrote:
> > This function does not modify the host. It merely puts the file
> > descriptor into a list in virCommandPtr.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >  tests/qemuxml2argvmock.c | 8 
> >  1 file changed, 8 deletions(-)
> > 
> > diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> > index 4df92cf396..c8a5f186d5 100644
> > --- a/tests/qemuxml2argvmock.c
> > +++ b/tests/qemuxml2argvmock.c
> > @@ -184,14 +184,6 @@ virNetDevRunEthernetScript(const char *ifname 
> > ATTRIBUTE_UNUSED,
> >  return 0;
> >  }
> > 
> > -void
> > -virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
> > - int fd ATTRIBUTE_UNUSED,
> > - unsigned int flags ATTRIBUTE_UNUSED)
> > -{
> > -/* nada */
> > -}
> > -
> >  int
> >  virNetDevOpenvswitchGetVhostuserIfname(const char *path ATTRIBUTE_UNUSED,
> > char **ifname)
> > 
> 
> NACK, this indeed causes caller to close wrong FD:
> 
> 
> libvirt.git/tests $ ../run valgrind --trace-children=yes ./qemuxml2argvtest
> 
> ==210382== Warning: invalid file descriptor 1729 in syscall close()

I've posted a v2 of these yesterday in the morning.

Also in this case we know that fd 1729 is in fact invalid as we check
that is unused prior to this and the test would abort if it was used.

The new approach in v2 whitelists only the two FDs (which are still
invalid though) to be passed in virCommand since otherwise the command
line generator will not work.

Also note that when forking subprocesses we close a bunch of invalid FDs
anyways so it should not be a problem if we know they are not used
elsewhere.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/5] tests: qemuxml2argvmock: Don't mock virCommandPassFD

2018-08-16 Thread Michal Privoznik 
On 08/14/2018 03:21 PM, Peter Krempa wrote:
> This function does not modify the host. It merely puts the file
> descriptor into a list in virCommandPtr.
> 
> Signed-off-by: Peter Krempa 
> ---
>  tests/qemuxml2argvmock.c | 8 
>  1 file changed, 8 deletions(-)
> 
> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> index 4df92cf396..c8a5f186d5 100644
> --- a/tests/qemuxml2argvmock.c
> +++ b/tests/qemuxml2argvmock.c
> @@ -184,14 +184,6 @@ virNetDevRunEthernetScript(const char *ifname 
> ATTRIBUTE_UNUSED,
>  return 0;
>  }
> 
> -void
> -virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
> - int fd ATTRIBUTE_UNUSED,
> - unsigned int flags ATTRIBUTE_UNUSED)
> -{
> -/* nada */
> -}
> -
>  int
>  virNetDevOpenvswitchGetVhostuserIfname(const char *path ATTRIBUTE_UNUSED,
> char **ifname)
> 

NACK, this indeed causes caller to close wrong FD:


libvirt.git/tests $ ../run valgrind --trace-children=yes ./qemuxml2argvtest

==210382== Warning: invalid file descriptor 1729 in syscall close()


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/5] tests: qemuxml2argvmock: Don't mock virCommandPassFD

2018-08-14 Thread Daniel P . Berrangé 
On Tue, Aug 14, 2018 at 03:21:06PM +0200, Peter Krempa wrote:
> This function does not modify the host. It merely puts the file
> descriptor into a list in virCommandPtr.

Yes & no.

It doesn't directly modify the host, but my adding the FD to 'passfd' set,
when virCommandFree is run later, it will call VIR_FORCE_CLOSE on every
FD in the set that has the CLOSE_PARENT flag. So not mocking this could
indirectly cause cleanup code to modify the host by closing FDs.

All depends on whether we have any other mocks that are faking file
descriptors. If we don't then removing the mock is safe.

> Signed-off-by: Peter Krempa 
> ---
>  tests/qemuxml2argvmock.c | 8 
>  1 file changed, 8 deletions(-)
> 
> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> index 4df92cf396..c8a5f186d5 100644
> --- a/tests/qemuxml2argvmock.c
> +++ b/tests/qemuxml2argvmock.c
> @@ -184,14 +184,6 @@ virNetDevRunEthernetScript(const char *ifname 
> ATTRIBUTE_UNUSED,
>  return 0;
>  }
> 
> -void
> -virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
> - int fd ATTRIBUTE_UNUSED,
> - unsigned int flags ATTRIBUTE_UNUSED)
> -{
> -/* nada */
> -}
> -
>  int
>  virNetDevOpenvswitchGetVhostuserIfname(const char *path ATTRIBUTE_UNUSED,
> char **ifname)
> -- 
> 2.16.2
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/5] tests: qemuxml2argvmock: Don't mock virCommandPassFD

2018-08-14 Thread Peter Krempa 
This function does not modify the host. It merely puts the file
descriptor into a list in virCommandPtr.

Signed-off-by: Peter Krempa 
---
 tests/qemuxml2argvmock.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index 4df92cf396..c8a5f186d5 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -184,14 +184,6 @@ virNetDevRunEthernetScript(const char *ifname 
ATTRIBUTE_UNUSED,
 return 0;
 }

-void
-virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
- int fd ATTRIBUTE_UNUSED,
- unsigned int flags ATTRIBUTE_UNUSED)
-{
-/* nada */
-}
-
 int
 virNetDevOpenvswitchGetVhostuserIfname(const char *path ATTRIBUTE_UNUSED,
char **ifname)
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list