Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary (rewrite)

2014-05-12 Thread Ján Tomko 
On 05/08/2014 06:02 PM, John Ferlan wrote:
 A post commit id 'e3d66229' review (and followup):
 
 http://www.redhat.com/archives/libvir-list/2014-May/msg00268.html
 
 noted some issues with the code, so I have adjusted the code
 accordingly. The difference between this and the commit prior
 to the change (commit id 'f3be5f0c') will just be the check for
 qcow2/qed using a non 512 block aligned size will result in a
 round up of size. If the size is within the last 512 bytes to
 ULLONG_MAX, then just set it there rather than erroring out.

This patch essentially reverts commit e3d66229.
Reverting it explicitly using 'git revert' and basing this patch on that would
make it smaller and maybe easier to backport to the -maint branches.

If you choose to do an explicit revert, please include the bugzilla link in
the commit message.

 
 Signed-off-by: John Ferlan jfer...@redhat.com
 ---
  src/qemu/qemu_driver.c | 28 +---
  1 file changed, 9 insertions(+), 19 deletions(-)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 4ff8a2d..8771cae 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -9413,7 +9413,6 @@ qemuDomainBlockResize(virDomainPtr dom,
  virDomainObjPtr vm;
  qemuDomainObjPrivatePtr priv;
  int ret = -1, idx;
 -unsigned long long size_up;
  char *device = NULL;
  virDomainDiskDefPtr disk = NULL;
  
 @@ -9434,12 +9433,6 @@ qemuDomainBlockResize(virDomainPtr dom,
  return -1;
  }
  size *= 1024;
 -size_up = size;
 -} else {
 -/* For 'qcow2' and 'qed', qemu resize blocks expects values
 - * on sector boundary, so round our value up to prepare
 - */
 -size_up = VIR_ROUND_UP(size, 512);
  }
  
  if (!(vm = qemuDomObjFromDomain(dom)))
 @@ -9466,19 +9459,16 @@ qemuDomainBlockResize(virDomainPtr dom,
  }
  disk = vm-def-disks[idx];
  
 -/* qcow2 and qed must be sized appropriately, so be sure our value
 - * is sized appropriately and will fit
 +/* qcow2 and qed must be sized on 512 byte blocks/sectors,
 + * so adjust size if necessary to round up (if possible).
   */
 -if (size != size_up 
 -(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
 - disk-src.format == VIR_STORAGE_FILE_QED)) {
 -if (size_up  ULLONG_MAX) {
 -virReportError(VIR_ERR_OVERFLOW,
 -   _(size must be less than %llu KiB),
 -   ULLONG_MAX / 1024);
 -goto endjob;
 -}
 -size = size_up;
 +if ((disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
 + disk-src.format == VIR_STORAGE_FILE_QED) 
 +(size % 512)) {

The (size % 512) condition is not necessary - VIR_ROUND_UP won't change the
value if the number is divisible by 512 if it does not overflow.

 +if ((ULLONG_MAX - size)  512)
 +size = ULLONG_MAX;

ULLONG_MAX is not divisible by 512 thus I don't think it's any better than
what the user entered.

 +else
 +size = VIR_ROUND_UP(size, 512);
  }
  
  if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,
 

ACK with the ULLONG_MAX assignment removed - whether you do 'git revert' or not.

Jan



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary (rewrite)

2014-05-12 Thread John Ferlan 


On 05/12/2014 07:11 AM, Ján Tomko wrote:
 On 05/08/2014 06:02 PM, John Ferlan wrote:
 A post commit id 'e3d66229' review (and followup):

 http://www.redhat.com/archives/libvir-list/2014-May/msg00268.html

 noted some issues with the code, so I have adjusted the code
 accordingly. The difference between this and the commit prior
 to the change (commit id 'f3be5f0c') will just be the check for
 qcow2/qed using a non 512 block aligned size will result in a
 round up of size. If the size is within the last 512 bytes to
 ULLONG_MAX, then just set it there rather than erroring out.
 
 This patch essentially reverts commit e3d66229.
 Reverting it explicitly using 'git revert' and basing this patch on that would
 make it smaller and maybe easier to backport to the -maint branches.
 
 If you choose to do an explicit revert, please include the bugzilla link in
 the commit message.
 

So with the revert and the changes below this essentially turns into:

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f0e82e9..63a9e77 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9459,6 +9459,13 @@ qemuDomainBlockResize(virDomainPtr dom,
 }
 disk = vm-def-disks[idx];

+/* qcow2 and qed must be sized on 512 byte blocks/sectors,
+ * so adjust size if necessary to round up.
+ */
+if (disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+disk-src.format == VIR_STORAGE_FILE_QED)
+size = VIR_ROUND_UP(size, 512);
+
 if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,
 disk-info.alias)  0)
 goto endjob;


The whole overflow issue for VIR_ROUND_UP and VIR_DIV_UP is something
that could/would/should be handled separately...

John


 Signed-off-by: John Ferlan jfer...@redhat.com
 ---
  src/qemu/qemu_driver.c | 28 +---
  1 file changed, 9 insertions(+), 19 deletions(-)

 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 4ff8a2d..8771cae 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -9413,7 +9413,6 @@ qemuDomainBlockResize(virDomainPtr dom,
  virDomainObjPtr vm;
  qemuDomainObjPrivatePtr priv;
  int ret = -1, idx;
 -unsigned long long size_up;
  char *device = NULL;
  virDomainDiskDefPtr disk = NULL;
  
 @@ -9434,12 +9433,6 @@ qemuDomainBlockResize(virDomainPtr dom,
  return -1;
  }
  size *= 1024;
 -size_up = size;
 -} else {
 -/* For 'qcow2' and 'qed', qemu resize blocks expects values
 - * on sector boundary, so round our value up to prepare
 - */
 -size_up = VIR_ROUND_UP(size, 512);
  }
  
  if (!(vm = qemuDomObjFromDomain(dom)))
 @@ -9466,19 +9459,16 @@ qemuDomainBlockResize(virDomainPtr dom,
  }
  disk = vm-def-disks[idx];
  
 -/* qcow2 and qed must be sized appropriately, so be sure our value
 - * is sized appropriately and will fit
 +/* qcow2 and qed must be sized on 512 byte blocks/sectors,
 + * so adjust size if necessary to round up (if possible).
   */
 -if (size != size_up 
 -(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
 - disk-src.format == VIR_STORAGE_FILE_QED)) {
 -if (size_up  ULLONG_MAX) {
 -virReportError(VIR_ERR_OVERFLOW,
 -   _(size must be less than %llu KiB),
 -   ULLONG_MAX / 1024);
 -goto endjob;
 -}
 -size = size_up;
 +if ((disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
 + disk-src.format == VIR_STORAGE_FILE_QED) 
 +(size % 512)) {
 
 The (size % 512) condition is not necessary - VIR_ROUND_UP won't change the
 value if the number is divisible by 512 if it does not overflow.
 
 +if ((ULLONG_MAX - size)  512)
 +size = ULLONG_MAX;
 
 ULLONG_MAX is not divisible by 512 thus I don't think it's any better than
 what the user entered.
 
 +else
 +size = VIR_ROUND_UP(size, 512);
  }
  
  if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,

 
 ACK with the ULLONG_MAX assignment removed - whether you do 'git revert' or 
 not.
 
 Jan
 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qemu: Adjust size for qcow2/qed if not on sector boundary (rewrite)

2014-05-08 Thread John Ferlan 
A post commit id 'e3d66229' review (and followup):

http://www.redhat.com/archives/libvir-list/2014-May/msg00268.html

noted some issues with the code, so I have adjusted the code
accordingly. The difference between this and the commit prior
to the change (commit id 'f3be5f0c') will just be the check for
qcow2/qed using a non 512 block aligned size will result in a
round up of size. If the size is within the last 512 bytes to
ULLONG_MAX, then just set it there rather than erroring out.

Signed-off-by: John Ferlan jfer...@redhat.com
---
 src/qemu/qemu_driver.c | 28 +---
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4ff8a2d..8771cae 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9413,7 +9413,6 @@ qemuDomainBlockResize(virDomainPtr dom,
 virDomainObjPtr vm;
 qemuDomainObjPrivatePtr priv;
 int ret = -1, idx;
-unsigned long long size_up;
 char *device = NULL;
 virDomainDiskDefPtr disk = NULL;
 
@@ -9434,12 +9433,6 @@ qemuDomainBlockResize(virDomainPtr dom,
 return -1;
 }
 size *= 1024;
-size_up = size;
-} else {
-/* For 'qcow2' and 'qed', qemu resize blocks expects values
- * on sector boundary, so round our value up to prepare
- */
-size_up = VIR_ROUND_UP(size, 512);
 }
 
 if (!(vm = qemuDomObjFromDomain(dom)))
@@ -9466,19 +9459,16 @@ qemuDomainBlockResize(virDomainPtr dom,
 }
 disk = vm-def-disks[idx];
 
-/* qcow2 and qed must be sized appropriately, so be sure our value
- * is sized appropriately and will fit
+/* qcow2 and qed must be sized on 512 byte blocks/sectors,
+ * so adjust size if necessary to round up (if possible).
  */
-if (size != size_up 
-(disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
- disk-src.format == VIR_STORAGE_FILE_QED)) {
-if (size_up  ULLONG_MAX) {
-virReportError(VIR_ERR_OVERFLOW,
-   _(size must be less than %llu KiB),
-   ULLONG_MAX / 1024);
-goto endjob;
-}
-size = size_up;
+if ((disk-src.format == VIR_STORAGE_FILE_QCOW2 ||
+ disk-src.format == VIR_STORAGE_FILE_QED) 
+(size % 512)) {
+if ((ULLONG_MAX - size)  512)
+size = ULLONG_MAX;
+else
+size = VIR_ROUND_UP(size, 512);
 }
 
 if (virAsprintf(device, %s%s, QEMU_DRIVE_HOST_PREFIX,
-- 
1.9.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list