On Fri, 2018-11-30 at 15:41 +0100, David Disseldorp wrote: > On Fri, 30 Nov 2018 14:17:49 +0100, David Disseldorp wrote: > > > > Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I > > > perhaps overlook something? > > > > This is done in target_configure_device() . > > Hmm, continuing to do it there may cause a bit of confusion if vendor_id > is set before the backstore is enabled, which would cause it to be > overwritten. That said, we already have similarly strange behaviour for > emulate_model_alias / product. E.g.: > > rapido1:/# cd /sys/kernel/config/target/ > rapido1:target# mkdir -p core/fileio_1/testing > rapido1:target# echo -n "fd_dev_name=/asdf,fd_dev_size=4096" > > core/fileio_1/testing/control > rapido1:target# echo -n "testing1" > core/fileio_1/testing/wwn/vendor_id > rapido1:target# cat core/fileio_1/testing/wwn/vendor_id > testing1 > rapido1:target# echo 1 > ./core/fileio_1/testing/attrib/emulate_model_alias > rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod > testing > rapido1:target# echo 1 > core/fileio_1/testing/enable > rapido1:target# cat core/fileio_1/testing/wwn/vendor_id > LIO-ORG > rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod > FILEIO > rapido1:target# cat ./core/fileio_1/testing/attrib/emulate_model_alias > 1 > > Not sure how best to handle this... > 1. move vendor/model/rev initialization into target_alloc_device() > 2. move vendor (only) initialization into target_alloc_device() > 3. fail attempts to set emulate_model_alias or vendor_id before the > backstore has been enabled > 4. leave as-is > > (1) would IMO be the most straightforward, but it's a slight change to > the existing (IMO broken) emulate_model_alias user interface.
I'm in favor of moving some of the target_configure_device() code into the target_alloc_device() function. Today target_configure_device() overwrites the vendor, model and revision string and is called when "1" is written into the "enable" attribute. Overwriting these attributes when enabling a backstore seems wrong to me. Thanks, Bart.
