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

["Denis V. Lunev" <den@openvz.org>]

On 17/02/15 20:56, Michael Roth wrote:
> 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).
>

OK to me.

I'll check Win32 version with Olga to be somewhat
sure with upcoming cleanup.