[Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block

[Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block

[Alon Levy]

Used by the followin patch.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 include/qemu-common.h |  1 +
 util/oslib-posix.c    | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index cb82ef3..c24d75c 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -232,6 +232,7 @@ ssize_t qemu_recv_full(int fd, void *buf, size_t count, int flags)
 
 #ifndef _WIN32
 int qemu_pipe(int pipefd[2]);
+int qemu_pipe_non_block(int pipefd[2]);
 #endif
 
 #ifdef _WIN32
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 3dc8b1b..bc2ce2e 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -188,6 +188,25 @@ int qemu_pipe(int pipefd[2])
     return ret;
 }
 
+int qemu_pipe_non_block(int pipefd[2])
+{
+    int ret;
+
+    ret = qemu_pipe(pipefd);
+    if (ret) {
+        return ret;
+    }
+    if (fcntl(card->pipe[0], F_SETFL, O_NONBLOCK) == -1) {
+        return -errno;
+    }
+    if (fcntl(card->pipe[1], F_SETFL, O_NONBLOCK) == -1) {
+        return -errno;
+    }
+    if (fcntl(card->pipe[0], F_SETOWN, getpid()) == -1) {
+        return -errno;
+    }
+}
+
 int qemu_utimens(const char *path, const struct timespec *times)
 {
     struct timeval tv[2], tv_now;
-- 
1.8.2.1

[Qemu-devel] [PATCH 2/5] use qemu_pipe_non_block

[Alon Levy]

This fixes six instances of unchecked fcntl return status, spotted by
Coverity.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/display/qxl.c            | 10 +++-------
 hw/usb/ccid-card-emulated.c |  8 +++-----
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 9e5b7ad..25c8c5a 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1797,15 +1797,11 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
 
 static void init_pipe_signaling(PCIQXLDevice *d)
 {
-    if (pipe(d->pipe) < 0) {
-        fprintf(stderr, "%s:%s: qxl pipe creation failed\n",
-                __FILE__, __func__);
+    if (qxl_pipe_non_block(d->pipe)) {
+        fprintf(stderr, "%s:%s: qxl pipe creation failed: %s\n",
+                __FILE__, __func__, stderror(errno));
         exit(1);
     }
-    fcntl(d->pipe[0], F_SETFL, O_NONBLOCK);
-    fcntl(d->pipe[1], F_SETFL, O_NONBLOCK);
-    fcntl(d->pipe[0], F_SETOWN, getpid());
-
     qemu_thread_get_self(&d->main);
     qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d);
 }
diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index deb6d47..2e6942e 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -406,13 +406,11 @@ static void pipe_read(void *opaque)
 
 static int init_pipe_signaling(EmulatedState *card)
 {
-    if (pipe(card->pipe) < 0) {
-        DPRINTF(card, 2, "pipe creation failed\n");
+    if (qemu_pipe_non_block(card->pipe) < 0) {
+        DPRINTF(card, 2, "pipe creation failed: %s\n",
+                strerror(errno));
         return -1;
     }
-    fcntl(card->pipe[0], F_SETFL, O_NONBLOCK);
-    fcntl(card->pipe[1], F_SETFL, O_NONBLOCK);
-    fcntl(card->pipe[0], F_SETOWN, getpid());
     qemu_set_fd_handler(card->pipe[0], pipe_read, NULL, card);
     return 0;
 }
-- 
1.8.2.1

[Qemu-devel] [PATCH 3/5] libcacard/vscclient: fix leakage of socket on error paths

[Alon Levy]

Spotted by Coverity.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vscclient.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index ac23647..5180d29 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -618,18 +618,22 @@ connect_to_qemu(
     if (ret != 0) {
         /* Error */
         fprintf(stderr, "getaddrinfo failed\n");
-        return -1;
+        goto cleanup_socket;
     }
 
     if (connect(sock, server->ai_addr, server->ai_addrlen) < 0) {
         /* Error */
         fprintf(stderr, "Could not connect\n");
-        return -1;
+        goto cleanup_socket;
     }
     if (verbose) {
         printf("Connected (sizeof Header=%zd)!\n", sizeof(VSCMsgHeader));
     }
     return sock;
+
+cleanup_socket:
+    closesocket(sock);
+    return -1;
 }
 
 int
@@ -759,5 +763,6 @@ main(
     g_io_channel_unref(channel_socket);
     g_byte_array_unref(socket_to_send);
 
+    closesocket(sock);
     return 0;
 }
-- 
1.8.2.1

[Qemu-devel] [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference

[Alon Levy]

Reported by Coverity:

Error: FORWARD_NULL (CWE-476):
qemu-1.5.0/libcacard/vreader.c:267: cond_false: Condition "card == NULL", taking false branch
qemu-1.5.0/libcacard/vreader.c:269: if_end: End of if statement
qemu-1.5.0/libcacard/vreader.c:272: cond_false: Condition "apdu == NULL", taking false branch
qemu-1.5.0/libcacard/vreader.c:275: else_branch: Reached else branch
qemu-1.5.0/libcacard/vreader.c:280: cond_false: Condition "response", taking false branch
qemu-1.5.0/libcacard/vreader.c:284: if_end: End of if statement
qemu-1.5.0/libcacard/vreader.c:280: var_compare_op: Comparing "response" to null implies that "response" might be null.
qemu-1.5.0/libcacard/vreader.c:286: cond_true: Condition "card_status == VCARD_DONE", taking true branch
qemu-1.5.0/libcacard/vreader.c:287: cond_true: Condition "card_status == VCARD_DONE", taking true branch
qemu-1.5.0/libcacard/vreader.c:288: var_deref_op: Dereferencing null pointer "response".

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vreader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libcacard/vreader.c b/libcacard/vreader.c
index 5793d73..60eb43b 100644
--- a/libcacard/vreader.c
+++ b/libcacard/vreader.c
@@ -260,7 +260,7 @@ vreader_xfr_bytes(VReader *reader,
 {
     VCardAPDU *apdu;
     VCardResponse *response = NULL;
-    VCardStatus card_status;
+    VCardStatus card_status = VCARD_FAIL;
     unsigned short status;
     VCard *card = vreader_get_card(reader);
 
-- 
1.8.2.1

[Qemu-devel] [PATCH 5/5] libcacard/vscclient.c: fix use of uninitialized variable

[Alon Levy]

Found by Coverity.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 libcacard/vscclient.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 5180d29..4275c23 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -645,7 +645,7 @@ main(
     GIOChannel *channel_stdin;
     char *qemu_host;
     char *qemu_port;
-    VSCMsgHeader mhHeader;
+    VSCMsgHeader mhHeader = {0,};
 
     VCardEmulOptions *command_line_options = NULL;
 
-- 
1.8.2.1

Re: [Qemu-devel] [PATCH 2/5] use qemu_pipe_non_block

[Paolo Bonzini]

Il 04/06/2013 22:23, Alon Levy ha scritto:
> This fixes six instances of unchecked fcntl return status, spotted by
> Coverity.

I think we're just assuming that they cannot fail...  I don't think we
need the previous patch and this one, unless they help porting stuff to
Windows.

Paolo

Re: [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block

[Peter Maydell]

On 4 June 2013 21:23, Alon Levy <alevy@redhat.com> wrote:
>
> +int qemu_pipe_non_block(int pipefd[2])
> +{
> +    int ret;
> +
> +    ret = qemu_pipe(pipefd);
> +    if (ret) {
> +        return ret;
> +    }
> +    if (fcntl(card->pipe[0], F_SETFL, O_NONBLOCK) == -1) {
> +        return -errno;
> +    }
> +    if (fcntl(card->pipe[1], F_SETFL, O_NONBLOCK) == -1) {
> +        return -errno;
> +    }

    qemu_set_nonblock(card->pipe[0]);
    qemu_set_nonblock(card->pipe[1]);

> +    if (fcntl(card->pipe[0], F_SETOWN, getpid()) == -1) {
> +        return -errno;
> +    }

You should either just trust that the fcntl() succeeds
(as we do in qemu_set_block() and friends), or you need
to close the pipe fds on failure here.

> +}

You've forgotten to return anything at the end of the function.
(surprised the compiler didn't pick that up, maybe it's
one of the warnings that needs optimimisation turned on).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/5] use qemu_pipe_non_block

[Peter Maydell]

On 4 June 2013 21:23, Alon Levy <alevy@redhat.com> wrote:
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1797,15 +1797,11 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
>
>  static void init_pipe_signaling(PCIQXLDevice *d)
>  {
> -    if (pipe(d->pipe) < 0) {
> -        fprintf(stderr, "%s:%s: qxl pipe creation failed\n",
> -                __FILE__, __func__);
> +    if (qxl_pipe_non_block(d->pipe)) {

Surely this can't compile? -- this function doesn't exist.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/5] libcacard/vscclient: fix leakage of socket on error paths

[Peter Maydell]

On 4 June 2013 21:23, Alon Levy <alevy@redhat.com> wrote:
> Spotted by Coverity.
>
> Signed-off-by: Alon Levy <alevy@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH 2/5] use qemu_pipe_non_block

[Alon Levy]

> On 4 June 2013 21:23, Alon Levy <alevy@redhat.com> wrote:
> > --- a/hw/display/qxl.c
> > +++ b/hw/display/qxl.c
> > @@ -1797,15 +1797,11 @@ static void qxl_send_events(PCIQXLDevice *d,
> > uint32_t events)
> >
> >  static void init_pipe_signaling(PCIQXLDevice *d)
> >  {
> > -    if (pipe(d->pipe) < 0) {
> > -        fprintf(stderr, "%s:%s: qxl pipe creation failed\n",
> > -                __FILE__, __func__);
> > +    if (qxl_pipe_non_block(d->pipe)) {
> 
> Surely this can't compile? -- this function doesn't exist.

I am abusing my right to post to this list, sorry. I didn't actually try to compile.

> 
> thanks
> -- PMM
> 
> 

Re: [Qemu-devel] [PATCH 2/5] use qemu_pipe_non_block

[Alon Levy]

> Il 04/06/2013 22:23, Alon Levy ha scritto:
> > This fixes six instances of unchecked fcntl return status, spotted by
> > Coverity.
> 
> I think we're just assuming that they cannot fail...  I don't think we
> need the previous patch and this one, unless they help porting stuff to
> Windows.

This was purely to satisfy coverity, but I thought we would want to check fcntl return status? also, shouldn't we be looping if EINTR?

> 
> Paolo
> 
> 

Re: [Qemu-devel] [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference

[Peter Maydell]

On 4 June 2013 21:23, Alon Levy <alevy@redhat.com> wrote:
> Reported by Coverity:
>
> Error: FORWARD_NULL (CWE-476):
> qemu-1.5.0/libcacard/vreader.c:267: cond_false: Condition "card == NULL", taking false branch
> qemu-1.5.0/libcacard/vreader.c:269: if_end: End of if statement
> qemu-1.5.0/libcacard/vreader.c:272: cond_false: Condition "apdu == NULL", taking false branch
> qemu-1.5.0/libcacard/vreader.c:275: else_branch: Reached else branch
> qemu-1.5.0/libcacard/vreader.c:280: cond_false: Condition "response", taking false branch
> qemu-1.5.0/libcacard/vreader.c:284: if_end: End of if statement
> qemu-1.5.0/libcacard/vreader.c:280: var_compare_op: Comparing "response" to null implies that "response" might be null.
> qemu-1.5.0/libcacard/vreader.c:286: cond_true: Condition "card_status == VCARD_DONE", taking true branch
> qemu-1.5.0/libcacard/vreader.c:287: cond_true: Condition "card_status == VCARD_DONE", taking true branch
> qemu-1.5.0/libcacard/vreader.c:288: var_deref_op: Dereferencing null pointer "response".
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  libcacard/vreader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libcacard/vreader.c b/libcacard/vreader.c
> index 5793d73..60eb43b 100644
> --- a/libcacard/vreader.c
> +++ b/libcacard/vreader.c
> @@ -260,7 +260,7 @@ vreader_xfr_bytes(VReader *reader,
>  {
>      VCardAPDU *apdu;
>      VCardResponse *response = NULL;
> -    VCardStatus card_status;
> +    VCardStatus card_status = VCARD_FAIL;
>      unsigned short status;
>      VCard *card = vreader_get_card(reader);
>

This doesn't make any sense to me as a fix for the quoted coverity
issue. It's complaining because the function both checks
that response isn't NULL (line 280) and uses it without
checking (line 288). If your change makes it stop complaining
it's only because you've managed to confuse it.

You either need to:
 * assume that vcard_make_response() and vcard_process_apdu()
   both guarantee to set response to non-NULL, and drop the
   "if (response)" check
 * don't assume it, and handle NULL response consistently
   in this function (eg by changing the line 287 check to
   "if (card_status == VCARD_DONE && response)"
 * take some middle line, eg "response is guaranteed not to
   be NULL if and only if status is VCARD_DONE" and then
   consistently check card_status in both places.

Also, this sequence:
    assert(card_status == VCARD_DONE);
    if (card_status == VCARD_DONE) {

is nonsensical. Either we should assert that the status
is always DONE, or we have code to handle the DONE and not
DONE cases; not both.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/5] use qemu_pipe_non_block

[Peter Maydell]

On 4 June 2013 21:59, Alon Levy <alevy@redhat.com> wrote:
> shouldn't we be looping if EINTR?

Don't open that can of worms if you value your sanity ;-)

-- PMM

Re: [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1872 bytes --]

On 06/04/2013 02:23 PM, Alon Levy wrote:
> Used by the followin patch.

s/followin/following/

>  
> +int qemu_pipe_non_block(int pipefd[2])
> +{
> +    int ret;
> +
> +    ret = qemu_pipe(pipefd);

qemu_pipe() already uses pipe2() when available; it seems like it would
be nicer to use pipe2's O_NONBLOCK option directly in one syscall (where
supported) instead of having to make additional syscalls after the fact.
 Would it just be smarter to change the signature of qemu_pipe() to add
a bool block parameter, and then change the 5 existing callers to pass
false with your later patch in the series passing true, and do it
without creating a new wrapper?

> +    if (ret) {
> +        return ret;
> +    }
> +    if (fcntl(card->pipe[0], F_SETFL, O_NONBLOCK) == -1) {
> +        return -errno;
> +    }
> +    if (fcntl(card->pipe[1], F_SETFL, O_NONBLOCK) == -1) {
> +        return -errno;

Leaks fds.  If you're going to report error, then you must close the fds
already created.

> +    }
> +    if (fcntl(card->pipe[0], F_SETOWN, getpid()) == -1) {
> +        return -errno;

Same comment about fd leaks.

This part seems like a useful change, IF you plan on using SIGIO and
SIGURG signals; and it is something which pipe2() cannot optimize, so I
can see why you are adding a new function instead of changing
qemu_pipe() and adjust all its callers to pass an additional parameter.
 But are you really planning on using SIGIO/SIGURG?

Furthermore, this is undefined behavior.  According to POSIX, use of
F_SETOWN is only portable on sockets, not pipes.  It may work on Linux,
but you'll need to be aware of what it does on other platforms.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html
-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 5/5] libcacard/vscclient.c: fix use of uninitialized variable

[Peter Maydell]

On 4 June 2013 21:23, Alon Levy <alevy@redhat.com> wrote:
> Found by Coverity.
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  libcacard/vscclient.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
> index 5180d29..4275c23 100644
> --- a/libcacard/vscclient.c
> +++ b/libcacard/vscclient.c
> @@ -645,7 +645,7 @@ main(
>      GIOChannel *channel_stdin;
>      char *qemu_host;
>      char *qemu_port;
> -    VSCMsgHeader mhHeader;
> +    VSCMsgHeader mhHeader = {0,};

As far as I can see we only use this variable once:

    send_msg(VSC_Init, mhHeader.reader_id, &init, sizeof(init));

so wouldn't it be better just to directly pass a constant
"0" to the send_msg() call? Hiding a single uint32_t in
a struct seems a bit obscure.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block

[Alon Levy]

> On 06/04/2013 02:23 PM, Alon Levy wrote:
> > Used by the followin patch.
> 
> s/followin/following/

Thanks.

> 
> >  
> > +int qemu_pipe_non_block(int pipefd[2])
> > +{
> > +    int ret;
> > +
> > +    ret = qemu_pipe(pipefd);
> 
> qemu_pipe() already uses pipe2() when available; it seems like it would
> be nicer to use pipe2's O_NONBLOCK option directly in one syscall (where
> supported) instead of having to make additional syscalls after the fact.
>  Would it just be smarter to change the signature of qemu_pipe() to add
> a bool block parameter, and then change the 5 existing callers to pass
> false with your later patch in the series passing true, and do it
> without creating a new wrapper?

Answered below.

> 
> > +    if (ret) {
> > +        return ret;
> > +    }
> > +    if (fcntl(card->pipe[0], F_SETFL, O_NONBLOCK) == -1) {
> > +        return -errno;
> > +    }
> > +    if (fcntl(card->pipe[1], F_SETFL, O_NONBLOCK) == -1) {
> > +        return -errno;
> 
> Leaks fds.  If you're going to report error, then you must close the fds
> already created.

As Peter pointed out, I should not go here, so I'll drop these checks, instead doing naked fcntl calls, so no fd leak possible (no returns).

> 
> > +    }
> > +    if (fcntl(card->pipe[0], F_SETOWN, getpid()) == -1) {
> > +        return -errno;
> 
> Same comment about fd leaks.
> 
> This part seems like a useful change, IF you plan on using SIGIO and
> SIGURG signals; and it is something which pipe2() cannot optimize, so I
> can see why you are adding a new function instead of changing
> qemu_pipe() and adjust all its callers to pass an additional parameter.
>  But are you really planning on using SIGIO/SIGURG?

I don't plan on using those signals, so I'll add a parameter instead.

> 
> Furthermore, this is undefined behavior.  According to POSIX, use of
> F_SETOWN is only portable on sockets, not pipes.  It may work on Linux,
> but you'll need to be aware of what it does on other platforms.
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block

[Michael Tokarev]

05.06.2013 00:23, Alon Levy wrote:
> Used by the followin patch.
> 
> +int qemu_pipe_non_block(int pipefd[2]);

A nitpick.  I'd name it qemu_pipe_nonblock(), like O_NONBLOCK
is named, but that may be just me.

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block

[Michael Tokarev]

05.06.2013 00:23, Alon Levy wrote:

 [PATCH 1/5] oslib-posix: add qemu_pipe_non_block
 [PATCH 2/5] use qemu_pipe_non_block
 [PATCH 3/5] libcacard/vscclient: fix leakage of socket on error paths
 [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference
 [PATCH 5/5] libcacard/vscclient.c: fix use of uninitialized variable

So, what happened with this series?

>From the above 5 patches, only 3/5 (leakage of socket on error paths)
is ready to be applied.  Should I apply it now, or wait for the
respin of whole series?  (Or both?)

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block

[Alon Levy]

> 05.06.2013 00:23, Alon Levy wrote:
> 
>  [PATCH 1/5] oslib-posix: add qemu_pipe_non_block
>  [PATCH 2/5] use qemu_pipe_non_block
>  [PATCH 3/5] libcacard/vscclient: fix leakage of socket on error paths
>  [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference
>  [PATCH 5/5] libcacard/vscclient.c: fix use of uninitialized variable
> 
> So, what happened with this series?

I still plan to do it, but didn't get to it yet.

> 
> From the above 5 patches, only 3/5 (leakage of socket on error paths)
> is ready to be applied.  Should I apply it now, or wait for the
> respin of whole series?  (Or both?)

I'll be happy if you do. (Both don't actually make sense :)

> 
> Thanks,
> 
> /mjt
> 
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH 3/5] libcacard/vscclient: fix leakage of socket on error paths

[Michael Tokarev]

05.06.2013 00:23, Alon Levy wrote:
> --- a/libcacard/vscclient.c
> +++ b/libcacard/vscclient.c
> @@ -759,5 +763,6 @@ main(
>      g_io_channel_unref(channel_socket);
>      g_byte_array_unref(socket_to_send);
>  
> +    closesocket(sock);
>      return 0;
>  }

This one isn't really needed, -- there's no need to close
filedescriptors at the end of main().  I understand the
memory unref/free calls above it, to make valgrind and
glib trackers happy.  But it ofcourse does not hurt.

Besides, in all these error places it'd be really nice
to print the actual cause of the problem too - like
strerror(errno) or something like that.  Unfortunately we
had too many of these already in all parts of the code,
and it really is sometimes difficult to understand WHY
it fails without seeing the actual cause.  I'll send a
separate patch for that.

Thanks, applied to the trivial patches queue.

/mjt