<div dir="ltr">Doug,<div><br></div><div>would this patch address the Linux kernel backward-compatibility concern you raised ?</div><div><br></div><div>Note, you would have to substitute the 3000 version by the appropriate version that introduced support for prin commands submitted on a open-ro device ...</div>
<div><br></div><div><div><div>--- sg_persist.c.orig<span class="" style="white-space:pre"> </span>2014-05-04 01:10:01.987981956 +0200</div><div>+++ sg_persist.c<span class="" style="white-space:pre"> </span>2014-05-04 20:44:34.665292548 +0200</div>
<div>@@ -16,6 +16,7 @@</div><div> #include <string.h></div><div> #include <ctype.h></div><div> #include <getopt.h></div><div>+#include <sys/ioctl.h></div><div> #define __STDC_FORMAT_MACROS 1</div><div>
</div><div> #include <inttypes.h></div><div>@@ -26,6 +27,7 @@</div><div> #include "sg_lib.h"</div><div> #include "sg_cmds_basic.h"</div><div> #include "sg_cmds_extra.h"</div><div>+#include "sg_io_linux.h"</div>
<div> </div><div> static const char * version_str = "0.44 20140202";</div><div> </div><div>@@ -1029,6 +1031,8 @@</div><div> struct sg_simple_inquiry_resp inq_resp;</div><div> const char * cp;</div><div> struct opts_t opts;</div>
<div>+ int omode = 0;</div><div>+ const char *omode_desc = "rw";</div><div> </div><div> memset(&opts, 0, sizeof(opts));</div><div> opts.prin = 1;</div><div>@@ -1292,10 +1296,31 @@</div><div> sg_cmds_close_device(sg_fd);</div>
<div> }</div><div> </div><div>- if ((sg_fd = sg_cmds_open_device(device_name, 0 /* rw */,</div><div>+#ifdef SG_LIB_LINUX</div><div>+ /*</div><div>+ * PRIN commands do not provoke device changes, so we should avoid to</div>
<div>+ * open the device read-write so that the Linux kernel does not generate</div><div>+ * a change event.</div><div>+ * Older kernel do not support PRIN commands submission on a read-only</div><div>+ * opened device, so don't try in this case.</div>
<div>+ */</div><div>+ int v;</div><div>+ if (opts.prin) {</div><div>+ sg_fd = sg_cmds_open_device(device_name, 1, opts.verbose);</div><div>+ if (sg_fd >= 0) {</div><div>+ if (ioctl(sg_fd, SG_GET_VERSION_NUM, &v) >= 0 && v >= 30000) {</div>
<div>+ omode = 1;</div><div>+ omode_desc = "ro";</div><div>+ }</div><div>+ sg_cmds_close_device(sg_fd);</div><div>+ }</div><div>+ }</div><div>+#endif</div>
<div>+</div><div>+ if ((sg_fd = sg_cmds_open_device(device_name, omode,</div><div> opts.verbose)) < 0) {</div><div>- pr2serr("sg_persist: error opening file (rw): %s: %s\n", device_name,</div>
<div>- safe_strerror(-sg_fd));</div><div>+ pr2serr("sg_persist: error opening file (%s): %s: %s\n", omode_desc,</div><div>+ device_name, safe_strerror(-sg_fd));</div><div> return SG_LIB_FILE_ERROR;</div>
<div> }</div><div> </div></div><div><br></div><div> </div></div><div><br></div><div>Best regards,</div><div>Christophe Varoqui</div><div><a href="http://www.opensvc.com">www.opensvc.com</a></div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Sun, May 4, 2014 at 8:01 PM, Christophe Varoqui <span dir="ltr"><<a href="mailto:christophe.varoqui@opensvc.com" target="_blank">christophe.varoqui@opensvc.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Indeed, I'd rather let udev do its work on legitimate device changes. Problem is, this change event caused by sg_persist prin commands in not legitimate : no device change can be caused by interrogating the disk pr status.<div>
<br></div><div>Doug informed me newer kernels accept prin commands inside a open-ro=>close sequence, which I now verified by testing with the patch I sent ... but older kernels do not. My patch is thus not backward compatible as-is.</div>
<div><br></div><div>Any advice on how to treat this backward-compatibility issue ?</div><div><br></div><div>For information, here's a more recent version of the patch I sent to Doug earlier to address not-Linux platform behaviour stability :</div>
<div><br></div><div><div class=""><div style="font-family:arial,sans-serif;font-size:19.428571701049805px">--- sg_persist.c.orig<span style="white-space:pre-wrap"> </span>2014-05-04 01:10:01.987981956 +0200</div></div><div style="font-family:arial,sans-serif;font-size:19.428571701049805px">
+++ sg_persist.c<span style="white-space:pre-wrap"> </span>2014-05-04 08:41:29.482017943 +0200</div><div class=""><div style="font-family:arial,sans-serif;font-size:19.428571701049805px"><div>@@ -1029,6 +1029,8 @@</div><div>
struct sg_simple_inquiry_resp inq_resp;</div><div> const char * cp;</div><div> struct opts_t opts;</div><div>+ int omode = 0;</div></div></div><div style="font-family:arial,sans-serif;font-size:19.428571701049805px">
+ const char *omode_desc = "rw";</div><div class=""><div style="font-family:arial,sans-serif;font-size:19.428571701049805px"><div> </div><div> memset(&opts, 0, sizeof(opts));</div><div> opts.prin = 1;</div>
</div></div><div style="font-family:arial,sans-serif;font-size:19.428571701049805px">@@ -1292,10 +1294,17 @@</div><div class=""><div style="font-family:arial,sans-serif;font-size:19.428571701049805px"><div> sg_cmds_close_device(sg_fd);</div>
<div> }</div><div> </div><div>- if ((sg_fd = sg_cmds_open_device(device_name, 0 /* rw */,</div></div></div><div style="font-family:arial,sans-serif;font-size:19.428571701049805px">+#ifdef SG_LIB_LINUX</div><div class="">
<div style="font-family:arial,sans-serif;font-size:19.428571701049805px">
<div>+ if (opts.prin) {</div><div>+ omode = 1;</div><div>+ omode_desc = "ro";</div><div>+ }</div></div></div><div style="font-family:arial,sans-serif;font-size:19.428571701049805px">+#endif</div>
<div class=""><div style="font-family:arial,sans-serif;font-size:19.428571701049805px">
<div>+</div><div>+ if ((sg_fd = sg_cmds_open_device(device_name, omode,</div><div> opts.verbose)) < 0) {</div><div>- pr2serr("sg_persist: error opening file (rw): %s: %s\n", device_name,</div>
<div>- safe_strerror(-sg_fd));</div><div>+ pr2serr("sg_persist: error opening file (%s): %s: %s\n", omode_desc,</div><div>+ device_name, safe_strerror(-sg_fd));</div><div> return SG_LIB_FILE_ERROR;</div>
<div> }</div></div></div></div><div><br></div><div>Regards,</div><div>Christophe Varoqui</div><div><a href="http://www.opensvc.com" target="_blank">www.opensvc.com</a></div><div><br></div></div><div class="HOEnZb"><div class="h5">
<div class="gmail_extra"><br><br><div class="gmail_quote">
On Sun, May 4, 2014 at 7:23 PM, Zdenek Kabelac <span dir="ltr"><<a href="mailto:zkabelac@redhat.com" target="_blank">zkabelac@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Dne 4.5.2014 18:54, Hannes Reinecke napsal(a):<div><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 05/04/2014 01:34 AM, Christophe Varoqui wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
It seems sg_persist is doing an "open rw => close" for --in commands,<br>
causing a kernel change-event.<br>
</blockquote>
Yep.<br>
<br>
Look for 'watch' in the udev rules, that's precisely what it's doing.<br>
<br>
(Bloody annoying if you ask me. Generally I recommend to remove that thing<br>
from the rules).<br>
</blockquote>
<br></div>
When watch rule is disabled/removed in udev rules - your udev db becomes invalid when i.e. you run command like 'mkfs' - since the udev db will not be updated to list information about newly formatted filesystem.<br>
<br>
Of course there are many cases where disabling watch rule makes sense<br>
(i.e. you export lots of disks to virtual guests) - but unless you are<br>
familiar with udev and you know what you are doing - think twice before disabling.<br>
<br>
Zdenek<br>
<br>
--<br>
dm-devel mailing list<br>
<a href="mailto:dm-devel@redhat.com" target="_blank">dm-devel@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/dm-devel" target="_blank">https://www.redhat.com/<u></u>mailman/listinfo/dm-devel</a><br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>