On 06/17/2015 04:29 PM, Matthew Rosato wrote:
On 06/16/2015 11:29 PM, Eric Farman wrote:
While working with the hostdev tag and SCSI LUNs, a problem was discovered with the XML schema (see commit message in patch 4). This spawned some further corrections to the handling of the logical unit field throughout libvirt.
This series was split from a single patch, from this feedback: http://www.redhat.com/archives/libvir-list/2015-June/msg00489.html
Eric Farman (5): Print SCSI logical unit as a positive integer Print SCSI logical unit as unsigned integer Convert SCSI logical unit from int to long long docs: Fix XML schema handling of LUN address in hostdev tag docs: Correct typos in scsi hostdev and address elements
I get the value of small patches & agree with the way patches 4 and 5 are split out, but patch 1 and 2 are completely replaced by patch 3 with a different type. These are pretty straightforward changes, so I'd suggest squashing patches 1-3 as a single patch that just goes from signed int --> unsigned long long and has a commit that explains why we needed to change both size and sign... I see now that it was a v1 comment from John, so I'll leave it to his judgment. The recommendation came from John and since he can push... following it to get it upstream is ... OK. :-)
Regardless, code still looks good to me so you can feel free to keep my reviewed-by tag for the set, whether patches 1-3 are squashed or not.
Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
I agree with Mat! Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Another side info: I will currently not revert your former patch and apply this patch set on branches devel and rebased but wait until I update to libvirt v1.3.0! If you disagree with this approach please let me know. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294