From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40675) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WsbSm-0006Ne-6g for qemu-devel@nongnu.org; Thu, 05 Jun 2014 13:28:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WsbSe-0003o5-Of for qemu-devel@nongnu.org; Thu, 05 Jun 2014 13:28:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31423) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WsbSe-0003nl-Hf for qemu-devel@nongnu.org; Thu, 05 Jun 2014 13:28:20 -0400 Message-ID: <5390A8A9.6040709@redhat.com> Date: Thu, 05 Jun 2014 19:28:09 +0200 From: Max Reitz MIME-Version: 1.0 References: <1401561792-13410-1-git-send-email-mreitz@redhat.com> <1401561792-13410-2-git-send-email-mreitz@redhat.com> <20140604115246.GB11073@stefanha-thinkpad.redhat.com> In-Reply-To: <20140604115246.GB11073@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC 1/5] nbd: Correct name comparison for export_set_name() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Paolo Bonzini , Fam Zheng , qemu-devel@nongnu.org, Stefan Hajnoczi On 04.06.2014 13:52, Stefan Hajnoczi wrote: > On Sat, May 31, 2014 at 08:43:08PM +0200, Max Reitz wrote: >> exp->name == name is certainly true if both strings are equal and will >> work for both of them being NULL (which is important to check here); >> however, the strings may also be equal without having the same address, >> in which case there is no need to replace the export's name either. >> Therefore, add a check for this case. >> >> Signed-off-by: Max Reitz >> --- >> nbd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/nbd.c b/nbd.c >> index e5084b6..0787cba 100644 >> --- a/nbd.c >> +++ b/nbd.c >> @@ -832,7 +832,7 @@ NBDExport *nbd_export_find(const char *name) >> >> void nbd_export_set_name(NBDExport *exp, const char *name) >> { >> - if (exp->name == name) { >> + if (exp->name == name || (exp->name && name && !strcmp(exp->name, name))) { >> return; >> } > It's not clear to me why we even bother. The function is idempotent and > there are only 2 call sites in QEMU. This is not a performance-critical > function where it helps to bail early. You're probably right. I just happened to stumble over this code when looking into NBD. > Can we just drop the if statement completely? Probably, yes, but then again, I think it's not worth bothering with dropping it, either. ;-) Max > void nbd_export_set_name(NBDExport *exp, const char *name) > { > if (exp->name == name) { > return; > } > > nbd_export_get(exp); > if (exp->name != NULL) { > g_free(exp->name); > exp->name = NULL; > QTAILQ_REMOVE(&exports, exp, next); > nbd_export_put(exp); > } > if (name != NULL) { > nbd_export_get(exp); > exp->name = g_strdup(name); > QTAILQ_INSERT_TAIL(&exports, exp, next); > } > nbd_export_put(exp); > }