Convert the string duplication APIs to use the g_strdup family of APIs.
Annoyingly our virVasprintf/virAsprintf functions return the character
count, even though 90% of our usage doesn't need it. To retain compat
with these semantics we have a call to strlen which costs CPU time.
We previously used the 'strdup-posix' gnulib module because mingw does
not set errno to ENOMEM on failure
We previously used the 'strndup' gnulib module because this function
does not exist on mingw.
We previously used the 'vasprintf' gnulib module because of many GNU
supported format specifiers not working on non-Linux platforms. glib's
own equivalent standardizes on GNU format specifiers too.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
bootstrap.conf | 3 ---
src/util/virstring.c | 19 +++++++------------
2 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/bootstrap.conf b/bootstrap.conf
index 549d18c6d4..b6b75f9301 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -100,8 +100,6 @@ stat-time
stdarg
stpcpy
strchrnul
-strdup-posix
-strndup
strerror
strerror_r-posix
strptime
@@ -117,7 +115,6 @@ uname
unsetenv
useless-if-before-free
usleep
-vasprintf
verify
vc-list-files
vsnprintf
diff --git a/src/util/virstring.c b/src/util/virstring.c
index a4cc7e9c0a..c8c888b2a0 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -730,12 +730,9 @@ virVasprintfInternal(char **strp,
const char *fmt,
va_list list)
{
- int ret;
+ *strp = g_strdup_vprintf(fmt, list);
- if ((ret = vasprintf(strp, fmt, list)) == -1)
- abort();
-
- return ret;
+ return strlen(*strp);
}
int
@@ -743,12 +740,12 @@ virAsprintfInternal(char **strp,
const char *fmt, ...)
{
va_list ap;
- int ret;
va_start(ap, fmt);
- ret = virVasprintfInternal(strp, fmt, ap);
+ *strp = g_strdup_vprintf(fmt, ap);
va_end(ap);
- return ret;
+
+ return strlen(*strp);
}
/**
@@ -936,8 +933,7 @@ virStrdup(char **dest,
*dest = NULL;
if (!src)
return 0;
- if (!(*dest = strdup(src)))
- abort();
+ *dest = g_strdup(src);
return 1;
}
@@ -965,8 +961,7 @@ virStrndup(char **dest,
return 0;
if (n < 0)
n = strlen(src);
- if (!(*dest = strndup(src, n)))
- abort();
+ *dest = g_strndup(src, n);
return 1;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn Fri, Sep 27, 2019 at 18:17:27 +0100, Daniel Berrange wrote: > Convert the string duplication APIs to use the g_strdup family of APIs. > > Annoyingly our virVasprintf/virAsprintf functions return the character > count, even though 90% of our usage doesn't need it. To retain compat > with these semantics we have a call to strlen which costs CPU time. I'd rather refactor the handful of cases which care about the formatted length and drop the strlens. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 30, 2019 at 10:37:53AM +0200, Peter Krempa wrote: >On Fri, Sep 27, 2019 at 18:17:27 +0100, Daniel Berrange wrote: >> Convert the string duplication APIs to use the g_strdup family of APIs. >> >> Annoyingly our virVasprintf/virAsprintf functions return the character >> count, even though 90% of our usage doesn't need it. To retain compat The actual percentage is way closer to 100%, the only function that actually uses the return value of virAsprintf is virNWFilterSnoopLeaseFileWrite. And the only other function needing adjustment is libxlDomainCleanup where we check for > 0 instead of >= 0 like in other places. Also, making virAsprintf return 0/-1 will actually fix the return value of virLogSetDefaultOutput to match its documentation. Jano >> with these semantics we have a call to strlen which costs CPU time. > >I'd rather refactor the handful of cases which care about the formatted >length and drop the strlens. > >-- >libvir-list mailing list >libvir-list@redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 30, 2019 at 01:32:50PM +0200, Ján Tomko wrote: > On Mon, Sep 30, 2019 at 10:37:53AM +0200, Peter Krempa wrote: > > On Fri, Sep 27, 2019 at 18:17:27 +0100, Daniel Berrange wrote: > > > Convert the string duplication APIs to use the g_strdup family of APIs. > > > > > > Annoyingly our virVasprintf/virAsprintf functions return the character > > > count, even though 90% of our usage doesn't need it. To retain compat > > The actual percentage is way closer to 100%, the only function that > actually uses the return value of virAsprintf is virNWFilterSnoopLeaseFileWrite. Something inthe test suite uses one of these functions, because when I did have it returning 0, the test suite never printed any output at all :-) Given the huge number of calls I didn't want to check them all for bugs. > And the only other function needing adjustment is libxlDomainCleanup > where we check for > 0 instead of >= 0 like in other places. > > Also, making virAsprintf return 0/-1 will actually fix the return value > of virLogSetDefaultOutput to match its documentation. > > Jano > > > > with these semantics we have a call to strlen which costs CPU time. > > > > I'd rather refactor the handful of cases which care about the formatted > > length and drop the strlens. I'd certainly like to Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Sep 27, 2019 at 06:17:27PM +0100, Daniel P. Berrangé wrote:
> Convert the string duplication APIs to use the g_strdup family of APIs.
>
> Annoyingly our virVasprintf/virAsprintf functions return the character
> count, even though 90% of our usage doesn't need it. To retain compat
> with these semantics we have a call to strlen which costs CPU time.
>
> We previously used the 'strdup-posix' gnulib module because mingw does
> not set errno to ENOMEM on failure
>
> We previously used the 'strndup' gnulib module because this function
> does not exist on mingw.
>
> We previously used the 'vasprintf' gnulib module because of many GNU
> supported format specifiers not working on non-Linux platforms. glib's
> own equivalent standardizes on GNU format specifiers too.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> bootstrap.conf | 3 ---
> src/util/virstring.c | 19 +++++++------------
> 2 files changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/bootstrap.conf b/bootstrap.conf
> index 549d18c6d4..b6b75f9301 100644
> --- a/bootstrap.conf
> +++ b/bootstrap.conf
> @@ -100,8 +100,6 @@ stat-time
> stdarg
> stpcpy
> strchrnul
> -strdup-posix
> -strndup
> strerror
> strerror_r-posix
> strptime
> @@ -117,7 +115,6 @@ uname
> unsetenv
> useless-if-before-free
> usleep
> -vasprintf
> verify
> vc-list-files
> vsnprintf
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index a4cc7e9c0a..c8c888b2a0 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -730,12 +730,9 @@ virVasprintfInternal(char **strp,
> const char *fmt,
> va_list list)
> {
> - int ret;
> + *strp = g_strdup_vprintf(fmt, list);
>
> - if ((ret = vasprintf(strp, fmt, list)) == -1)
> - abort();
> -
> - return ret;
> + return strlen(*strp);
This will cause a SEGFAULT if strp is NULL as g_strdup_vprintf doesn't
abort on failure.
We can use g_vasprintf which returns length.
But if we want to return only -1 or 0 and let the caller to decide on
the length there are only few places to modify.
src/nwfilter/nwfilter_dhcpsnoop.c:1770
src/util/virfile.c:3410
These two looks like the only cases where we actually care about the
length. There are some other cases for which we would have to only
tweak to comparison:
src/libxl/libxl_domain.c:916:
There is a function virDoubleToStr that returns the length but it's
usage doesn't care about the length so we would have to change the
description.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 30, 2019 at 01:35:36PM +0200, Pavel Hrdina wrote:
> On Fri, Sep 27, 2019 at 06:17:27PM +0100, Daniel P. Berrangé wrote:
> > Convert the string duplication APIs to use the g_strdup family of APIs.
> >
> > Annoyingly our virVasprintf/virAsprintf functions return the character
> > count, even though 90% of our usage doesn't need it. To retain compat
> > with these semantics we have a call to strlen which costs CPU time.
> >
> > We previously used the 'strdup-posix' gnulib module because mingw does
> > not set errno to ENOMEM on failure
> >
> > We previously used the 'strndup' gnulib module because this function
> > does not exist on mingw.
> >
> > We previously used the 'vasprintf' gnulib module because of many GNU
> > supported format specifiers not working on non-Linux platforms. glib's
> > own equivalent standardizes on GNU format specifiers too.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > bootstrap.conf | 3 ---
> > src/util/virstring.c | 19 +++++++------------
> > 2 files changed, 7 insertions(+), 15 deletions(-)
> >
> > diff --git a/bootstrap.conf b/bootstrap.conf
> > index 549d18c6d4..b6b75f9301 100644
> > --- a/bootstrap.conf
> > +++ b/bootstrap.conf
> > @@ -100,8 +100,6 @@ stat-time
> > stdarg
> > stpcpy
> > strchrnul
> > -strdup-posix
> > -strndup
> > strerror
> > strerror_r-posix
> > strptime
> > @@ -117,7 +115,6 @@ uname
> > unsetenv
> > useless-if-before-free
> > usleep
> > -vasprintf
> > verify
> > vc-list-files
> > vsnprintf
> > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > index a4cc7e9c0a..c8c888b2a0 100644
> > --- a/src/util/virstring.c
> > +++ b/src/util/virstring.c
> > @@ -730,12 +730,9 @@ virVasprintfInternal(char **strp,
> > const char *fmt,
> > va_list list)
> > {
> > - int ret;
> > + *strp = g_strdup_vprintf(fmt, list);
> >
> > - if ((ret = vasprintf(strp, fmt, list)) == -1)
> > - abort();
> > -
> > - return ret;
> > + return strlen(*strp);
>
> This will cause a SEGFAULT if strp is NULL as g_strdup_vprintf doesn't
> abort on failure.
>
> We can use g_vasprintf which returns length.
Oh yes, that makes life easier.
> But if we want to return only -1 or 0 and let the caller to decide on
> the length there are only few places to modify.
>
> src/nwfilter/nwfilter_dhcpsnoop.c:1770
> src/util/virfile.c:3410
>
> These two looks like the only cases where we actually care about the
> length. There are some other cases for which we would have to only
> tweak to comparison:
>
> src/libxl/libxl_domain.c:916:
>
> There is a function virDoubleToStr that returns the length but it's
> usage doesn't care about the length so we would have to change the
> description.
There something else hiding as we break all test suite output
if we return 0
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 30, 2019 at 12:42:56PM +0100, Daniel P. Berrangé wrote:
>On Mon, Sep 30, 2019 at 01:35:36PM +0200, Pavel Hrdina wrote:
>> On Fri, Sep 27, 2019 at 06:17:27PM +0100, Daniel P. Berrangé wrote:
>> > Convert the string duplication APIs to use the g_strdup family of APIs.
>> >
>> > Annoyingly our virVasprintf/virAsprintf functions return the character
>> > count, even though 90% of our usage doesn't need it. To retain compat
>> > with these semantics we have a call to strlen which costs CPU time.
>> >
>> > We previously used the 'strdup-posix' gnulib module because mingw does
>> > not set errno to ENOMEM on failure
>> >
>> > We previously used the 'strndup' gnulib module because this function
>> > does not exist on mingw.
>> >
>> > We previously used the 'vasprintf' gnulib module because of many GNU
>> > supported format specifiers not working on non-Linux platforms. glib's
>> > own equivalent standardizes on GNU format specifiers too.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> > bootstrap.conf | 3 ---
>> > src/util/virstring.c | 19 +++++++------------
>> > 2 files changed, 7 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/bootstrap.conf b/bootstrap.conf
>> > index 549d18c6d4..b6b75f9301 100644
>> > --- a/bootstrap.conf
>> > +++ b/bootstrap.conf
>> > @@ -100,8 +100,6 @@ stat-time
>> > stdarg
>> > stpcpy
>> > strchrnul
>> > -strdup-posix
>> > -strndup
>> > strerror
>> > strerror_r-posix
>> > strptime
>> > @@ -117,7 +115,6 @@ uname
>> > unsetenv
>> > useless-if-before-free
>> > usleep
>> > -vasprintf
>> > verify
>> > vc-list-files
>> > vsnprintf
>> > diff --git a/src/util/virstring.c b/src/util/virstring.c
>> > index a4cc7e9c0a..c8c888b2a0 100644
>> > --- a/src/util/virstring.c
>> > +++ b/src/util/virstring.c
>> > @@ -730,12 +730,9 @@ virVasprintfInternal(char **strp,
>> > const char *fmt,
>> > va_list list)
>> > {
>> > - int ret;
>> > + *strp = g_strdup_vprintf(fmt, list);
>> >
>> > - if ((ret = vasprintf(strp, fmt, list)) == -1)
>> > - abort();
>> > -
>> > - return ret;
>> > + return strlen(*strp);
>>
>> This will cause a SEGFAULT if strp is NULL as g_strdup_vprintf doesn't
>> abort on failure.
>>
>> We can use g_vasprintf which returns length.
>
>Oh yes, that makes life easier.
>
>> But if we want to return only -1 or 0 and let the caller to decide on
>> the length there are only few places to modify.
>>
>> src/nwfilter/nwfilter_dhcpsnoop.c:1770
>> src/util/virfile.c:3410
[0]
>>
>> These two looks like the only cases where we actually care about the
>> length. There are some other cases for which we would have to only
>> tweak to comparison:
>>
>> src/libxl/libxl_domain.c:916:
>>
>> There is a function virDoubleToStr that returns the length but it's
>> usage doesn't care about the length so we would have to change the
>> description.
>
>There something else hiding as we break all test suite output
>if we return 0
>
Yes, that's the virFilePrintf function Pavel pointed out above [0]
Jano
>Regards,
>Daniel
>--
>|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org -o- https://fstop138.berrange.com :|
>|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 30, 2019 at 01:35:36PM +0200, Pavel Hrdina wrote:
> On Fri, Sep 27, 2019 at 06:17:27PM +0100, Daniel P. Berrangé wrote:
> > Convert the string duplication APIs to use the g_strdup family of APIs.
> >
> > Annoyingly our virVasprintf/virAsprintf functions return the character
> > count, even though 90% of our usage doesn't need it. To retain compat
> > with these semantics we have a call to strlen which costs CPU time.
> >
> > We previously used the 'strdup-posix' gnulib module because mingw does
> > not set errno to ENOMEM on failure
> >
> > We previously used the 'strndup' gnulib module because this function
> > does not exist on mingw.
> >
> > We previously used the 'vasprintf' gnulib module because of many GNU
> > supported format specifiers not working on non-Linux platforms. glib's
> > own equivalent standardizes on GNU format specifiers too.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > bootstrap.conf | 3 ---
> > src/util/virstring.c | 19 +++++++------------
> > 2 files changed, 7 insertions(+), 15 deletions(-)
> >
> > diff --git a/bootstrap.conf b/bootstrap.conf
> > index 549d18c6d4..b6b75f9301 100644
> > --- a/bootstrap.conf
> > +++ b/bootstrap.conf
> > @@ -100,8 +100,6 @@ stat-time
> > stdarg
> > stpcpy
> > strchrnul
> > -strdup-posix
> > -strndup
> > strerror
> > strerror_r-posix
> > strptime
> > @@ -117,7 +115,6 @@ uname
> > unsetenv
> > useless-if-before-free
> > usleep
> > -vasprintf
> > verify
> > vc-list-files
> > vsnprintf
> > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > index a4cc7e9c0a..c8c888b2a0 100644
> > --- a/src/util/virstring.c
> > +++ b/src/util/virstring.c
> > @@ -730,12 +730,9 @@ virVasprintfInternal(char **strp,
> > const char *fmt,
> > va_list list)
> > {
> > - int ret;
> > + *strp = g_strdup_vprintf(fmt, list);
> >
> > - if ((ret = vasprintf(strp, fmt, list)) == -1)
> > - abort();
> > -
> > - return ret;
> > + return strlen(*strp);
>
> This will cause a SEGFAULT if strp is NULL as g_strdup_vprintf doesn't
> abort on failure.
I spent a long time investigating this....
g_strdup_vprintf calls g_vasprintf() which in turn has 3 impls.
2 out of the 3 impls will abort on OOM, but one won't. The one
we use on Linux is the one that won't abort.
No application code that I can find ever checks the return value
of g_strdup_vprintf or the output string of g_vasprintf.
I eventually found a bug indicating the lack of abort on OOM is
indeed considered a mistake:
https://gitlab.gnome.org/GNOME/glib/issues/1622
I've thus sent a patch to force an abort on OOM:
https://gitlab.gnome.org/GNOME/glib/merge_requests/1145
Thus I think from libvirt's POV we can assume this aborts on OOM,
since every single other application using this does the same.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.