[Libguestfs] [libnbd PATCH v2 4/5] copy: Pass in dummy variable rather than &errno to callback
Nir Soffer
nsoffer at redhat.com
Thu Feb 3 23:19:15 UTC 2022
On Thu, Feb 3, 2022 at 11:46 PM Richard W.M. Jones <rjones at redhat.com> wrote:
>
> On Thu, Feb 03, 2022 at 02:25:57PM -0600, Eric Blake wrote:
> > In several places where asynch handlers manually call the provided
> > nbd_completion_callback, the value of errno is indeterminate (for
> > example, in file-ops.c:file_asynch_read(), the previous call to
> > file_synch_read() already triggered exit() on error, but does not
> > guarantee what is left in errno on success). As the callback should
> > be paying attention to the value of *error (to be fixed in the next
> > patch), we are better off ensuring that we pass in a pointer to a
> > known-zero value; at which point, it is easier to use a dummy variable
> > on the stack than to mess around with errno and it's magic macro
> > expansion into a thread-local storage location.
> >
> > Note that several callsites then check if the callback returned -1,
> > and if so assume that the callback has caused errno to now have a sane
> > value to pass on to perror. In theory, the fact that we are no longer
> > passing in &errno means that if the callback assigns into *error but
> > did not otherwise affect errno, our perror call would no longer
> > reflect that value. But in practice, since the callback never
> > actually returned -1, nor even assigned into *error, the call to
> > perror is dead code; although I have chosen to defer that additional
> > cleanup to the next patch.
>
> And I guess another reason not to use &errno is that it could be
> updated by random system calls in the same thread, perhaps even after
> we have set it. It sounds like &errno would always be wrong.
I agree, using global (even if thread local) is risky.
>
> > copy/file-ops.c | 17 ++++++++++-------
> > copy/multi-thread-copying.c | 8 +++++---
> > copy/null-ops.c | 12 +++++++-----
> > 3 files changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/copy/file-ops.c b/copy/file-ops.c
> > index 84704341..e37a5014 100644
> > --- a/copy/file-ops.c
> > +++ b/copy/file-ops.c
> > @@ -1,5 +1,5 @@
> > /* NBD client library in userspace.
> > - * Copyright (C) 2020 Red Hat Inc.
> > + * Copyright (C) 2020-2022 Red Hat Inc.
> > *
> > * This library is free software; you can redistribute it and/or
> > * modify it under the terms of the GNU Lesser General Public
> > @@ -587,10 +587,11 @@ file_asynch_read (struct rw *rw,
> > struct command *command,
> > nbd_completion_callback cb)
> > {
> > + int dummy = 0;
> > +
> > file_synch_read (rw, slice_ptr (command->slice),
> > command->slice.len, command->offset);
> > - errno = 0;
> > - if (cb.callback (cb.user_data, &errno) == -1) {
> > + if (cb.callback (cb.user_data, &dummy) == -1) {
> > perror (rw->name);
> > exit (EXIT_FAILURE);
> > }
> > @@ -601,10 +602,11 @@ file_asynch_write (struct rw *rw,
> > struct command *command,
> > nbd_completion_callback cb)
> > {
> > + int dummy = 0;
> > +
> > file_synch_write (rw, slice_ptr (command->slice),
> > command->slice.len, command->offset);
> > - errno = 0;
> > - if (cb.callback (cb.user_data, &errno) == -1) {
> > + if (cb.callback (cb.user_data, &dummy) == -1) {
> > perror (rw->name);
> > exit (EXIT_FAILURE);
> > }
> > @@ -614,10 +616,11 @@ static bool
> > file_asynch_zero (struct rw *rw, struct command *command,
> > nbd_completion_callback cb, bool allocate)
> > {
> > + int dummy = 0;
> > +
> > if (!file_synch_zero (rw, command->offset, command->slice.len, allocate))
> > return false;
> > - errno = 0;
> > - if (cb.callback (cb.user_data, &errno) == -1) {
> > + if (cb.callback (cb.user_data, &dummy) == -1) {
> > perror (rw->name);
> > exit (EXIT_FAILURE);
> > }
> > diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
> > index b17ca598..815b8a02 100644
> > --- a/copy/multi-thread-copying.c
> > +++ b/copy/multi-thread-copying.c
> > @@ -1,5 +1,5 @@
> > /* NBD client library in userspace.
> > - * Copyright (C) 2020 Red Hat Inc.
> > + * Copyright (C) 2020-2022 Red Hat Inc.
> > *
> > * This library is free software; you can redistribute it and/or
> > * modify it under the terms of the GNU Lesser General Public
> > @@ -393,6 +393,7 @@ finished_read (void *vp, int *error)
> > bool last_is_hole = false;
> > uint64_t i;
> > struct command *newcommand;
> > + int dummy = 0;
> >
> > /* Iterate over whole blocks in the command, starting on a block
> > * boundary.
> > @@ -475,7 +476,7 @@ finished_read (void *vp, int *error)
> > /* Free the original command since it has been split into
> > * subcommands and the original is no longer needed.
> > */
> > - free_command (command, &errno);
> > + free_command (command, &dummy);
> > }
> >
> > return 1; /* auto-retires the command */
> > @@ -502,6 +503,7 @@ fill_dst_range_with_zeroes (struct command *command)
> > {
> > char *data;
> > size_t data_size;
> > + int dummy = 0;
> >
> > if (destination_is_zero)
> > goto free_and_return;
> > @@ -537,7 +539,7 @@ fill_dst_range_with_zeroes (struct command *command)
> > free (data);
> >
> > free_and_return:
> > - free_command (command, &errno);
> > + free_command (command, &dummy);
> > }
> >
> > static int
> > diff --git a/copy/null-ops.c b/copy/null-ops.c
> > index a38666d6..11cd5116 100644
> > --- a/copy/null-ops.c
> > +++ b/copy/null-ops.c
> > @@ -1,5 +1,5 @@
> > /* NBD client library in userspace.
> > - * Copyright (C) 2020-2021 Red Hat Inc.
> > + * Copyright (C) 2020-2022 Red Hat Inc.
> > *
> > * This library is free software; you can redistribute it and/or
> > * modify it under the terms of the GNU Lesser General Public
> > @@ -117,8 +117,9 @@ null_asynch_write (struct rw *rw,
> > struct command *command,
> > nbd_completion_callback cb)
> > {
> > - errno = 0;
> > - if (cb.callback (cb.user_data, &errno) == -1) {
> > + int dummy = 0;
> > +
> > + if (cb.callback (cb.user_data, &dummy) == -1) {
> > perror (rw->name);
> > exit (EXIT_FAILURE);
> > }
> > @@ -128,8 +129,9 @@ static bool
> > null_asynch_zero (struct rw *rw, struct command *command,
> > nbd_completion_callback cb, bool allocate)
> > {
> > - errno = 0;
> > - if (cb.callback (cb.user_data, &errno) == -1) {
> > + int dummy = 0;
> > +
> > + if (cb.callback (cb.user_data, &dummy) == -1) {
> > perror (rw->name);
> > exit (EXIT_FAILURE);
> > }
> > --
> > 2.34.1
Looks good.
Nir
More information about the Libguestfs
mailing list