Re: [libvirt] [PATCH 1/1] Add shrink flag to blockresize command

On 10/5/19 2:56 PM, martinsson.patrik(a)gmail.com wrote: > From: patchon <martinsson.patrik(a)gmail.com&gt; > 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 I disagree the check of disk shrink is added to API level because it

> Only implemented in the qemu-driver atm. > > Signed-off-by: Patrik Martinsson <martinsson.patrik(a)gmail.com&gt; > --- > 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list