[libvirt] [PATCH] eliminate strerror from qemu_driver.c: use virReportSystemError instead

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> wrote:
> On Thu, Jan 29, 2009 at 09:54:59PM +0100, Jim Meyering wrote:
>> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
> You can actually just kill off the SetNonBlock method, since we
> added one to util.h. We should probably do same for SetCloseExec
> since I believe we have several copies of that with the sme
> problem too.

Ah.  I see that now.  Have done both things.

>> @@ -854,8 +842,7 @@ static int qemudWaitForMonitor(virConnectPtr conn,
>>                                   qemudFindCharDevicePTYs,
>>                                   "console", 3000);
>>      if (close(logfd) < 0)
>> -        qemudLog(QEMUD_WARN, _("Unable to close logfile: %s\n"),
>> -                 strerror(errno));
>> +        virReportSystemError(NULL, errno, "%s", _("Unable to close logfile"));
>>      return ret;
>>  }
>
> If you report an error in this scenario then you must also ensure
> a failure code is returned. This particular scenario is non-fatal

Why?  because of semantics implied by the "Error" suffix?
Then maybe we need a *Warn function with similar functionality.

> though, so I don't think we should be raising an error here at all.
> I'd be inclined to just keep qemudLog, but with the raw errno value
> as an int.

Why not?  If I get EIO or ENOSPC, I'd like to see diagnostics
in the logs ASAP.  The closer to the point of origin the better.

If we keep using qemudLog, then I'll just make it use virStrerror or
something along those lines.  there's no need to print raw errno values.

>> @@ -1134,30 +1121,30 @@ static int qemudStartVMDaemon(virConnectPtr conn,
>>      tmp = progenv;
>>      while (*tmp) {
>>          if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0)
>> -            qemudLog(QEMUD_WARN, _("Unable to write envv to logfile %d: %s\n"),
>> -                     errno, strerror(errno));
>> +            virReportSystemError(NULL, errno,
>> +                                 "%s", _("Unable to write envv to logfile"));
>>          if (safewrite(vm->logfile, " ", 1) < 0)
>> -            qemudLog(QEMUD_WARN, _("Unable to write envv to logfile %d: %s\n"),
>> -                     errno, strerror(errno));
>> +            virReportSystemError(NULL, errno,
>> +                                 "%s", _("Unable to write envv to logfile"));
>>          tmp++;
>>      }
>>      tmp = argv;
>>      while (*tmp) {
>>          if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0)
>> -            qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"),
>> -                     errno, strerror(errno));
>> +            virReportSystemError(NULL, errno,
>> +                                 "%s", _("Unable to write argv to logfile"));
>>          if (safewrite(vm->logfile, " ", 1) < 0)
>> -            qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"),
>> -                     errno, strerror(errno));
>> +            virReportSystemError(NULL, errno,
>> +                                 "%s", _("Unable to write argv to logfile"));
>>          tmp++;
>>      }
>>      if (safewrite(vm->logfile, "\n", 1) < 0)
>> -        qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s\n"),
>> -                 errno, strerror(errno));
>> +        virReportSystemError(NULL, errno,
>> +                             "%s", _("Unable to write argv to logfile"));
>>
>>      if ((pos = lseek(vm->logfile, 0, SEEK_END)) < 0)
>> -        qemudLog(QEMUD_WARN, _("Unable to seek to end of logfile %d: %s\n"),
>> -                 errno, strerror(errno));
>> +        virReportSystemError(NULL, errno,
>> +                             "%s", _("Unable to seek to end of logfile"));
>
> Likewise with all these - we're just printing ARGV to a logfile - this should
> not result in errors propagated to the caller - unless we intend to tear down
> the VM we just started, but I think that's overkill.

But failure still means write errors.  It doesn't matter
so much what we're writing as *that* there's been a relatively
serious (write) error. Not reporting a write error now could
easily mask or delay reporting a later one involving important data.

That said, I don't feel very strongly either way,
so tell me what you'd prefer.

> Hmm, these Log -> Error conversions all are same non-fatal scneario
> I mention earlier.
>
>> @@ -1492,7 +1480,7 @@ static int kvmGetMaxVCPUs(void) {
>>
>>      fd = open(KVM_DEVICE, O_RDONLY);
>>      if (fd < 0) {
>> -        qemudLog(QEMUD_WARN, _("Unable to open %s: %s\n"), KVM_DEVICE, strerror(errno));
>> +        virReportSystemError(NULL, errno, _("Unable to open %s"), KVM_DEVICE);
>>          return maxvcpus;
>>      }
>>
>
> This conversion looks good - we SHOULD be raising a real error
> here. The original code was wrong to return 'maxvcpus' here,
> it should be -1 upon error.

I've just converted another that I'll merge onto the rest:

>From d02669442e0cd2354e6b429dbedcf665d15cfa87 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Fri, 30 Jan 2009 23:18:04 +0100
Subject: [PATCH] report OOMError

---
 src/qemu_driver.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index c40fda4..599af0b 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -462,8 +462,7 @@ qemudStartup(void) {
     return 0;

 out_of_memory:
-    qemudLog (QEMUD_ERR,
-              "%s", _("qemudStartup: out of memory\n"));
+    virReportOOMError(NULL);
 error:
     if (qemu_driver)
         qemuDriverUnlock(qemu_driver);
--
1.6.1.2.418.gd79e6