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 instead... 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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
