* [PATCH] libmultipath/discovery: read sysfs files uncached
@ 2013-04-02 14:28 Sebastian Riemer
2013-04-02 21:00 ` Christophe Varoqui
0 siblings, 1 reply; 8+ messages in thread
From: Sebastian Riemer @ 2013-04-02 14:28 UTC (permalink / raw)
To: dm-devel; +Cc: Sebastian Riemer, Bart Van Assche
The libudev function udev_device_get_sysattr_value() reads sysfs
attributes cached. This is useless for checking a device state.
There we want to see if it changes.
Unfortunately, libudev doesn't provide an uncached variant. This
is why we have to reimplement the functionality and some libudev
internal functions here.
Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
---
libmultipath/discovery.c | 78 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 77 insertions(+), 1 deletion(-)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 0b5fd1d..1cb1f15 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -132,6 +132,82 @@ path_discovery (vector pathvec, struct config * conf, int flag)
return r;
}
+/* helpers for get_udev_device_sysattr_value() */
+static size_t
+strpcpy(char **dest, size_t size, const char *src)
+{
+ size_t len;
+
+ len = strlen(src);
+ if (len >= size) {
+ if (size > 1)
+ *dest = mempcpy(*dest, src, size-1);
+ size = 0;
+ } else {
+ if (len > 0) {
+ *dest = mempcpy(*dest, src, len);
+ size -= len;
+ }
+ }
+ *dest[0] = '\0';
+ return size;
+}
+
+static size_t
+strscpyl(char *dest, size_t size, const char *src, ...)
+{
+ va_list va;
+ char *s;
+
+ va_start(va, src);
+ s = dest;
+ do {
+ size = strpcpy(&s, size, src);
+ src = va_arg(va, char *);
+ } while (src != NULL);
+ va_end(va);
+
+ return size;
+}
+
+static void
+util_remove_trailing_chars(char *path, char c)
+{
+ size_t len;
+
+ if (path == NULL)
+ return;
+ len = strlen(path);
+ while (len > 0 && path[len-1] == c)
+ path[--len] = '\0';
+}
+
+/* like udev_device_get_sysattr_value() but uncached */
+static const char *
+get_udev_device_sysattr_value(struct udev_device *udev,
+ const char *sysattr)
+{
+ const char *val = NULL;
+ char path[1024];
+ char value[4096];
+ int fd;
+ ssize_t size;
+
+ strscpyl(path, sizeof(path), udev_device_get_syspath(udev),
+ "/", sysattr, NULL);
+ fd = open(path, O_RDONLY|O_CLOEXEC);
+ if (fd >= 0) {
+ size = read(fd, value, sizeof(value));
+ close(fd);
+ if (size >= 0 && size < sizeof(value)) {
+ value[size] = '\0';
+ util_remove_trailing_chars(value, '\n');
+ val = value;
+ }
+ }
+ return val;
+}
+
#define declare_sysfs_get_str(fname) \
extern int \
sysfs_get_##fname (struct udev_device * udev, char * buff, size_t len) \
@@ -141,7 +217,7 @@ sysfs_get_##fname (struct udev_device * udev, char * buff, size_t len) \
\
devname = udev_device_get_sysname(udev); \
\
- attr = udev_device_get_sysattr_value(udev, #fname); \
+ attr = get_udev_device_sysattr_value(udev, #fname); \
if (!attr) { \
condlog(3, "%s: attribute %s not found in sysfs", \
devname, #fname); \
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] libmultipath/discovery: read sysfs files uncached
2013-04-02 14:28 [PATCH] libmultipath/discovery: read sysfs files uncached Sebastian Riemer
@ 2013-04-02 21:00 ` Christophe Varoqui
2013-04-04 6:43 ` Hannes Reinecke
0 siblings, 1 reply; 8+ messages in thread
From: Christophe Varoqui @ 2013-04-02 21:00 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: dm-devel, Sebastian Riemer, Bart Van Assche
On mar., 2013-04-02 at 16:28 +0200, Sebastian Riemer wrote:
> The libudev function udev_device_get_sysattr_value() reads sysfs
> attributes cached. This is useless for checking a device state.
> There we want to see if it changes.
>
> Unfortunately, libudev doesn't provide an uncached variant. This
> is why we have to reimplement the functionality and some libudev
> internal functions here.
>
Hannes,
as the commit 058a0044cb2ab7cac6f7c3e2e17b16e00b5e57fa author, would you
sign-off this patch ? Or do you prefer the issue addressed on the udev
side ...
Best regards,
Christophe Varoqui
www.opensvc.com
> Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
> ---
> libmultipath/discovery.c | 78 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 0b5fd1d..1cb1f15 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -132,6 +132,82 @@ path_discovery (vector pathvec, struct config * conf, int flag)
> return r;
> }
>
> +/* helpers for get_udev_device_sysattr_value() */
> +static size_t
> +strpcpy(char **dest, size_t size, const char *src)
> +{
> + size_t len;
> +
> + len = strlen(src);
> + if (len >= size) {
> + if (size > 1)
> + *dest = mempcpy(*dest, src, size-1);
> + size = 0;
> + } else {
> + if (len > 0) {
> + *dest = mempcpy(*dest, src, len);
> + size -= len;
> + }
> + }
> + *dest[0] = '\0';
> + return size;
> +}
> +
> +static size_t
> +strscpyl(char *dest, size_t size, const char *src, ...)
> +{
> + va_list va;
> + char *s;
> +
> + va_start(va, src);
> + s = dest;
> + do {
> + size = strpcpy(&s, size, src);
> + src = va_arg(va, char *);
> + } while (src != NULL);
> + va_end(va);
> +
> + return size;
> +}
> +
> +static void
> +util_remove_trailing_chars(char *path, char c)
> +{
> + size_t len;
> +
> + if (path == NULL)
> + return;
> + len = strlen(path);
> + while (len > 0 && path[len-1] == c)
> + path[--len] = '\0';
> +}
> +
> +/* like udev_device_get_sysattr_value() but uncached */
> +static const char *
> +get_udev_device_sysattr_value(struct udev_device *udev,
> + const char *sysattr)
> +{
> + const char *val = NULL;
> + char path[1024];
> + char value[4096];
> + int fd;
> + ssize_t size;
> +
> + strscpyl(path, sizeof(path), udev_device_get_syspath(udev),
> + "/", sysattr, NULL);
> + fd = open(path, O_RDONLY|O_CLOEXEC);
> + if (fd >= 0) {
> + size = read(fd, value, sizeof(value));
> + close(fd);
> + if (size >= 0 && size < sizeof(value)) {
> + value[size] = '\0';
> + util_remove_trailing_chars(value, '\n');
> + val = value;
> + }
> + }
> + return val;
> +}
> +
> #define declare_sysfs_get_str(fname) \
> extern int \
> sysfs_get_##fname (struct udev_device * udev, char * buff, size_t len) \
> @@ -141,7 +217,7 @@ sysfs_get_##fname (struct udev_device * udev, char * buff, size_t len) \
> \
> devname = udev_device_get_sysname(udev); \
> \
> - attr = udev_device_get_sysattr_value(udev, #fname); \
> + attr = get_udev_device_sysattr_value(udev, #fname); \
> if (!attr) { \
> condlog(3, "%s: attribute %s not found in sysfs", \
> devname, #fname); \
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libmultipath/discovery: read sysfs files uncached
2013-04-02 21:00 ` Christophe Varoqui
@ 2013-04-04 6:43 ` Hannes Reinecke
2013-04-04 9:42 ` Sebastian Riemer
2013-04-04 10:00 ` Kay Sievers
0 siblings, 2 replies; 8+ messages in thread
From: Hannes Reinecke @ 2013-04-04 6:43 UTC (permalink / raw)
To: christophe.varoqui
Cc: dm-devel, Kay Sievers, Sebastian Riemer, Bart Van Assche,
Christophe Varoqui
On 04/02/2013 11:00 PM, Christophe Varoqui wrote:
> On mar., 2013-04-02 at 16:28 +0200, Sebastian Riemer wrote:
>> The libudev function udev_device_get_sysattr_value() reads sysfs
>> attributes cached. This is useless for checking a device state.
>> There we want to see if it changes.
>>
>> Unfortunately, libudev doesn't provide an uncached variant. This
>> is why we have to reimplement the functionality and some libudev
>> internal functions here.
>>
> Hannes,
>
> as the commit 058a0044cb2ab7cac6f7c3e2e17b16e00b5e57fa author, would you
> sign-off this patch ? Or do you prefer the issue addressed on the udev
> side ...
>
Hmm; this is not the only issue we have with libudev.
_Setting_ of sysfs attributes is also a problem (I just send a patch
to Kay for this).
However, having to duplicate things feels like a waste; I'd rather
have it implemented at the libudev side.
The problem with this patch is that we're bypassing the cache of
libudev; so any _other_ application using libudev would still be
reading the stale value.
I'd very much prefer to have a 'uncached' variant (or adding another
argument 'force') which would force libudev to always read the
current value but also adding it to the internal cache for the
benefit of other applications.
Kay?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libmultipath/discovery: read sysfs files uncached
2013-04-04 6:43 ` Hannes Reinecke
@ 2013-04-04 9:42 ` Sebastian Riemer
2013-04-04 10:00 ` Kay Sievers
1 sibling, 0 replies; 8+ messages in thread
From: Sebastian Riemer @ 2013-04-04 9:42 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: dm-devel, Kay Sievers, Bart Van Assche
On 04.04.2013 08:43, Hannes Reinecke wrote:
> Hmm; this is not the only issue we have with libudev.
> _Setting_ of sysfs attributes is also a problem (I just send a patch
> to Kay for this).
> However, having to duplicate things feels like a waste; I'd rather
> have it implemented at the libudev side.
>
> The problem with this patch is that we're bypassing the cache of
> libudev; so any _other_ application using libudev would still be
> reading the stale value.
>
> I'd very much prefer to have a 'uncached' variant (or adding another
> argument 'force') which would force libudev to always read the
> current value but also adding it to the internal cache for the
> benefit of other applications.
Fine for me. I just wanted to point out that there is an issue. In the
long run it also makes most sense to me to improve libudev.
For early adopters this can be a workaround as libraries aren't that
easy to replace if they come from a binary distribution. The libudev is
part of systemd by now. So back-porting fixes to an older variant of
libudev from a distribution without systemd can be difficult. We already
work like a distribution but we don't want to maintain a git repo for
that stuff as well if we already need to maintain a git repo for the
multipath-tools.
For me as a kind of user in this case the mistake was to use a library
function which doesn't do the expected instead of implementing
differently and reporting the requirement to the library for future use.
If it's in the library, then the workaround can be removed.
Cheers,
Sebastian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libmultipath/discovery: read sysfs files uncached
2013-04-04 6:43 ` Hannes Reinecke
2013-04-04 9:42 ` Sebastian Riemer
@ 2013-04-04 10:00 ` Kay Sievers
2013-04-04 10:20 ` Hannes Reinecke
1 sibling, 1 reply; 8+ messages in thread
From: Kay Sievers @ 2013-04-04 10:00 UTC (permalink / raw)
To: Hannes Reinecke
Cc: dm-devel, Sebastian Riemer, Bart Van Assche, Christophe Varoqui
On Thu, Apr 4, 2013 at 8:43 AM, Hannes Reinecke <hare@suse.de> wrote:
>>> Unfortunately, libudev doesn't provide an uncached variant. This
>>> is why we have to reimplement the functionality and some libudev
>>> internal functions here.
Hannes,
didn't you intend to use udev_device_set_sysattr_value(..., attribute,
NULL); to clear the cached value?
Kay
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libmultipath/discovery: read sysfs files uncached
2013-04-04 10:00 ` Kay Sievers
@ 2013-04-04 10:20 ` Hannes Reinecke
2013-04-04 10:47 ` Sebastian Riemer
2013-04-04 11:59 ` Kay Sievers
0 siblings, 2 replies; 8+ messages in thread
From: Hannes Reinecke @ 2013-04-04 10:20 UTC (permalink / raw)
To: Kay Sievers
Cc: dm-devel, Sebastian Riemer, Bart Van Assche, Christophe Varoqui
On 04/04/2013 12:00 PM, Kay Sievers wrote:
> On Thu, Apr 4, 2013 at 8:43 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>> Unfortunately, libudev doesn't provide an uncached variant. This
>>>> is why we have to reimplement the functionality and some libudev
>>>> internal functions here.
>
> Hannes,
>
> didn't you intend to use udev_device_set_sysattr_value(..., attribute,
> NULL); to clear the cached value?
>
Yes, it should. Question remains, though, what happens to the actual
sysfs attribute.
Does it remain unchained? Will it be set to '0'?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libmultipath/discovery: read sysfs files uncached
2013-04-04 10:20 ` Hannes Reinecke
@ 2013-04-04 10:47 ` Sebastian Riemer
2013-04-04 11:59 ` Kay Sievers
1 sibling, 0 replies; 8+ messages in thread
From: Sebastian Riemer @ 2013-04-04 10:47 UTC (permalink / raw)
To: Hannes Reinecke
Cc: dm-devel, Kay Sievers, Bart Van Assche, Christophe Varoqui
On 04.04.2013 12:20, Hannes Reinecke wrote:
> On 04/04/2013 12:00 PM, Kay Sievers wrote:
>> On Thu, Apr 4, 2013 at 8:43 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>>> Unfortunately, libudev doesn't provide an uncached variant. This
>>>>> is why we have to reimplement the functionality and some libudev
>>>>> internal functions here.
>>
>> Hannes,
>>
>> didn't you intend to use udev_device_set_sysattr_value(..., attribute,
>> NULL); to clear the cached value?
>>
>
> Yes, it should. Question remains, though, what happens to the actual
> sysfs attribute.
> Does it remain unchained? Will it be set to '0'?
I had a look at systemd-200.tar.xz. If the value is NULL, then only the
value length is set to zero. So the sysfs file is still opened and
write() gets zero length as argument. So the file remains unchanged.
Afterwards, NULL is stored in the cache as value. For me this isn't
implemented right as you are bugging the kernel with open() and close()
without a need. Furthermore, Debian Squeeze doesn't have that
"udev_device_set_sysattr_value()" function. So at least that
distribution could need a workaround.
Cheers,
Sebastian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libmultipath/discovery: read sysfs files uncached
2013-04-04 10:20 ` Hannes Reinecke
2013-04-04 10:47 ` Sebastian Riemer
@ 2013-04-04 11:59 ` Kay Sievers
1 sibling, 0 replies; 8+ messages in thread
From: Kay Sievers @ 2013-04-04 11:59 UTC (permalink / raw)
To: Hannes Reinecke
Cc: dm-devel, Sebastian Riemer, Bart Van Assche, Christophe Varoqui
On Thu, Apr 4, 2013 at 12:20 PM, Hannes Reinecke <hare@suse.de> wrote:
> On 04/04/2013 12:00 PM, Kay Sievers wrote:
>> On Thu, Apr 4, 2013 at 8:43 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>>> Unfortunately, libudev doesn't provide an uncached variant. This
>>>>> is why we have to reimplement the functionality and some libudev
>>>>> internal functions here.
>>
>> Hannes,
>>
>> didn't you intend to use udev_device_set_sysattr_value(..., attribute,
>> NULL); to clear the cached value?
>>
>
> Yes, it should. Question remains, though, what happens to the actual
> sysfs attribute.
> Does it remain unchained? Will it be set to '0'?
Hey, you wrote that function. :) It writes 0 bytes, so it should not
do anything. It should probably skip all that, or did you do that for
a reason?
Kay
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-04-04 11:59 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-02 14:28 [PATCH] libmultipath/discovery: read sysfs files uncached Sebastian Riemer
2013-04-02 21:00 ` Christophe Varoqui
2013-04-04 6:43 ` Hannes Reinecke
2013-04-04 9:42 ` Sebastian Riemer
2013-04-04 10:00 ` Kay Sievers
2013-04-04 10:20 ` Hannes Reinecke
2013-04-04 10:47 ` Sebastian Riemer
2013-04-04 11:59 ` Kay Sievers
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.