Hi @ahrens, @grwilson,
Are there news about that patch ?
Best regards,
Ganael.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-393063362
Hi @grwilson,
As a recall, the original problem that led to the patch is described here :
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=204622
> These knobs expose the internals of the product.
Yes, but I don't think exposing the internals of the product is a problem here
because it does
I'll chime in here.
There have been a number of cases in the past where I have wanted the moral
equivalent to dd if=/dev/zero of=/dev/da0 bs=1M
Typically, and this may be FreeBSD specific, it's a case of ZFS thinks the
device is unused, then geom attaches to the device, then ZFS magically sees
One of the guiding principles for zfs is simple administration and it seems
like we're exposing way too many knobs to the administrator. These knobs expose
the internals of the product. For example, if I want to clear the labels, why
wouldn't I just run `zpool labelclear` and have the command
martymac commented on this pull request.
> + if (pread64(fd, , sizeof (vdev_label_t),
+ label_offset(size, l)) != sizeof (vdev_label_t))
+ return (-1);
+
+ if (!force) {
+
martymac commented on this pull request.
> @@ -709,8 +751,12 @@ zpool_do_labelclear(int argc, char **argv)
}
if (zpool_read_label(fd, ) != 0) {
- (void) fprintf(stderr,
- gettext("failed to read label from %s\n"), vdev);
+ if
grwilson commented on this pull request.
I have concerns about the ultimate goal for this command. It's unclear if we
really want to "wipe" or just "forget" about this device.
> @@ -709,8 +751,12 @@ zpool_do_labelclear(int argc, char **argv)
}
if (zpool_read_label(fd, ) != 0)
ahrens approved this pull request.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#pullrequestreview-93366574
--
openzfs-developer
Hi and happy new year :)
I've updated the patch with latest changes from master. If there are no more
comments, can it be merged upstream ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@martymac pushed 1 commit.
29d743b Merge branch 'master' of https://github.com/openzfs/openzfs into
zpool-labelclear-improvements
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@ahrens Have you had time to take a deeper look at latest changes ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-337288816
@martymac I'll take a look at your latest changes.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-328663255
--
Compilation warnings have been fixed and all checks have passed.
@ahrens @rmustacc @yuripv Do you have additional comments regarding that PR ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@martymac pushed 1 commit.
7be4c74 Move VDEV_LABELS definition to sys/fs/zfs.h
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
martymac commented on this pull request.
> @@ -47,6 +48,7 @@
#include
#include
#include
+#include
Yes, this is needed just for VDEV_LABELS. You mean moving VDEV_LABELS
definition to sys/fs/zfs.h ? I don't know what that would imply for other parts
of the code :/ Help would be welcome
Hi Matthew,
I've pushed fixes following your recommendations, thanks :)
Only remains warnings about abd stuff. Any hint ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@ahrens Any thoughts on that new patch ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/424#issuecomment-326551259
--
openzfs-developer
@ahrens Hi Matthew,
Well, I have modified the patch to try to keep simplicity as well as
flexibility :
- As you suggest, always check label validity by default (but one can use the
-f flag twice to override that behaviour)
- Use the minimal mode by default but provide an option (-w) to wipe
@martymac pushed 4 commits.
dc767f1 Check label validity by default and remove option '-c'
81bb4c6 Invalidate labels the minimal way by default
8c9eeb3 Rewrite boolean test for more readability
e0dbeda Specify -f twice to treat invalid labels as valid
--
You are receiving this because you
I'm not convinced that people are "used to the fact that this command blindly
zeroes 512KiB at the beginning and the end of the disk". I think that this
command is seldom-used, and when it's used it's for the documented purpose:
"Removes ZFS label information." That said, I'm open to evidence
@ahrens Hi Matthew, what do you think about my previous comments ? Can we leave
the default options as they are ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
This adds quite a lot of flexibility to an already obscure subcommand, making
it even harder to know how to use correctly. Is there any reason you wouldn't
always want the `-cm` behavior? What would you think about changing `zpool
labelclear` (without any flags) to always make the minimal
Hi,
Thanks for your feedback.
C99LMODE seems to be derived from C99MODE (see Makefile.master) so setting
C99MODE only should be enough ? I've modified the Makefile accordingly.
Regards,
Ganael.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly
yuripv approved this pull request.
This looks good to me except for the small nit in the Makefile, thanks!
> INCS += -I../../common/zfs -I$(STATCOMMONDIR)
+C99MODE= -xc99=%all
Please use the $(C99_ENABLE) macro instead for both.
--
You are receiving this because you are subscribed to
@martymac pushed 6 commits.
ccdede9 Use strtonum() instead of strtoll()
31452fc Add buflen check to nvlist_invalidate()
c876bde Move nvlist_invalidate symbol to SUNWprivate_1.1
1561da9 Simplify boolean check syntax
72b508c Avoid compilation warning related to ignored return value of memset()
rmustacc commented on this pull request.
> @@ -2355,6 +2355,18 @@ nvlist_common(nvlist_t *nvl, char *buf, size_t
> *buflen, int encoding,
}
int
+nvlist_invalidate(char *buf)
Shouldn't this function be taking the size of the buffer in question that we're
modifying? I realize that we're
yuripv commented on this pull request.
> @@ -198,6 +198,7 @@ SYMBOL_VERSION SUNW_1.1 {
nvlist_alloc;
nvlist_dup;
nvlist_free;
+ nvlist_invalidate;
You can't add the symbols to already existing public version, either move it to
SUNWprivate_1.1, or provide new
27 matches
Mail list logo
