Re: [Libguestfs] [libnbd PATCH v2 4/5] copy: Pass in dummy variable rather than &errno to callback

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.

> 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