Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
On Wed, May 05, 2010 at 08:54:35PM +0200, Jim Meyering wrote:
Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when
connecting as qemu:///session, since my uid could stat /dev/sdb
but not open it. That test now skips for unprivileged users, as well
as adds a layer of sanity checking against expected size to avoid
trashing the wrong device.
Can we provide the option to specify the device serial number so that
it's really impossible to trash the wrong device?
* conf/default.cfg (host_block_devices): Document optional size.
* lib/Sys/Virt/TCK.pm (get_host_block_device): If optional size is
supplied, skip a device that does not match. Also, avoid devices
that can't be opened.
---
Go easy on me - I'm not that fluent in perl (yet); if there's
a better way to do the sanity check, I'm all ears.
The overall approach sounds fine, from my limited perspective.
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
...
-return $self-config(host_block_devices/[$devindex], undef);
+my $device = $self-config(host_block_devices/[$devindex]/path,
undef);
+my $size = $self-config(host_block_devices/[$devindex]/size, 0);
+
+if (!defined $device) {
+ $device = $self-config(host_block_devices/[$devindex], undef);
+}
This can be equivalently (idiomatically) written as:
$device ||= $self-config(host_block_devices/[$devindex], undef);
I prefer that, since it eliminates one use of $device.
+if ($size) {
+ sysopen(BLK, $device, O_RDONLY) or return undef;
+ return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024;
+ close(BLK);
+}
(Note no need for double quotes around $device;
Leaving parens off of some built-ins like close is a personal
preference (less syntax is better), but obviously keeping in sync
with the prevailing style is more important)
This is similar, but doesn't leave BLK open upon failure or size mismatch:
if ($size) {
my $match = (sysopen (BLK, $device, O_RDONLY)
sysseek (BLK, 0, SEEK_END) == $size * 1024);
close BLK;
$match or return undef;
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
Dave Allan wrote: On Wed, May 05, 2010 at 08:54:35PM +0200, Jim Meyering wrote: Eric Blake wrote: I was getting failures of domain/103-blockdev-save-restore.t when connecting as qemu:///session, since my uid could stat /dev/sdb but not open it. That test now skips for unprivileged users, as well as adds a layer of sanity checking against expected size to avoid trashing the wrong device. Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device? Good point. That would be even better. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
Dave Allan wrote:
On Wed, May 05, 2010 at 08:54:35PM +0200, Jim Meyering wrote:
Eric Blake wrote:
I was getting failures of domain/103-blockdev-save-restore.t when
connecting as qemu:///session, since my uid could stat /dev/sdb
but not open it. That test now skips for unprivileged users, as well
as adds a layer of sanity checking against expected size to avoid
trashing the wrong device.
Can we provide the option to specify the device serial number so that
it's really impossible to trash the wrong device?
Given that this is a good idea, next question is obviously
how to get the serial number. One way seems to be via hdparm,
e.g., hdparm -i /dev/sda
/dev/sda:
Model=ST3320620AS, FwRev=3.AAK, SerialNo=9QF6ET0H
Config={ HardSect NotMFM HdSw15uSec Fixed DTR10Mbs RotSpdTol.5% }
RawCHS=16383/16/63, TrkSize=0, SectSize=0, ECCbytes=4
BuffType=unknown, BuffSize=16384kB, MaxMultSect=16, MultSect=off
CurCHS=16383/16/63, CurSects=16514064, LBA=yes, LBAsects=625142448
IORDY=on/off, tPIO={min:120,w/IORDY:120}, tDMA={min:120,rec:120}
PIO modes: pio0 pio1 pio2 pio3 pio4
DMA modes: mdma0 mdma1 mdma2
UDMA modes: udma0 udma1 udma2 udma3 udma4 udma5 *udma6
AdvancedPM=no WriteCache=enabled
Drive conforms to: Unspecified: ATA/ATAPI-1,2,3,4,5,6,7
* signifies the current active mode
which reminds me that some devices have a serial number of all zeros,
so maybe to be truly bullet-proof you'd want all three from that
first line: Model, FwRev, SerialNo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
On 05/05/2010 01:31 PM, Jim Meyering wrote: Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device? Given that this is a good idea, next question is obviously how to get the serial number. One way seems to be via hdparm, e.g., hdparm -i /dev/sda /dev/sda: Model=ST3320620AS, FwRev=3.AAK, SerialNo=9QF6ET0H Great for SCSI, not so great for USB sticks: # hdparm -i /dev/sdb /dev/sdb: HDIO_DRIVE_CMD(identify) failed: Invalid exchange HDIO_GET_IDENTITY failed: Invalid argument # echo $? 22 -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
On 05/05/2010 12:54 PM, Jim Meyering wrote:
+if (!defined $device) {
+$device = $self-config(host_block_devices/[$devindex], undef);
+}
This can be equivalently (idiomatically) written as:
$device ||= $self-config(host_block_devices/[$devindex], undef);
Thanks for the tip.
+if ($size) {
+sysopen(BLK, $device, O_RDONLY) or return undef;
+return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024;
+close(BLK);
+}
(Note no need for double quotes around $device;
Leaving parens off of some built-ins like close is a personal
preference (less syntax is better), but obviously keeping in sync
with the prevailing style is more important)
This is similar, but doesn't leave BLK open upon failure or size mismatch:
Aargh. I noticed that too, and even have it in my editor, but I hadn't
hit save before I did 'git commit'. But your alternative
if ($size) {
my $match = (sysopen (BLK, $device, O_RDONLY)
sysseek (BLK, 0, SEEK_END) == $size * 1024);
close BLK;
$match or return undef;
}
is even shorter that what 'git diff' says was still in my editor:
diff --git i/lib/Sys/Virt/TCK.pm w/lib/Sys/Virt/TCK.pm
index fc325a3..1fcfdf0 100644
--- i/lib/Sys/Virt/TCK.pm
+++ w/lib/Sys/Virt/TCK.pm
@@ -835,15 +835,16 @@ sub get_host_block_device {
my $devindex = @_ ? shift : 0;
my $device = $self-config(host_block_devices/[$devindex]/path,
undef);
-my $size = $self-config(host_block_devices/[$devindex]/size, 0);
+my $blocks = $self-config(host_block_devices/[$devindex]/size, 0);
if (!defined $device) {
$device = $self-config(host_block_devices/[$devindex], undef);
}
-if ($size) {
+if ($blocks) {
sysopen(BLK, $device, O_RDONLY) or return undef;
- return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024;
+ my $size = sysseek(BLK, 0, SEEK_END);
close(BLK);
+ return undef unless $size == $blocks * 1024;
}
return $device;
}
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
Eric Blake wrote:
On 05/05/2010 12:54 PM, Jim Meyering wrote:
+if (!defined $device) {
+ $device = $self-config(host_block_devices/[$devindex], undef);
+}
This can be equivalently (idiomatically) written as:
$device ||= $self-config(host_block_devices/[$devindex], undef);
Thanks for the tip.
+if ($size) {
+ sysopen(BLK, $device, O_RDONLY) or return undef;
+ return undef unless sysseek(BLK, 0, SEEK_END) == $size * 1024;
+ close(BLK);
+}
(Note no need for double quotes around $device;
Leaving parens off of some built-ins like close is a personal
preference (less syntax is better), but obviously keeping in sync
with the prevailing style is more important)
This is similar, but doesn't leave BLK open upon failure or size mismatch:
Aargh. I noticed that too, and even have it in my editor, but I hadn't
hit save before I did 'git commit'. But your alternative
if ($size) {
my $match = (sysopen (BLK, $device, O_RDONLY)
sysseek (BLK, 0, SEEK_END) == $size * 1024);
close BLK;
$match or return undef;
}
is even shorter that what 'git diff' says was still in my editor:
Your $blocks is better than $size, but
how about $kb_blocks instead blocks, to distinguish from
the relatively common connotation of 512-byte blocks.
diff --git i/lib/Sys/Virt/TCK.pm w/lib/Sys/Virt/TCK.pm
index fc325a3..1fcfdf0 100644
--- i/lib/Sys/Virt/TCK.pm
+++ w/lib/Sys/Virt/TCK.pm
@@ -835,15 +835,16 @@ sub get_host_block_device {
my $devindex = @_ ? shift : 0;
my $device = $self-config(host_block_devices/[$devindex]/path,
undef);
-my $size = $self-config(host_block_devices/[$devindex]/size, 0);
+my $blocks = $self-config(host_block_devices/[$devindex]/size, 0);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
Eric Blake wrote: On 05/05/2010 01:31 PM, Jim Meyering wrote: Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device? Given that this is a good idea, next question is obviously how to get the serial number. One way seems to be via hdparm, e.g., hdparm -i /dev/sda /dev/sda: Model=ST3320620AS, FwRev=3.AAK, SerialNo=9QF6ET0H Great for SCSI, not so great for USB sticks: # hdparm -i /dev/sdb /dev/sdb: HDIO_DRIVE_CMD(identify) failed: Invalid exchange HDIO_GET_IDENTITY failed: Invalid argument # echo $? 22 Using a device path in /dev/disk/by-id/ would make more sense than specifying /dev/sdX if you're concerned about hitting the wrong disk. -jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
On 05/05/2010 02:21 PM, Jim Paris wrote: Great for SCSI, not so great for USB sticks: # hdparm -i /dev/sdb /dev/sdb: HDIO_DRIVE_CMD(identify) failed: Invalid exchange HDIO_GET_IDENTITY failed: Invalid argument # echo $? 22 Using a device path in /dev/disk/by-id/ would make more sense than specifying /dev/sdX if you're concerned about hitting the wrong disk. At this point, I'm happy making default.cfg show an example using /dev/disk/by-id/xxx + size. Look for v2 of the patch coming up soon. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
On Wed, May 05, 2010 at 01:56:20PM -0600, Eric Blake wrote: On 05/05/2010 01:31 PM, Jim Meyering wrote: Can we provide the option to specify the device serial number so that it's really impossible to trash the wrong device? Given that this is a good idea, next question is obviously how to get the serial number. One way seems to be via hdparm, e.g., hdparm -i /dev/sda /dev/sda: Model=ST3320620AS, FwRev=3.AAK, SerialNo=9QF6ET0H Great for SCSI, not so great for USB sticks: # hdparm -i /dev/sdb What does the following give you? scsi_id --whitelisted --replace-whitespace --device=/dev/sdb /dev/sdb: HDIO_DRIVE_CMD(identify) failed: Invalid exchange HDIO_GET_IDENTITY failed: Invalid argument # echo $? 22 -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
On 05/05/2010 02:48 PM, Dave Allan wrote: Great for SCSI, not so great for USB sticks: # hdparm -i /dev/sdb What does the following give you? scsi_id --whitelisted --replace-whitespace --device=/dev/sdb # scsi_id --whitelisted --replace-whitespace --device=/dev/sdb # echo $? 1 Not a peep on stderr (for shame). -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [TCK PATCH] block devices: allow specification of size for safety
On Wed, May 05, 2010 at 02:49:44PM -0600, Eric Blake wrote: On 05/05/2010 02:48 PM, Dave Allan wrote: Great for SCSI, not so great for USB sticks: # hdparm -i /dev/sdb What does the following give you? scsi_id --whitelisted --replace-whitespace --device=/dev/sdb # scsi_id --whitelisted --replace-whitespace --device=/dev/sdb # echo $? 1 Not a peep on stderr (for shame). Ok, looks like that device doesn't return a serial number. For devices that do have serials, especially when there are lots of nearly identical disks on a system, it'd be really helpful. Dave -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
