vdev type missing in zpool status #57
Comments
|
This issue is back again: |
|
I came across this cosmetic issue on current builds. zpool status only shows -0 . Suggest a Reopen . |
|
I confirm this on rc10: No idea what causes it, though. |
|
The original issue was fixed by commit 858219c which still looks intact. This is going to take some digging to see what happened, but since it's a regression we want it fixed before -rc11. |
|
I did a little more digging around in here. It seems commit 858219c make more sense down below in the "if (verbose)" section of the code. Initially, buf and path would never point to the same location. Once path = buf is set on a raidz vdev, the code may drop into the verbose section depending on the verbose flag. In here, using a tmpbuf makes sense since now, buf==path . My initial test with this patch applied to rc-10 shows the correct values. First time patch post for me, so let me if there is a more-appropriate procedure. Results with patch applied: diff --git a/lib/libzfs/libzfs_pool.c b/lib/libzfs/libzfs_pool.c
index 68bfdee..4f2de99 100644
--- a/lib/libzfs/libzfs_pool.c
+++ b/lib/libzfs/libzfs_pool.c
@@ -3205,14 +3205,14 @@ zpool_vdev_name(libzfs_handle_t *hdl, zpool_handle_t *zhp, nvlist_t *nv,
/*
* If it's a raidz device, we need to stick in the parity level.
*/
+
if (strcmp(path, VDEV_TYPE_RAIDZ) == 0) {
- char tmpbuf[PATH_BUF_LEN];
verify(nvlist_lookup_uint64(nv, ZPOOL_CONFIG_NPARITY,
&value) == 0);
- (void) snprintf(tmpbuf, sizeof (tmpbuf), "%s%llu", path,
+ (void) snprintf(buf, sizeof (buf), "%s%llu", path,
(u_longlong_t)value);
- path = tmpbuf;
+ path = buf;
}
/*
@@ -3220,13 +3220,14 @@ zpool_vdev_name(libzfs_handle_t *hdl, zpool_handle_t *zhp, nvlist_t *nv,
* naming convention.
*/
if (verbose) {
+ char tmpbuf[PATH_BUF_LEN];
uint64_t id;
verify(nvlist_lookup_uint64(nv, ZPOOL_CONFIG_ID,
&id) == 0);
- (void) snprintf(buf, sizeof (buf), "%s-%llu", path,
+ (void) snprintf(tmpbuf, sizeof (tmpbuf), "%s-%llu", path,
(u_longlong_t)id);
- path = buf;
+ path = tmpbuf;
}
} |
|
@mgmartin: Your fix looks good to me. When submit a patch you'll want to open a pull request with the change. This allows me to easily review it, merge it, and properly attribute the path. Here's a link which describes the process: |
|
@mgmartin I'd like to get this merged but I want to properly attribute you with the fix. I'm happy to do the leg work merging the patch but I need a name and email address from you for the commit author field. |
|
Thanks, Brian. My public email should be visible here now. I'll work on getting familiar with the pull requests features of git hub for future fixes. |
|
Great, I'll get it done tomorrow. |
|
In the if (verbose) case the tmpbuf variable is local to this if clause and "path = tmpbuf" sends its address beyond the if () construct. Does it feel right ? |
|
tmpbuf goes out of scope, but the stack allocated array--which path now points to--remains on the stack through the return call where zfs_strdup returns a copy of the array. Moving tmpbuf scope up would always allocated the space on the stack--even when verbose was false. |
|
Indeed, I'll widen the scope of tmpbuf. I'm a bit surprised gcc didn't catch this. |
|
I think it's fine where it's at. My point was it could be moved up, but where it is now shouldn't cause any issues, and avoids creating the buffer if it's not going to be used. |
|
It works, but it doesn't feel safe. At that point we're just counting on the fact that nothing else will touch that bit of the stack. It happens to be true right now, but another code block added latter in the function could cause that chunk of the stack to be overwritten and that would be a pain to debug. |
|
Good point. I was assuming the stack wasn't really freed until the entire method was finished. I'll have too look into how that works, but better safe. Follow up: My assumption was wrong. A quick test program shows how it could definitely be clobbered by later code. Good catch. |
Replace redundant_metadata all value by none, which should be the new default. We cannot extend the set of existing values (all and most), because of compatibility reason. Fix minor ref count leak on mgmt connection due to iscsi target quering zvol which is not associated with mgmt connection. Signed-off-by: Jan Kryl <jan.kryl@cloudbyte.com>
Interestingly the top level vdev type is missing from the zpool status command. It should say 'raidz2-0' below instead of just '-0'. Everything appears OK for other top level vdev types. This is probably a pretty easy bug I just wanted to file it so I don't forget.
$ sudo ./cmd/zpool/zpool status tank pool: tank state: ONLINE scan: none requested config: NAME STATE READ WRITE CKSUM tank ONLINE 0 0 0 -0 ONLINE 0 0 0 loop0 ONLINE 0 0 0 loop1 ONLINE 0 0 0 loop2 ONLINE 0 0 0 loop3 ONLINE 0 0 0$ sudo ./cmd/zpool/zpool status pool: tank state: ONLINE scan: none requested config: NAME STATE READ WRITE CKSUM tank ONLINE 0 0 0 mirror-0 ONLINE 0 0 0 loop0 ONLINE 0 0 0 loop1 ONLINE 0 0 0 mirror-1 ONLINE 0 0 0 loop2 ONLINE 0 0 0 loop3 ONLINE 0 0 0The text was updated successfully, but these errors were encountered: