On Wed, Jul 04, 2012 at 05:54:16PM +0200, Paolo Bonzini wrote: > Il 04/07/2012 17:42, Michael S. Tsirkin ha scritto: > > On Tue, Jul 03, 2012 at 03:19:37PM +0200, Paolo Bonzini wrote: > >> This patch adds support for the new VIRTIO_BLK_F_CONFIG_WCE feature, > >> which exposes the cache mode in the configuration space and lets the > >> driver modify it. The cache mode is exposed via sysfs. > >> > >> Even if the host does not support the new feature, the cache mode is > >> visible (thanks to the existing VIRTIO_BLK_F_WCE), but not modifiable. > >> > >> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx> > > > > I note this has been applied but I think the userspace > > API is a bit painful to use. Let's fix it before > > it gets set in stone? > > I'm trying to mimic the existing userspace API for SCSI disks. FWIW I > would totally agree with you. Hmm. Want to try fixing scsi? Need to be compatible but it could maybe ignore spaces. > >> +static int virtblk_get_cache_mode(struct virtio_device *vdev) > > > > Why are you converting u8 to int here? > > The fact that it is a u8 is really an internal detail. Perhaps the bug > is using u8 in the callers. Make it bool then? You are using u8 in the config. So you could get any value besides 0 and 1, and you interpret that as 1. Is 1 always a safe value? If not maybe it's better to set to a safe value if it is not 0 or 1, that is we don't know how to interpret it. > >> +static const char *virtblk_cache_types[] = { > >> + "write through", "write back" > >> +}; > >> + > > > > I wonder whether something that lacks space would have been better, > > especially for show: shells might get confused and split > > a string at a space. How about we change it to writethrough, > > writeback before it's too late? It's part of a userspace API > > after all, and you see to prefer writeback in one word in your own > > code so let's not inflict pain on others :) > > > > Also, would be nice to make it discoverable what the legal > > values are. Another attribute valid_cache_types with all values > > space separated? > > We probably should add such an attribute to SCSI disks too... Even with > the spaces we could make the values \n-separated. > > >> +static ssize_t > >> +virtblk_cache_type_store(struct device *dev, struct device_attribute *attr, > >> + const char *buf, size_t count) > >> +{ > >> + struct gendisk *disk = dev_to_disk(dev); > >> + struct virtio_blk *vblk = disk->private_data; > >> + struct virtio_device *vdev = vblk->vdev; > >> + int i; > >> + u8 writeback; > >> + > >> + BUG_ON(!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_CONFIG_WCE)); > >> + for (i = ARRAY_SIZE(virtblk_cache_types); --i >= 0; ) > >> + if (sysfs_streq(buf, virtblk_cache_types[i])) > >> + break; > >> + > >> + if (i < 0) > >> + return -EINVAL; > >> + > >> + writeback = i; > >> + vdev->config->set(vdev, > >> + offsetof(struct virtio_blk_config, wce), > >> + &writeback, sizeof(writeback)); > >> + > >> + virtblk_update_cache_mode(vdev); > >> + return count; > >> +} > >> + > >> +static ssize_t > >> +virtblk_cache_type_show(struct device *dev, struct device_attribute *attr, > >> + char *buf) > >> +{ > >> + struct gendisk *disk = dev_to_disk(dev); > >> + struct virtio_blk *vblk = disk->private_data; > >> + u8 writeback = virtblk_get_cache_mode(vblk->vdev); > >> + > >> + BUG_ON(writeback >= ARRAY_SIZE(virtblk_cache_types)); > >> + return snprintf(buf, 40, "%s\n", virtblk_cache_types[writeback]); > > > > Why 40? Why snprintf? A plain sprintf won't do? > > I plead guilty of cut-and-paste from drivers/scsi/sd.c. :) > > Paolo _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization