Since we have a number of places where we workaround timing issues with
devices, attributes (files in general) not being available at the time
of processing them by calling usleep in a loop for a fixed number of
tries, we could as well have a utility function that would do that.
Therefore we won't have to duplicate this ugly workaround even more.
This is a prerequisite for
https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 36 ++++++++++++++++++++++++++++++++++++
src/util/virfile.h | 2 ++
3 files changed, 39 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c1e9471c5..53878a30f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1698,6 +1698,7 @@ virFileStripSuffix;
virFileTouch;
virFileUnlock;
virFileUpdatePerm;
+virFileWaitForAccess;
virFileWrapperFdClose;
virFileWrapperFdFree;
virFileWrapperFdNew;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 6bbcc3d15..0b1a91699 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const char *format, ...)
VIR_FREE(str);
return ret;
}
+
+
+/**
+ * virFileWaitForAccess:
+ * @path: absolute path to a sysfs attribute (can be a symlink)
+ * @ms: how long to wait (in milliseconds)
+ * @tries: how many times should we try to wait for @path to become accessible
+ *
+ * Checks the existence of @path. In case the file defined by @path
+ * doesn't exist, we wait for it to appear in 100ms (for up to @tries times).
+ *
+ * Returns 0 on success, -1 on error (ENOENT is fine here).
+ */
+int
+virFileWaitForAccess(const char *path, size_t ms, size_t tries)
+{
+ errno = 0;
+
+ /* wait for @path to be accessible in @ms milliseconds, up to @tries */
+ while (tries-- > 0 && !virFileExists(path)) {
+ if (errno != ENOENT) {
+ virReportSystemError(errno, "%s", path);
+ return -1;
+ } else if (tries == 10) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to access '%s' after %zu tries"),
+ path, tries);
+ return -1;
+ } else {
+ VIR_DEBUG("Failed to access '%s', re-try in %zu ms", path, ms);
+ usleep(ms * 1000);
+ }
+ }
+
+ return 0;
+}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 57ceb8072..42630ebb5 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -347,6 +347,8 @@ int virFileReadValueScaledInt(unsigned long long *value, const char *format, ...
int virFileReadValueString(char **value, const char *format, ...)
ATTRIBUTE_FMT_PRINTF(2, 3);
+int virFileWaitForAccess(const char *path, size_t ms, size_t tries);
+
int virFileInData(int fd,
int *inData,
--
2.13.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2017-06-20 at 17:03 +0200, Erik Skultety wrote:
> Since we have a number of places where we workaround timing issues
> with
> devices, attributes (files in general) not being available at the
> time
> of processing them by calling usleep in a loop for a fixed number of
> tries, we could as well have a utility function that would do that.
> Therefore we won't have to duplicate this ugly workaround even more.
>
> This is a prerequisite for
> https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
>
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virfile.c | 36 ++++++++++++++++++++++++++++++++++++
> src/util/virfile.h | 2 ++
> 3 files changed, 39 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c1e9471c5..53878a30f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1698,6 +1698,7 @@ virFileStripSuffix;
> virFileTouch;
> virFileUnlock;
> virFileUpdatePerm;
> +virFileWaitForAccess;
> virFileWrapperFdClose;
> virFileWrapperFdFree;
> virFileWrapperFdNew;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 6bbcc3d15..0b1a91699 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const
> char *format, ...)
> VIR_FREE(str);
> return ret;
> }
> +
> +
> +/**
> + * virFileWaitForAccess:
> + * @path: absolute path to a sysfs attribute (can be a symlink)
> + * @ms: how long to wait (in milliseconds)
> + * @tries: how many times should we try to wait for @path to become
> accessible
> + *
> + * Checks the existence of @path. In case the file defined by @path
> + * doesn't exist, we wait for it to appear in 100ms (for up to
> @tries times).
> + *
> + * Returns 0 on success, -1 on error (ENOENT is fine here).
> + */
> +int
> +virFileWaitForAccess(const char *path, size_t ms, size_t tries)
> +{
> + errno = 0;
> +
> + /* wait for @path to be accessible in @ms milliseconds, up to
> @tries */
> + while (tries-- > 0 && !virFileExists(path)) {
> + if (errno != ENOENT) {
> + virReportSystemError(errno, "%s", path);
> + return -1;
> + } else if (tries == 10) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to access '%s' after %zu
> tries"),
> + path, tries);
> + return -1;
> + } else {
> + VIR_DEBUG("Failed to access '%s', re-try in %zu ms",
> path, ms);
> + usleep(ms * 1000);
> + }
> + }
> +
> + return 0;
> +}
Just FYI, there's another way to address it by calling udevadm settle
before and after "touching" a block device, libguestfs is using this
approach and it works very well:
https://github.com/libguestfs/libguestfs/search?utf8=%E2%9C%93&q=udev_s
ettle&type=
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 57ceb8072..42630ebb5 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -347,6 +347,8 @@ int virFileReadValueScaledInt(unsigned long long
> *value, const char *format, ...
> int virFileReadValueString(char **value, const char *format, ...)
> ATTRIBUTE_FMT_PRINTF(2, 3);
>
> +int virFileWaitForAccess(const char *path, size_t ms, size_t tries);
> +
>
> int virFileInData(int fd,
> int *inData,
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/20/2017 05:21 PM, Dawid Zamirski wrote:
> On Tue, 2017-06-20 at 17:03 +0200, Erik Skultety wrote:
>> Since we have a number of places where we workaround timing issues
>> with
>> devices, attributes (files in general) not being available at the
>> time
>> of processing them by calling usleep in a loop for a fixed number of
>> tries, we could as well have a utility function that would do that.
>> Therefore we won't have to duplicate this ugly workaround even more.
>>
>> This is a prerequisite for
>> https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
>>
>> Signed-off-by: Erik Skultety <eskultet@redhat.com>
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virfile.c | 36 ++++++++++++++++++++++++++++++++++++
>> src/util/virfile.h | 2 ++
>> 3 files changed, 39 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index c1e9471c5..53878a30f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1698,6 +1698,7 @@ virFileStripSuffix;
>> virFileTouch;
>> virFileUnlock;
>> virFileUpdatePerm;
>> +virFileWaitForAccess;
>> virFileWrapperFdClose;
>> virFileWrapperFdFree;
>> virFileWrapperFdNew;
>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>> index 6bbcc3d15..0b1a91699 100644
>> --- a/src/util/virfile.c
>> +++ b/src/util/virfile.c
>> @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const
>> char *format, ...)
>> VIR_FREE(str);
>> return ret;
>> }
>> +
>> +
>> +/**
>> + * virFileWaitForAccess:
>> + * @path: absolute path to a sysfs attribute (can be a symlink)
>> + * @ms: how long to wait (in milliseconds)
>> + * @tries: how many times should we try to wait for @path to become
>> accessible
>> + *
>> + * Checks the existence of @path. In case the file defined by @path
>> + * doesn't exist, we wait for it to appear in 100ms (for up to
>> @tries times).
>> + *
>> + * Returns 0 on success, -1 on error (ENOENT is fine here).
>> + */
>> +int
>> +virFileWaitForAccess(const char *path, size_t ms, size_t tries)
>> +{
>> + errno = 0;
>> +
>> + /* wait for @path to be accessible in @ms milliseconds, up to
>> @tries */
>> + while (tries-- > 0 && !virFileExists(path)) {
>> + if (errno != ENOENT) {
>> + virReportSystemError(errno, "%s", path);
>> + return -1;
>> + } else if (tries == 10) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Failed to access '%s' after %zu
>> tries"),
>> + path, tries);
>> + return -1;
>> + } else {
>> + VIR_DEBUG("Failed to access '%s', re-try in %zu ms",
>> path, ms);
>> + usleep(ms * 1000);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>
> Just FYI, there's another way to address it by calling udevadm settle
> before and after "touching" a block device, libguestfs is using this
> approach and it works very well:
>
> https://github.com/libguestfs/libguestfs/search?utf8=%E2%9C%93&q=udev_s
> ettle&type=
Does it? udevadm settle waits for all the events to be processed, not
just the one that we want. The wait time would be unpredictable IMO.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2017-06-20 at 17:45 +0200, Michal Privoznik wrote:
> On 06/20/2017 05:21 PM, Dawid Zamirski wrote:
> > On Tue, 2017-06-20 at 17:03 +0200, Erik Skultety wrote:
> > > Since we have a number of places where we workaround timing
> > > issues
> > > with
> > > devices, attributes (files in general) not being available at the
> > > time
> > > of processing them by calling usleep in a loop for a fixed number
> > > of
> > > tries, we could as well have a utility function that would do
> > > that.
> > > Therefore we won't have to duplicate this ugly workaround even
> > > more.
> > >
> > > This is a prerequisite for
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
> > >
> > > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > > ---
> > > src/libvirt_private.syms | 1 +
> > > src/util/virfile.c | 36
> > > ++++++++++++++++++++++++++++++++++++
> > > src/util/virfile.h | 2 ++
> > > 3 files changed, 39 insertions(+)
> > >
> > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > > index c1e9471c5..53878a30f 100644
> > > --- a/src/libvirt_private.syms
> > > +++ b/src/libvirt_private.syms
> > > @@ -1698,6 +1698,7 @@ virFileStripSuffix;
> > > virFileTouch;
> > > virFileUnlock;
> > > virFileUpdatePerm;
> > > +virFileWaitForAccess;
> > > virFileWrapperFdClose;
> > > virFileWrapperFdFree;
> > > virFileWrapperFdNew;
> > > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > > index 6bbcc3d15..0b1a91699 100644
> > > --- a/src/util/virfile.c
> > > +++ b/src/util/virfile.c
> > > @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const
> > > char *format, ...)
> > > VIR_FREE(str);
> > > return ret;
> > > }
> > > +
> > > +
> > > +/**
> > > + * virFileWaitForAccess:
> > > + * @path: absolute path to a sysfs attribute (can be a symlink)
> > > + * @ms: how long to wait (in milliseconds)
> > > + * @tries: how many times should we try to wait for @path to
> > > become
> > > accessible
> > > + *
> > > + * Checks the existence of @path. In case the file defined by
> > > @path
> > > + * doesn't exist, we wait for it to appear in 100ms (for up to
> > > @tries times).
> > > + *
> > > + * Returns 0 on success, -1 on error (ENOENT is fine here).
> > > + */
> > > +int
> > > +virFileWaitForAccess(const char *path, size_t ms, size_t tries)
> > > +{
> > > + errno = 0;
> > > +
> > > + /* wait for @path to be accessible in @ms milliseconds, up
> > > to
> > > @tries */
> > > + while (tries-- > 0 && !virFileExists(path)) {
> > > + if (errno != ENOENT) {
> > > + virReportSystemError(errno, "%s", path);
> > > + return -1;
> > > + } else if (tries == 10) {
> > > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > > + _("Failed to access '%s' after %zu
> > > tries"),
> > > + path, tries);
> > > + return -1;
> > > + } else {
> > > + VIR_DEBUG("Failed to access '%s', re-try in %zu ms",
> > > path, ms);
> > > + usleep(ms * 1000);
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > Just FYI, there's another way to address it by calling udevadm
> > settle
> > before and after "touching" a block device, libguestfs is using
> > this
> > approach and it works very well:
> >
> > https://github.com/libguestfs/libguestfs/search?utf8=%E2%9C%93&q=ud
> > ev_s
> > ettle&type=
>
> Does it? udevadm settle waits for all the events to be processed, not
> just the one that we want. The wait time would be unpredictable IMO.
>
> Michal
Well it's a kind of brute-force approach but at least guarantees the
device will be accessible once it exits. Why would setting a custom
arbitrary timeout be better which may be too short? Is the predictable
wait time here more important than being able to open the device at
all?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jun 20, 2017 at 12:21:48PM -0400, Dawid Zamirski wrote:
> On Tue, 2017-06-20 at 17:45 +0200, Michal Privoznik wrote:
> > On 06/20/2017 05:21 PM, Dawid Zamirski wrote:
> > > On Tue, 2017-06-20 at 17:03 +0200, Erik Skultety wrote:
> > > > Since we have a number of places where we workaround timing
> > > > issues
> > > > with
> > > > devices, attributes (files in general) not being available at the
> > > > time
> > > > of processing them by calling usleep in a loop for a fixed number
> > > > of
> > > > tries, we could as well have a utility function that would do
> > > > that.
> > > > Therefore we won't have to duplicate this ugly workaround even
> > > > more.
> > > >
> > > > This is a prerequisite for
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
> > > >
> > > > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > > > ---
> > > > src/libvirt_private.syms | 1 +
> > > > src/util/virfile.c | 36
> > > > ++++++++++++++++++++++++++++++++++++
> > > > src/util/virfile.h | 2 ++
> > > > 3 files changed, 39 insertions(+)
> > > >
> > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > > > index c1e9471c5..53878a30f 100644
> > > > --- a/src/libvirt_private.syms
> > > > +++ b/src/libvirt_private.syms
> > > > @@ -1698,6 +1698,7 @@ virFileStripSuffix;
> > > > virFileTouch;
> > > > virFileUnlock;
> > > > virFileUpdatePerm;
> > > > +virFileWaitForAccess;
> > > > virFileWrapperFdClose;
> > > > virFileWrapperFdFree;
> > > > virFileWrapperFdNew;
> > > > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > > > index 6bbcc3d15..0b1a91699 100644
> > > > --- a/src/util/virfile.c
> > > > +++ b/src/util/virfile.c
> > > > @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const
> > > > char *format, ...)
> > > > VIR_FREE(str);
> > > > return ret;
> > > > }
> > > > +
> > > > +
> > > > +/**
> > > > + * virFileWaitForAccess:
> > > > + * @path: absolute path to a sysfs attribute (can be a symlink)
> > > > + * @ms: how long to wait (in milliseconds)
> > > > + * @tries: how many times should we try to wait for @path to
> > > > become
> > > > accessible
> > > > + *
> > > > + * Checks the existence of @path. In case the file defined by
> > > > @path
> > > > + * doesn't exist, we wait for it to appear in 100ms (for up to
> > > > @tries times).
> > > > + *
> > > > + * Returns 0 on success, -1 on error (ENOENT is fine here).
> > > > + */
> > > > +int
> > > > +virFileWaitForAccess(const char *path, size_t ms, size_t tries)
> > > > +{
> > > > + errno = 0;
> > > > +
> > > > + /* wait for @path to be accessible in @ms milliseconds, up
> > > > to
> > > > @tries */
> > > > + while (tries-- > 0 && !virFileExists(path)) {
> > > > + if (errno != ENOENT) {
> > > > + virReportSystemError(errno, "%s", path);
> > > > + return -1;
> > > > + } else if (tries == 10) {
> > > > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > > > + _("Failed to access '%s' after %zu
> > > > tries"),
> > > > + path, tries);
> > > > + return -1;
> > > > + } else {
> > > > + VIR_DEBUG("Failed to access '%s', re-try in %zu ms",
> > > > path, ms);
> > > > + usleep(ms * 1000);
> > > > + }
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > Just FYI, there's another way to address it by calling udevadm
> > > settle
> > > before and after "touching" a block device, libguestfs is using
> > > this
> > > approach and it works very well:
> > >
> > > https://github.com/libguestfs/libguestfs/search?utf8=%E2%9C%93&q=ud
> > > ev_s
> > > ettle&type=
> >
> > Does it? udevadm settle waits for all the events to be processed, not
> > just the one that we want. The wait time would be unpredictable IMO.
> >
> > Michal
>
> Well it's a kind of brute-force approach but at least guarantees the
> device will be accessible once it exits. Why would setting a custom
> arbitrary timeout be better which may be too short? Is the predictable
> wait time here more important than being able to open the device at
> all?
>
So, there was this thing about udevadm settle that wasn't clear to me, because
it says it waits for udevd to create all the device nodes - so since udev only
manages "/dev", I assumed it wouldn't work for sysfs attributes, but gave it a
try since I wasn't sure and, no, we can't use udevadm here, because udevd really
doesn't care about sysfs (mdevs only live in sysfs - well, strictly speaking,
a corresponding /dev/vfio/ device IS created for an mdev, but for that we get a
separate event and because for node device purposes we don't report vfio
devices, we ignore such event) hence the explicit, unpredictable, best-effort
timeout here.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jun 20, 2017 at 17:03:31 +0200, Erik Skultety wrote:
> Since we have a number of places where we workaround timing issues with
> devices, attributes (files in general) not being available at the time
> of processing them by calling usleep in a loop for a fixed number of
> tries, we could as well have a utility function that would do that.
> Therefore we won't have to duplicate this ugly workaround even more.
>
> This is a prerequisite for
> https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
This statement is useless. The helper function can be reused elsewhere.
>
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virfile.c | 36 ++++++++++++++++++++++++++++++++++++
> src/util/virfile.h | 2 ++
> 3 files changed, 39 insertions(+)
[...]
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 6bbcc3d15..0b1a91699 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const char *format, ...)
> VIR_FREE(str);
> return ret;
> }
> +
> +
> +/**
> + * virFileWaitForAccess:
> + * @path: absolute path to a sysfs attribute (can be a symlink)
> + * @ms: how long to wait (in milliseconds)
> + * @tries: how many times should we try to wait for @path to become accessible
> + *
> + * Checks the existence of @path. In case the file defined by @path
> + * doesn't exist, we wait for it to appear in 100ms (for up to @tries times).
> + *
> + * Returns 0 on success, -1 on error (ENOENT is fine here).
This description does not make sense. You don't state that this reports
errors. Also the mention of ENOENT does not make sense.
This function in fact sometimes returns enoent if the file does not
appear until timeout.
> + */
> +int
> +virFileWaitForAccess(const char *path, size_t ms, size_t tries)
> +{
> + errno = 0;
> +
> + /* wait for @path to be accessible in @ms milliseconds, up to @tries */
> + while (tries-- > 0 && !virFileExists(path)) {
> + if (errno != ENOENT) {
> + virReportSystemError(errno, "%s", path);
This does not really explain stuff to users. You might want to add a
more comprehensive error message or leave error reporting to users.
> + return -1;
> + } else if (tries == 10) {
This does not make any sense. The while loop counts down and checks if
tries is more than 10. And this checks that tries is equal to 10.
So if somebody passes 11 as @tries he/she gets this error? If tries is
set to < 10 you get success even on timeout?
Did you modify the code without testing it?
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to access '%s' after %zu tries"),
> + path, tries);
> + return -1;
> + } else {
> + VIR_DEBUG("Failed to access '%s', re-try in %zu ms", path, ms);
This will spam logs too much. I think a debug prior to the while loop
and one after it succeeds should be enough.
> + usleep(ms * 1000);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jun 20, 2017 at 05:22:52PM +0200, Peter Krempa wrote:
> On Tue, Jun 20, 2017 at 17:03:31 +0200, Erik Skultety wrote:
> > Since we have a number of places where we workaround timing issues with
> > devices, attributes (files in general) not being available at the time
> > of processing them by calling usleep in a loop for a fixed number of
> > tries, we could as well have a utility function that would do that.
> > Therefore we won't have to duplicate this ugly workaround even more.
> >
> > This is a prerequisite for
> > https://bugzilla.redhat.com/show_bug.cgi?id=1463285.
>
> This statement is useless. The helper function can be reused elsewhere.
>
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> > src/libvirt_private.syms | 1 +
> > src/util/virfile.c | 36 ++++++++++++++++++++++++++++++++++++
> > src/util/virfile.h | 2 ++
> > 3 files changed, 39 insertions(+)
>
> [...]
>
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > index 6bbcc3d15..0b1a91699 100644
> > --- a/src/util/virfile.c
> > +++ b/src/util/virfile.c
> > @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const char *format, ...)
> > VIR_FREE(str);
> > return ret;
> > }
> > +
> > +
> > +/**
> > + * virFileWaitForAccess:
> > + * @path: absolute path to a sysfs attribute (can be a symlink)
> > + * @ms: how long to wait (in milliseconds)
> > + * @tries: how many times should we try to wait for @path to become accessible
> > + *
> > + * Checks the existence of @path. In case the file defined by @path
> > + * doesn't exist, we wait for it to appear in 100ms (for up to @tries times).
> > + *
> > + * Returns 0 on success, -1 on error (ENOENT is fine here).
>
> This description does not make sense. You don't state that this reports
> errors. Also the mention of ENOENT does not make sense.
>
> This function in fact sometimes returns enoent if the file does not
> appear until timeout.
>
> > + */
> > +int
> > +virFileWaitForAccess(const char *path, size_t ms, size_t tries)
> > +{
> > + errno = 0;
> > +
> > + /* wait for @path to be accessible in @ms milliseconds, up to @tries */
> > + while (tries-- > 0 && !virFileExists(path)) {
> > + if (errno != ENOENT) {
> > + virReportSystemError(errno, "%s", path);
>
> This does not really explain stuff to users. You might want to add a
> more comprehensive error message or leave error reporting to users.
>
> > + return -1;
> > + } else if (tries == 10) {
>
> This does not make any sense. The while loop counts down and checks if
> tries is more than 10. And this checks that tries is equal to 10.
>
> So if somebody passes 11 as @tries he/she gets this error? If tries is
> set to < 10 you get success even on timeout?
>
> Did you modify the code without testing it?
Damn :/, I actually tested it (after those 3+ modifications I did to it in an
iterative manner) and it worked, but surely didn't hit the issue you describe.
Nevertheless, yep, it's wrong and I need to rework it.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.