Re: [libvirt] [PATCH 4/4] storage_conf: Resolve libvirtd crash matching scsi_host

On 10/03/2014 09:20 AM, Ján Tomko wrote: > On 09/30/2014 11:35 PM, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1146837 >> >> Resolve a crash in libvirtd resulting from commit id 'a4bd62ad' (1.0.6) >> which added parentaddr and unique_id to allow unique identification of >> a scsi_host, but assumed that all the pool entries and the incoming >> definition would be similarly defined. If the existing pool uses the >> 'name' attribute and an incoming pool is using the parentaddr/unique_id, >> then the code will attempt to compare the existing name string against >> the incoming name string which doesn't exist (is NULL) and results in >> a core (STREQ). >> > > Fixing this crash would be nicer in a separate patch. > This patch does fix the crash and it must fix the side effect to having that check (e.g. both pool and incoming def use name). The crash is the condition where incoming definition doesn't use the same XML format as the already defined pool. Adding in the mismatched definition checks in a prior or future patch doesn't make sense mainly because all that was considered previously was matching definitions. >> Conversely, if the existing pool used the parentaddr/unique_id and the >> to be defined pool used the name, then the comparison would be against >> the parentaddr, but since the incoming pool doesn't have one - that would >> leave the comparison against a parentaddr of all 0's and a unique_id of 0, >> which will always comparison to fail. This means someone could define the >> same source adapter for two pools > > When defining a storage pool, we don't check if the adapter name or > parentaddr/unique_id is valid, so I don't think we should require it to be > valid to detect duplicates. If you mean we don't check that the name starts with 'scsi' or 'scsi_host', then sure I agree, but that would be a different bug or issue. I can certainly add a check if that's desired to ensure prefix is correct. Of course, the docs : http://libvirt.org/formatstorage.html do provide the rules for the name property (and less so for the parent).