Re: [libvirt] [PATCH 5/8 v8] qemu: set unpriv_sgio when domain starting

On 12/14/2012 08:43 AM, Osier Yang wrote: > Note that cdbfilter is not exactly match unpriv_sgio's value. > VIR_DOMAIN_DISK_CDB_FILTER_YES mean the kernel will filter the > SCSI commands, which maps to 0 of unpriv_sgio; > VIR_DOMAIN_DISK_CDB_FILTER_NO maps to 1 of unpriv_sgio. Again, I find this confusing to have negative logic, and think making rawio a tri-state will make it easier to reason about which disks have allowed unpriv_sgio vs. the default of being filtered. > --- > src/qemu/qemu_process.c | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index c3ecf6f..bcea0ff 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3730,10 +3730,19 @@ int qemuProcessStart(virConnectPtr conn, > _("Raw I/O is not supported on this platform")); > #endif > > - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&& disk->shared) { > + if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&& disk->shared) { The re-indent could be pushed independently and prior to the release (or, if it was introduced earlier in the series, then fix that earlier patch).

> if (qemuAddSharedDisk(driver->sharedDisks, disk->src)< 0) > goto cleanup; > } > + > + /* Set sysfs unpriv_sgio if cdbfilter is specified */ > + if (disk->cdbfilter) { > + if (virSetDeviceUnprivSGIO(disk->src, NULL, > + (disk->cdbfilter == > + VIR_DOMAIN_DISK_CDB_FILTER_NO) > + ? 1 : 0)< 0) > + goto cleanup; > + } Would it be any simpler to write: virSetDeviceUnprivSGIO(disk->src, NULL, disk->cdbfilter != VIR_DOMAIN_DISK_CDB_FILTER_NO)< 0 but you may be rewriting this anyways.