[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] util, qemu: Fix virDoes*Exist usage

On Mon, Nov 19, 2018 at 05:27:01PM +0100, Andrea Bolognani wrote:
On Mon, 2018-11-19 at 15:10 +0100, Martin Kletzander wrote:
Since the functions only return 0 or 1, they should return bool (missed the
change in the first commit).  That way it's clearer that the check for
non-existing group should be either "== 0" instead.

I don't get this part of the commit message... With the original
implementation, calling virDoesGroupExist() on a non-existing
group would have returned 0, correct? And now it will return false

Oh, I think I see what you mean: the original code expected the
integer return value to follow the usual libvirt conventions where
0 means success and <0 means failure, but in this case the functions
returned 1 on success and 0 on failure, so the logic in
virQEMUDriverConfigPtr() was wrong. Gotcha.

So, here's what I would do: I would split this into two patches, the
first one which contains only the first hunk - it will work even if
the return types are int - and fixes the bug, and the second one
which contains the remaining hunks and makes usage of the functions
more obvious and hopefully prevents more bugs from slipping in.

Does that sound reasonable?

Brutal honesty?  No.  For a patch that I would consider trivial I think it's a
waste of time, but nevertheless I'll do it.

Andrea Bolognani / Red Hat / Virtualization

Attachment: signature.asc
Description: Digital signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]