(1/2) new library interface for vg_read

/* FIXME: remove redundant vg_name */
int vg_remove_single(struct cmd_context *cmd, const char *vg_name,

(quoted diff) +2 -2

-                    struct volume_group *vg, int consistent,
+                    struct volume_group *vg,
                     force_t force __attribute((unused)))
 {
        struct physical_volume *pv;

@@ -340,7 +340,7 @@ int vg_remove_single(struct cmd_context 
        unsigned lv_count;
        int ret = 1;
 
-       if (!vg || !consistent || vg_missing_pv_count(vg)) {
+       if (vg_read_error(vg) || vg_missing_pv_count(vg)) {
                log_error("Volume group \"%s\" not found, is

inconsistent "

                          "or has PVs missing.", vg_name);
                log_error("Consider vgreduce --removemissing if

metadata "

Quoted 11 lines · Petr Rockai

/* FIXME: remove redundant vg_name */
int vg_remove_single(struct cmd_context *cmd, const char *vg_name,

(quoted diff) +1 -1

-                    struct volume_group *vg, int consistent,
+                    struct volume_group *vg,
                     force_t force __attribute((unused)))
 {
        struct physical_volume *pv;

@@ -340,7 +340,7 @@ int vg_remove_single(struct cmd_context 
        unsigned lv_count;
        int ret = 1;
 

Quoted 7 lines

(quoted diff) +1 -1

-       if (!vg || !consistent || vg_missing_pv_count(vg)) {
+       if (vg_read_error(vg) || vg_missing_pv_count(vg)) {
                log_error("Volume group \"%s\" not found, is

inconsistent "

                          "or has PVs missing.", vg_name);
                log_error("Consider vgreduce --removemissing if

metadata "

Did you mean to put this in the second patch?

Quoted 12 lines · Dave Wysochanski

Quoted 11 lines · Petr Rockai

/* FIXME: remove redundant vg_name */
int vg_remove_single(struct cmd_context *cmd, const char *vg_name,

(quoted diff) +1 -1

-                    struct volume_group *vg, int consistent,
+                    struct volume_group *vg,
                     force_t force __attribute((unused)))
 {
        struct physical_volume *pv;

@@ -340,7 +340,7 @@ int vg_remove_single(struct cmd_context 
        unsigned lv_count;
        int ret = 1;
 

Quoted 9 lines

Quoted 7 lines

(quoted diff) +1 -1

-       if (!vg || !consistent || vg_missing_pv_count(vg)) {
+       if (vg_read_error(vg) || vg_missing_pv_count(vg)) {
                log_error("Volume group \"%s\" not found, is

inconsistent "

                          "or has PVs missing.", vg_name);
                log_error("Consider vgreduce --removemissing if

metadata "

Did you mean to put this in the second patch?

Actually, hard to tell. It, after all, is part of the library, for a reason or other. It belongs equally well to the first as to the second patch. What do you propose?

Well, doesn't this cause a bug in this path though for the !consistent case? First patch drops the consistent variable in this vgremove path, but things like _process_one_vg() do not use the new APIs (which sets the proper error codes) until the second patch. You mention in your patch intro this first patch should apply and make a completely workable LVM so I'm assuming you mean no bugs.

Hi,

Quoted 6 lines · Dave Wysochanski

Well, doesn't this cause a bug in this path though for the !consistent case? First patch drops the consistent variable in this vgremove path, but things like _process_one_vg() do not use the new APIs (which sets the proper error codes) until the second patch. You mention in your patch intro this first patch should apply and make a completely workable LVM so I'm assuming you mean no bugs.

Oh, hm, you are apparently right. Interestingly enough, the presumably buggy code does pass the testsuite, so something's wrong with the testsuite, I suppose. It might be good to have a test triggering that problem. I will re-split the patch and resubmit later today.

diff -rN -p -u old-lvmlib/lib/device/dev-io.c
new-lvmlib/lib/device/dev-io.c
--- old-lvmlib/lib/device/dev-io.c 2008-12-09 17:44:06.638398136
+0100
+++ new-lvmlib/lib/device/dev-io.c 2008-12-09 17:44:06.930396980
+0100

(quoted diff) +0 -0

@@ -622,6 +622,11 @@ int dev_write(struct device *dev, uint64
        where.start = offset;

Review int dev_write(struct device *dev, uint64 · line 625

Quoted 7 lines · Petr Rockai

+++ new-lvmlib/lib/device/dev-io.c 2008-12-09 17:44:06.930396980
+0100

(quoted diff) +3 -0

@@ -622,6 +622,11 @@ int dev_write(struct device *dev, uint64
 
+       if (!vg_write_lock_held()) {
+               log_error("Attempted to write unlocked VG's device.");
+               return_0;

(quoted diff) +1 -1

-int vg_check_status(const struct volume_group *vg, uint32_t status)
+static int _vg_check_status(const struct volume_group *vg, uint32_t

status)
{
+ uint32_t ret = 0;

Can you clean up some of the return value prototypes so they use uint32_t consistently for return values? This includes vg_check_status(), vg_read_error(), vg_exists(), and the flag definitions.

Hi,

Quoted 4 lines · Dave Wysochanski

Can you clean up some of the return value prototypes so they use uint32_t consistently for return values? This includes vg_check_status(), vg_read_error(), vg_exists(), and the flag definitions.

I have changed those, plus removed the warning you mentioned in other mail, I'll probably re-add separately later. I'm attaching new-changes and new-all, first just giving the diff against the previous version of the patch. I'm not sure I agree that they should all be uint32_t though, but I suppose it's a minor point.