Re: [Qemu-devel] [PATCH 03/10] guest agent: guest-file-open: refactoring

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Denis V. Lunev (2015-02-17 10:06:49)
> On 17/02/15 18:27, Eric Blake wrote:
> > On 02/16/2015 08:14 PM, Michael Roth wrote:
> >> From: Simon Zolin <szolin@parallels.com>
> >>
> >> Moved the code that sets non-blocking flag on fd into a separate function.
> >>
> >> Signed-off-by: Simon Zolin <szolin@parallels.com>
> >> Reviewed-by: Roman Kagan <rkagan@parallels.com>
> >> Signed-off-by: Denis V. Lunev <den@openvz.org>
> >> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> CC: Eric Blake <eblake@redhat.com>
> >> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> ---
> >>   qga/commands-posix.c | 31 +++++++++++++++++++++++--------
> >>   1 file changed, 23 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> >> index 57409d0..ed527a3 100644
> >> --- a/qga/commands-posix.c
> >> +++ b/qga/commands-posix.c
> >> @@ -376,13 +376,33 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
> >>       return NULL;
> >>   }
> >>   
> >> +static int guest_file_toggle_flags(int fd, int flags, bool set, Error **err)
> >> +{
> > Why are you reinventing qemu_set_nonblock()?
> >
> because we are uneducated :)
> 
> Anyway, qemu_set_nonblock() does not handle error
> and resides in a strange header aka  "include/qemu/sockets.h"
> Technically I can switch to it immediately. Though error
> check condition will be lost.
> 
> What is better at your opinion?
> 
> a) return error from qemu_set_nonblock()/qemu_set_block()

I think making the existing functions a non-error-checking
wrapper around qemu_set_{block,nonblock}_error or something
would be best.

These are meant to be os-agnostic utility functions though,
but in the case of qemu-ga the win32 variant won't work as
expected, so I'm not sure it's a good idea to rely on them.

If the lack of it's usage in net/tap* compared to other parts
of QEMU that build on w32 is any indication, that seems to
be the pattern followed by other users.

In any case, since I was actually the one who re-invented it,
and this code just moves it to another function, I think we
can address it as a seperate patch and leave the PULL
intact (unless there are other objections).

> b) drop error check here. The descriptor is just opened
>      and we know that it is valid. I could not imagine real
>      error other than broken descriptor for this exact fcntl.
> 
> Regards,
>      Den