Re: [libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile

On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote: >On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko <jtomko(a)redhat.com&gt; wrote: > >>Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d >>Fixes: 43820e4b8037680ec451761216750c6b139db67a >>Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b >>Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; >>--- >> tests/virpcivpdtest.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >>diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c >>index 284350fe29..62c51cdeb9 100644 >>--- a/tests/virpcivpdtest.c >>+++ b/tests/virpcivpdtest.c >>@@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque >>G_GNUC_UNUSED) >> buf = g_malloc0(dataLen); >> >> fd = virCreateAnonymousFile(fullVPDExample, dataLen); >>+ if (fd < 0) >>+ return -1; >> > >I would prefer if you rewrote this before merging as: > > if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0) > return -1; > >(The whole patch.) I think it looks cleaner, but that's just my preference. > It might look cleaner for some until you get to the point of having more parentheses there and we already had some issues with that. I, for one, prefer an extra line or even an extra variable in some cases, but similarly to you it is only my preference and we do not have a coding style for this, unfortunately. We even have your preferred style used in the libvirt coding style page [0]. And guess what, we have that

wrong! Look: while ((rc = waitpid(pid, &st, 0) == -1) && errno == EINTR); // ok while ((rc = waitpid(pid, &st, 0) == -1) && errno == EINTR) { // Better /* nothing */ } I would hope that proves my point, but because we had this discussion a couple of times already I know there are lot of libvirt developers (probably in the majority) who prefer cramming as much into that one line, so I guess we won't achieve a consensus on this one ;) Just my €.02 ;)

Have a nice day, Martin