Message ID | 1401561792-13410-2-git-send-email-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
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 <mreitz@redhat.com> > --- > 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. Can we just drop the if statement completely? 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); }
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 <mreitz@redhat.com> >> --- >> 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); > }
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; }
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 <mreitz@redhat.com> --- nbd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)