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

On a Tuesday in 2021, Martin Kletzander wrote: >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 I got a SIGSEGV trying to access [0], so I'll go with the other suggestion.

>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 ;) Thank you for your 0,51 Kč ;) Jano > >Have a nice day, >Martin