Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
Linux centric. Provide stubs for non-Linux platforms.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/util/virprocess.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index c74bd16fe6..5788faea9c 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
}
+#ifdef __linux__
int
virProcessGetStatInfo(unsigned long long *cpuTime,
int *lastCpu,
@@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
return 0;
}
+
+#else
+int
+virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
+ int *lastCpu G_GNUC_UNUSED,
+ long *vm_rss G_GNUC_UNUSED,
+ pid_t pid G_GNUC_UNUSED,
+ pid_t tid G_GNUC_UNUSED)
+{
+ errno = ENOSYS;
+ return -1;
+}
+
+int
+virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
+ pid_t pid G_GNUC_UNUSED,
+ pid_t tid G_GNUC_UNUSED)
+{
+ virReportSystemError(ENOSYS, "%s",
+ _("scheduler information is not supported on this platform"));
+ return -1;
+}
+#endif /* __linux__ */
--
2.34.1
On Fri, 7 Jan 2022, Michal Privoznik wrote:
> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
> Linux centric. Provide stubs for non-Linux platforms.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/util/virprocess.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index c74bd16fe6..5788faea9c 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
> }
>
>
> +#ifdef __linux__
> int
> virProcessGetStatInfo(unsigned long long *cpuTime,
> int *lastCpu,
> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
>
> return 0;
> }
> +
> +#else
> +int
> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
> + int *lastCpu G_GNUC_UNUSED,
> + long *vm_rss G_GNUC_UNUSED,
> + pid_t pid G_GNUC_UNUSED,
> + pid_t tid G_GNUC_UNUSED)
> +{
> + errno = ENOSYS;
> + return -1;
> +}
> +
> +int
> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
> + pid_t pid G_GNUC_UNUSED,
> + pid_t tid G_GNUC_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("scheduler information is not supported on this platform"));
We should do this for both functions for non-linux platforms or not do it
at all. Like it should be consistent IMHO.
> + return -1;
> +}
> +#endif /* __linux__ */
> --
> 2.34.1
>
>
On 1/7/22 09:05, Ani Sinha wrote:
>
>
> On Fri, 7 Jan 2022, Michal Privoznik wrote:
>
>> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
>> Linux centric. Provide stubs for non-Linux platforms.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/util/virprocess.c | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>> index c74bd16fe6..5788faea9c 100644
>> --- a/src/util/virprocess.c
>> +++ b/src/util/virprocess.c
>> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
>> }
>>
>>
>> +#ifdef __linux__
>> int
>> virProcessGetStatInfo(unsigned long long *cpuTime,
>> int *lastCpu,
>> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
>>
>> return 0;
>> }
>> +
>> +#else
>> +int
>> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
>> + int *lastCpu G_GNUC_UNUSED,
>> + long *vm_rss G_GNUC_UNUSED,
>> + pid_t pid G_GNUC_UNUSED,
>> + pid_t tid G_GNUC_UNUSED)
>> +{
>> + errno = ENOSYS;
>> + return -1;
>> +}
>> +
>> +int
>> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
>> + pid_t pid G_GNUC_UNUSED,
>> + pid_t tid G_GNUC_UNUSED)
>> +{
>> + virReportSystemError(ENOSYS, "%s",
>> + _("scheduler information is not supported on this platform"));
>
> We should do this for both functions for non-linux platforms or not do it
> at all. Like it should be consistent IMHO.
I don't think so. Just like we've discussed under one patch of yours, a
function should either report error in all cases or none. And in case of
virProcessGetSchedInfo() the linux version does report error hence the
non-linux variant should report an error too. And in case of
virProcessGetStatInfo() no error is reported for linux version thus
non-linux version shouldn't report an error.
Think of it like this. You have a function that's called from somewhere.
However, the function has two (or more) implementations (e.g. one for
Linux, the other for FreeBSD, another one for MinGW, and so on). And you
call the function like this:
var = func();
if (!var) {
...
now should there be an error reported inside the if()? If all
implementations of the func() are consistent then there's a clear answer.
Hope this sheds more light.
Michal
On Fri, 7 Jan 2022, Michal Prívozník wrote:
> On 1/7/22 09:05, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Michal Privoznik wrote:
> >
> >> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
> >> Linux centric. Provide stubs for non-Linux platforms.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >> src/util/virprocess.c | 24 ++++++++++++++++++++++++
> >> 1 file changed, 24 insertions(+)
> >>
> >> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> >> index c74bd16fe6..5788faea9c 100644
> >> --- a/src/util/virprocess.c
> >> +++ b/src/util/virprocess.c
> >> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
> >> }
> >>
> >>
> >> +#ifdef __linux__
> >> int
> >> virProcessGetStatInfo(unsigned long long *cpuTime,
> >> int *lastCpu,
> >> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
> >>
> >> return 0;
> >> }
> >> +
> >> +#else
> >> +int
> >> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
> >> + int *lastCpu G_GNUC_UNUSED,
> >> + long *vm_rss G_GNUC_UNUSED,
> >> + pid_t pid G_GNUC_UNUSED,
> >> + pid_t tid G_GNUC_UNUSED)
> >> +{
> >> + errno = ENOSYS;
> >> + return -1;
> >> +}
> >> +
> >> +int
> >> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
> >> + pid_t pid G_GNUC_UNUSED,
> >> + pid_t tid G_GNUC_UNUSED)
> >> +{
> >> + virReportSystemError(ENOSYS, "%s",
> >> + _("scheduler information is not supported on this platform"));
> >
> > We should do this for both functions for non-linux platforms or not do it
> > at all. Like it should be consistent IMHO.
>
> I don't think so. Just like we've discussed under one patch of yours, a
> function should either report error in all cases or none. And in case of
> virProcessGetSchedInfo() the linux version does report error
I see your point but there is also a bug in that function - not all error
paths report errors. For example, !proc and !lines cases. We need to fix
that.
hence the
> non-linux variant should report an error too. And in case of
> virProcessGetStatInfo() no error is reported for linux version thus
> non-linux version shouldn't report an error.
>
No this is not my understanding from the discussion. What I understood is
that the lowest level of functions should always report error when an
error path is encountered. For example virFileReadAll() does this nicely.
Currently there is no error path in virProcessGetStatInfo() and it
uncondiotionally returns 0. For non-linux variant, I think it would be
correct to report an error.
Now having done that, we should also fix the callers so that the callers
are not overriding the narrower errors with broader ones.
> Think of it like this. You have a function that's called from somewhere.
> However, the function has two (or more) implementations (e.g. one for
> Linux, the other for FreeBSD, another one for MinGW, and so on). And you
> call the function like this:
>
> var = func();
> if (!var) {
> ...
>
> now should there be an error reported inside the if()? If all
> implementations of the func() are consistent then there's a clear answer.
>
> Hope this sheds more light.
>
> Michal
>
>
On 1/7/22 09:34, Ani Sinha wrote:
>
>
> On Fri, 7 Jan 2022, Michal Prívozník wrote:
>
>> On 1/7/22 09:05, Ani Sinha wrote:
>>>
>>>
>>> On Fri, 7 Jan 2022, Michal Privoznik wrote:
>>>
>>>> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
>>>> Linux centric. Provide stubs for non-Linux platforms.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>> src/util/virprocess.c | 24 ++++++++++++++++++++++++
>>>> 1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>>>> index c74bd16fe6..5788faea9c 100644
>>>> --- a/src/util/virprocess.c
>>>> +++ b/src/util/virprocess.c
>>>> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
>>>> }
>>>>
>>>>
>>>> +#ifdef __linux__
>>>> int
>>>> virProcessGetStatInfo(unsigned long long *cpuTime,
>>>> int *lastCpu,
>>>> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
>>>>
>>>> return 0;
>>>> }
>>>> +
>>>> +#else
>>>> +int
>>>> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
>>>> + int *lastCpu G_GNUC_UNUSED,
>>>> + long *vm_rss G_GNUC_UNUSED,
>>>> + pid_t pid G_GNUC_UNUSED,
>>>> + pid_t tid G_GNUC_UNUSED)
>>>> +{
>>>> + errno = ENOSYS;
>>>> + return -1;
>>>> +}
>>>> +
>>>> +int
>>>> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
>>>> + pid_t pid G_GNUC_UNUSED,
>>>> + pid_t tid G_GNUC_UNUSED)
>>>> +{
>>>> + virReportSystemError(ENOSYS, "%s",
>>>> + _("scheduler information is not supported on this platform"));
>>>
>>> We should do this for both functions for non-linux platforms or not do it
>>> at all. Like it should be consistent IMHO.
>>
>> I don't think so. Just like we've discussed under one patch of yours, a
>> function should either report error in all cases or none. And in case of
>> virProcessGetSchedInfo() the linux version does report error
>
> I see your point but there is also a bug in that function - not all error
> paths report errors. For example, !proc and !lines cases. We need to fix
> that.
>
D'oh! I knew I forgot something in the other patch I've sent today:
https://listman.redhat.com/archives/libvir-list/2022-January/msg00270.html
g_strsplit() won't return NULL on sensible inputs (i.e. !NULL). Let me
rerun my spatch and post another patch.
> hence the
>> non-linux variant should report an error too. And in case of
>> virProcessGetStatInfo() no error is reported for linux version thus
>> non-linux version shouldn't report an error.
>>
>
> No this is not my understanding from the discussion. What I understood is
> that the lowest level of functions should always report error when an
> error path is encountered. For example virFileReadAll() does this nicely.
> Currently there is no error path in virProcessGetStatInfo() and it
> uncondiotionally returns 0. For non-linux variant, I think it would be
> correct to report an error.
>
Yes and no. Reporting error where it happens is okay most of the time
[1]. But having a function that's consistent in reporting or not
reporting an error in all cases is above that. I'm failing to see how
one can write effective code if a function reports error in some cases
but it doesn't in others. Especially if you want to report less generic
errors and more specific ones.
1: In a few cases it may be worth reporting more generic error because
it may be more user friendly and thus shed more light into what's going
on. But this situation is very rare.
> Now having done that, we should also fix the callers so that the callers
> are not overriding the narrower errors with broader ones.
Agreed, see my reply to the patch you posted.
Michal
On a Friday in 2022, Ani Sinha wrote: >On Fri, 7 Jan 2022, Michal Prívozník wrote: >> I don't think so. Just like we've discussed under one patch of yours, a >> function should either report error in all cases or none. And in case of >> virProcessGetSchedInfo() the linux version does report error > >I see your point but there is also a bug in that function - not all error >paths report errors. For example, !proc and !lines cases. We need to fix >that. > I don't see a !proc error path in virProcessGetSchedInfo. The !lines case is inconsistent but thankfully it can only happen if /proc/%d/sched is empty. >hence the >> non-linux variant should report an error too. And in case of >> virProcessGetStatInfo() no error is reported for linux version thus >> non-linux version shouldn't report an error. >> > >No this is not my understanding from the discussion. What I understood is >that the lowest level of functions should always report error when an >error path is encountered. Errors should be reported by the function that has the context to provide a helpful error message. For some low-level helpers, we rely on errno's and let the caller report a less-specific error - either out of laziness because (like above) there would have to be a buggy kernel, or because most of the code paths are syscalls which set errno. Jano >For example virFileReadAll() does this nicely. >Currently there is no error path in virProcessGetStatInfo() and it >uncondiotionally returns 0. For non-linux variant, I think it would be >correct to report an error. > >Now having done that, we should also fix the callers so that the callers >are not overriding the narrower errors with broader ones.
On Fri, 7 Jan 2022, Ján Tomko wrote:
> On a Friday in 2022, Ani Sinha wrote:
> > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > I don't think so. Just like we've discussed under one patch of yours, a
> > > function should either report error in all cases or none. And in case of
> > > virProcessGetSchedInfo() the linux version does report error
> >
> > I see your point but there is also a bug in that function - not all error
> > paths report errors. For example, !proc and !lines cases. We need to fix
> > that.
> >
>
> I don't see a !proc error path in virProcessGetSchedInfo.
>
if (tid)
proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
tid);
else
proc = g_strdup_printf("/proc/%d/sched", (int) pid);
if (!proc)
return -1; <=== not reported
> The !lines case is inconsistent but thankfully it can only happen
> if /proc/%d/sched is empty.
>
> > hence the
> > > non-linux variant should report an error too. And in case of
> > > virProcessGetStatInfo() no error is reported for linux version thus
> > > non-linux version shouldn't report an error.
> > >
> >
> > No this is not my understanding from the discussion. What I understood is
> > that the lowest level of functions should always report error when an
> > error path is encountered.
>
> Errors should be reported by the function that has the context to
> provide a helpful error message.
Correct. Most often it is the lowest level functions.
For some low-level helpers, we rely
> on errno's and let the caller report a less-specific error - either
> out of laziness because (like above) there would have to be a buggy
> kernel, or because most of the code paths are syscalls which set errno.
>
> Jano
>
> > For example virFileReadAll() does this nicely.
> > Currently there is no error path in virProcessGetStatInfo() and it
> > uncondiotionally returns 0. For non-linux variant, I think it would be
> > correct to report an error.
> >
> > Now having done that, we should also fix the callers so that the callers
> > are not overriding the narrower errors with broader ones.
>
On a Friday in 2022, Ani Sinha wrote:
>
>
>On Fri, 7 Jan 2022, Ján Tomko wrote:
>
>> On a Friday in 2022, Ani Sinha wrote:
>> > On Fri, 7 Jan 2022, Michal Prívozník wrote:
>> > > I don't think so. Just like we've discussed under one patch of yours, a
>> > > function should either report error in all cases or none. And in case of
>> > > virProcessGetSchedInfo() the linux version does report error
>> >
>> > I see your point but there is also a bug in that function - not all error
>> > paths report errors. For example, !proc and !lines cases. We need to fix
>> > that.
>> >
>>
>> I don't see a !proc error path in virProcessGetSchedInfo.
>>
>
> if (tid)
> proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
>tid);
> else
> proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> if (!proc)
> return -1; <=== not reported
>
Oh, I did not realize that I had Michal's patch that removes it applied locally:
https://listman.redhat.com/archives/libvir-list/2022-January/msg00270.html
Jano
On Fri, 7 Jan 2022, Ján Tomko wrote:
> On a Friday in 2022, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Ján Tomko wrote:
> >
> > > On a Friday in 2022, Ani Sinha wrote:
> > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > I don't think so. Just like we've discussed under one patch of yours,
> > > a
> > > > > function should either report error in all cases or none. And in case
> > > of
> > > > > virProcessGetSchedInfo() the linux version does report error
> > > >
> > > > I see your point but there is also a bug in that function - not all
> > > error
> > > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > > that.
> > > >
> > >
> > > I don't see a !proc error path in virProcessGetSchedInfo.
> > >
> >
> > if (tid)
> > proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> > tid);
> > else
> > proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> > if (!proc)
> > return -1; <=== not reported
> >
>
> Oh, I did not realize that I had Michal's patch that removes it applied
> locally:
> https://listman.redhat.com/archives/libvir-list/2022-January/msg00270.html
Ah ok, all makes sense now.
On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
>
>
> On Fri, 7 Jan 2022, Ján Tomko wrote:
>
> > On a Friday in 2022, Ani Sinha wrote:
> > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > I don't think so. Just like we've discussed under one patch of yours, a
> > > > function should either report error in all cases or none. And in case of
> > > > virProcessGetSchedInfo() the linux version does report error
> > >
> > > I see your point but there is also a bug in that function - not all error
> > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > that.
> > >
> >
> > I don't see a !proc error path in virProcessGetSchedInfo.
> >
>
> if (tid)
> proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> tid);
> else
> proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> if (!proc)
> return -1; <=== not reported
g_strdup_printf can't fail unless we mangled the printf format string
(which we havent), so that 'if (!proc)' check is redundant / unreachable
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 :|
On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:
> On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Ján Tomko wrote:
> >
> > > On a Friday in 2022, Ani Sinha wrote:
> > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > I don't think so. Just like we've discussed under one patch of yours, a
> > > > > function should either report error in all cases or none. And in case of
> > > > > virProcessGetSchedInfo() the linux version does report error
> > > >
> > > > I see your point but there is also a bug in that function - not all error
> > > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > > that.
> > > >
> > >
> > > I don't see a !proc error path in virProcessGetSchedInfo.
> > >
> >
> > if (tid)
> > proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> > tid);
> > else
> > proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> > if (!proc)
> > return -1; <=== not reported
>
> g_strdup_printf can't fail unless we mangled the printf format string
> (which we havent), so that 'if (!proc)' check is redundant / unreachable
>
I am ok with Michal's patch that removes this check. However, such
assumptutions does makes me nervous. Since we do not really have
control over the library that implements g_strdup_printf, what if the
"can't fail" logic changes one day? This can happen deliberately or due to
some bug in the library. Does it not make sense to be defensive as the
consumer of this function and write code that is future proof?
On Fri, Jan 07, 2022 at 06:04:04PM +0530, Ani Sinha wrote:
>
>
> On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:
>
> > On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
> > >
> > >
> > > On Fri, 7 Jan 2022, Ján Tomko wrote:
> > >
> > > > On a Friday in 2022, Ani Sinha wrote:
> > > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > > I don't think so. Just like we've discussed under one patch of yours, a
> > > > > > function should either report error in all cases or none. And in case of
> > > > > > virProcessGetSchedInfo() the linux version does report error
> > > > >
> > > > > I see your point but there is also a bug in that function - not all error
> > > > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > > > that.
> > > > >
> > > >
> > > > I don't see a !proc error path in virProcessGetSchedInfo.
> > > >
> > >
> > > if (tid)
> > > proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> > > tid);
> > > else
> > > proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> > > if (!proc)
> > > return -1; <=== not reported
> >
> > g_strdup_printf can't fail unless we mangled the printf format string
> > (which we havent), so that 'if (!proc)' check is redundant / unreachable
> >
>
> I am ok with Michal's patch that removes this check. However, such
> assumptutions does makes me nervous. Since we do not really have
> control over the library that implements g_strdup_printf, what if the
> "can't fail" logic changes one day? This can happen deliberately or due to
> some bug in the library. Does it not make sense to be defensive as the
> consumer of this function and write code that is future proof?
Their API spec guarantees we are safe
https://docs.gtk.org/glib/func.strdup_printf.html
"The returned string is guaranteed to be non-NULL, unless format contains %lc or %ls conversions, which can fail if no multibyte representation is available for the given character."
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 :|
On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:
> On Fri, Jan 07, 2022 at 06:04:04PM +0530, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:
> >
> > > On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
> > > >
> > > >
> > > > On Fri, 7 Jan 2022, Ján Tomko wrote:
> > > >
> > > > > On a Friday in 2022, Ani Sinha wrote:
> > > > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > > > I don't think so. Just like we've discussed under one patch of yours, a
> > > > > > > function should either report error in all cases or none. And in case of
> > > > > > > virProcessGetSchedInfo() the linux version does report error
> > > > > >
> > > > > > I see your point but there is also a bug in that function - not all error
> > > > > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > > > > that.
> > > > > >
> > > > >
> > > > > I don't see a !proc error path in virProcessGetSchedInfo.
> > > > >
> > > >
> > > > if (tid)
> > > > proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> > > > tid);
> > > > else
> > > > proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> > > > if (!proc)
> > > > return -1; <=== not reported
> > >
> > > g_strdup_printf can't fail unless we mangled the printf format string
> > > (which we havent), so that 'if (!proc)' check is redundant / unreachable
> > >
> >
> > I am ok with Michal's patch that removes this check. However, such
> > assumptutions does makes me nervous. Since we do not really have
> > control over the library that implements g_strdup_printf, what if the
> > "can't fail" logic changes one day? This can happen deliberately or due to
> > some bug in the library. Does it not make sense to be defensive as the
> > consumer of this function and write code that is future proof?
>
> Their API spec guarantees we are safe
>
> https://docs.gtk.org/glib/func.strdup_printf.html
>
> "The returned string is guaranteed to be non-NULL, unless format contains %lc or %ls conversions, which can fail if no multibyte representation is available for the given character."
>
Ok fine but still, life is not ideal ... libraries do have bugs.
On 1/7/22 13:38, Ani Sinha wrote: > > > Ok fine but still, life is not ideal ... libraries do have bugs. In that case, where do we draw the line? Say pthread has a bug and it doesn't spawn threads. Worse, it doesn't even return any error value. Should we mitigate that too? I'd say stick with what's documented (abort on OOM and/or !NULL returned) and make our lives simpler. In fact, if we'd crash because we accessed NULL we will immediately see where and why and can report the bug to glib for benefit of us and others. Michal
On Fri, 7 Jan 2022, Michal Prívozník wrote: > On 1/7/22 13:38, Ani Sinha wrote: > > > > > > > Ok fine but still, life is not ideal ... libraries do have bugs. > > In that case, where do we draw the line? Say pthread has a bug and it > doesn't spawn threads. Worse, it doesn't even return any error value. > Should we mitigate that too? > > I'd say stick with what's documented (abort on OOM and/or !NULL > returned) and make our lives simpler. In fact, if we'd crash because we > accessed NULL we will immediately see where and why and can report the > bug to glib for benefit of us and others. Yes, so if I were you, I would not simply remove the check. I would replace it with assert().
On a Friday in 2022, Michal Privoznik wrote: >Both virProcessGetStatInfo() and virProcessGetSchedInfo() are >Linux centric. Provide stubs for non-Linux platforms. > >Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >--- > src/util/virprocess.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Fri, Jan 07, 2022 at 08:55:16 +0100, Michal Privoznik wrote: > Both virProcessGetStatInfo() and virProcessGetSchedInfo() are > Linux centric. Provide stubs for non-Linux platforms. Fixes: d73852c49962fbfe652fc7bec595a3f242faf847 > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > src/util/virprocess.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) IMO qualifies for build-breaker rule: https://gitlab.com/libvirt/libvirt/-/jobs/1950755381 Reviewed-by: Peter Krempa <pkrempa@redhat.com>
© 2016 - 2024 Red Hat, Inc.