Re: [libvirt] [PATCH 1/1] Add shrink flag to blockresize command
On Tue, Oct 15, 2019 at 10:46 PM Cole Robinson wrote:
> On 10/5/19 2:56 PM, martinsson.pat...@gmail.com wrote:
> > From: patchon
> >
>
> You probably want to fix this to match your signed-off-by name.
>
> > These commits simply adds the '--shrink' flag to the blockresize
>
I agree that an options should be added here to avoid undesirable disk
shrinking.
Because I have broken guest OS image for several times due to the shrinking
of
blockresize.
> > command to prevent accidental shrinking of a block device. This
> > behaviour is already present on the vol-resize command and it
> > makes sense to mimic that behaviour.
> >
>
> I don't have an opinion on whether the feature should be added at the
> API level, I will leave that to others. CCing pkrempa
>
> But as implemented it seems problematic to add a flag that changes the
> semantics of the API, essentially requiring a new flag to get the old
> behavior. I see the argument that this is a data safety issue, but maybe
> apps are depending on this already? I don't know enough about the usage
> to guess either way
>
> I disagree the check of disk shrink is added to API level because it
changes
the behavior of blockresize API and may cause unexpected error in uplayer
products.
It is better to add the check to codes of **virsh** to avoid undesirable
shrinking
in cmdline user. And we can use '--force' to make blockresize always.
The procedure will be:
if (new size < capacity && no --force)
print error("Can't shrink capacity below current unless --force
specified")
else
call blockresize
And also we can add comments to blockresize API to avoid uplayer use
undesirable disk shrinking.
> > Only implemented in the qemu-driver atm.
> >
> > Signed-off-by: Patrik Martinsson
> > ---
> > src/qemu/qemu_driver.c | 15 ++-
> > tools/virsh-domain.c | 7 +++
> > tools/virsh.pod| 9 -
> > 3 files changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 95fe844c34..4e34eec796 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -11265,8 +11265,10 @@ qemuDomainBlockResize(virDomainPtr dom,
> > char *device = NULL;
> > const char *nodename = NULL;
> > virDomainDiskDefPtr disk = NULL;
> > +virDomainBlockInfo info;
> >
> > -virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES, -1);
> > +virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES |
> > + VIR_STORAGE_VOL_RESIZE_SHRINK, -1);
> >
>
> We shouldn't mix and match flag prefix names. Each public API should be
> coupled with its own set of flag names. So you would want to call this
> VIR_DOMAIN_BLOCK_RESIZE_SHRINK, and add it to
> include/libvirt/libvirt-domain.h and add it to documentation in
> libvirt-domain.c
>
> - Cole
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Best regards,
---
Han Han
Quality Engineer
Redhat.
Email: h...@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Add shrink flag to blockresize command
On 10/5/19 2:56 PM, martinsson.pat...@gmail.com wrote: > From: patchon > You probably want to fix this to match your signed-off-by name. > These commits simply adds the '--shrink' flag to the blockresize > command to prevent accidental shrinking of a block device. This > behaviour is already present on the vol-resize command and it > makes sense to mimic that behaviour. > I don't have an opinion on whether the feature should be added at the API level, I will leave that to others. CCing pkrempa But as implemented it seems problematic to add a flag that changes the semantics of the API, essentially requiring a new flag to get the old behavior. I see the argument that this is a data safety issue, but maybe apps are depending on this already? I don't know enough about the usage to guess either way > Only implemented in the qemu-driver atm. > > Signed-off-by: Patrik Martinsson > --- > src/qemu/qemu_driver.c | 15 ++- > tools/virsh-domain.c | 7 +++ > tools/virsh.pod| 9 - > 3 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 95fe844c34..4e34eec796 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -11265,8 +11265,10 @@ qemuDomainBlockResize(virDomainPtr dom, > char *device = NULL; > const char *nodename = NULL; > virDomainDiskDefPtr disk = NULL; > +virDomainBlockInfo info; > > -virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES, -1); > +virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES | > + VIR_STORAGE_VOL_RESIZE_SHRINK, -1); > We shouldn't mix and match flag prefix names. Each public API should be coupled with its own set of flag names. So you would want to call this VIR_DOMAIN_BLOCK_RESIZE_SHRINK, and add it to include/libvirt/libvirt-domain.h and add it to documentation in libvirt-domain.c - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/1] Add shrink flag to blockresize command
From: patchon
These commits simply adds the '--shrink' flag to the blockresize
command to prevent accidental shrinking of a block device. This
behaviour is already present on the vol-resize command and it
makes sense to mimic that behaviour.
Only implemented in the qemu-driver atm.
Signed-off-by: Patrik Martinsson
---
src/qemu/qemu_driver.c | 15 ++-
tools/virsh-domain.c | 7 +++
tools/virsh.pod| 9 -
3 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 95fe844c34..4e34eec796 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11265,8 +11265,10 @@ qemuDomainBlockResize(virDomainPtr dom,
char *device = NULL;
const char *nodename = NULL;
virDomainDiskDefPtr disk = NULL;
+virDomainBlockInfo info;
-virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES, -1);
+virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES |
+ VIR_STORAGE_VOL_RESIZE_SHRINK, -1);
/* We prefer operating on bytes. */
if ((flags & VIR_DOMAIN_BLOCK_RESIZE_BYTES) == 0) {
@@ -11279,6 +11281,9 @@ qemuDomainBlockResize(virDomainPtr dom,
size *= 1024;
}
+if (virDomainGetBlockInfo(dom, path, , 0) < 0)
+goto cleanup;
+
if (!(vm = qemuDomainObjFromDomain(dom)))
goto cleanup;
@@ -11299,6 +11304,14 @@ qemuDomainBlockResize(virDomainPtr dom,
goto endjob;
}
+if (size < info.capacity &&
+!(flags & VIR_STORAGE_VOL_RESIZE_SHRINK)) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("Can't shrink capacity below current "
+ "capacity unless shrink flag explicitly specified"));
+goto endjob;
+}
+
/* qcow2 and qed must be sized on 512 byte blocks/sectors,
* so adjust size if necessary to round up.
*/
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index fbfdc09c0d..a67ee35428 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2920,6 +2920,10 @@ static const vshCmdOptDef opts_blockresize[] = {
.flags = VSH_OFLAG_REQ,
.help = N_("New size of the block device, as scaled integer (default
KiB)")
},
+{.name = "shrink",
+ .type = VSH_OT_BOOL,
+ .help = N_("allow the resize to shrink the volume")
+},
{.name = NULL}
};
@@ -2938,6 +2942,9 @@ cmdBlockresize(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptScaledInt(ctl, cmd, "size", , 1024, ULLONG_MAX) < 0)
return false;
+if (vshCommandOptBool(cmd, "shrink"))
+flags |= VIR_STORAGE_VOL_RESIZE_SHRINK;
+
/* Prefer the older interface of KiB. */
if (size % 1024 == 0)
size /= 1024;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index cf2798e71a..96387127d6 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1027,7 +1027,7 @@ I specifies copying bandwidth limit in MiB/s.
For further information
on the I argument see the corresponding section for the B
command.
-=item B I I I
+=item B I I I<--size> I<--shrink>
Resize a block device of domain while the domain is running, I
specifies the absolute path of the block device; it corresponds
@@ -1040,6 +1040,13 @@ I is a scaled integer (see B above) which
defaults to KiB
"B" to get bytes (note that for historical reasons, this differs from
B which defaults to bytes without a suffix).
+Attempts to shrink the block device will fail unless I<--shrink> is present.
+The I cannot be negative since the given capacity is the absolute
+size of the block device. Always exercise caution when shrinking a block
device.
+B that it is up to the driver to implement the check if shrinking
+should be allowed or not depending on if I<--shrink> is set. So far only the
+qemu-driver does this.
+
=item B I [I] [I<--safe>] [I<--force>]
Connect the virtual serial console for the guest. The optional
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
