Eric Blake
2022-Feb-03 20:25 UTC
[Libguestfs] [libnbd PATCH v2 4/5] copy: Pass in dummy variable rather than &errno to callback
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. --- 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
Richard W.M. Jones
2022-Feb-03 21:41 UTC
[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.1Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Laszlo Ersek
2022-Feb-04 09:00 UTC
[Libguestfs] [libnbd PATCH v2 4/5] copy: Pass in dummy variable rather than &errno to callback
On 02/03/22 21:25, 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. > --- > 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); > } >Reviewed-by: Laszlo Ersek <lersek at redhat.com>