[libvirt] [PATCH 03/11] storage: Add partition type checks for BLKID probing

[John Ferlan]

On 01/09/2017 10:32 AM, Michal Privoznik wrote:
> On 12/15/2016 10:42 PM, John Ferlan wrote:
>> A device may be formatted using some sort of disk partition format type.
>> We can check that using the blkid_ API's as well - so alter the logic to
>> allow checking the device for both a filesystem and a disk partition.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/storage/storage_backend.c | 177 ++++++++++++++++++++++++++++++++----------
>>  1 file changed, 138 insertions(+), 39 deletions(-)
>>
>> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>> index 108f2b9..065f2ef 100644
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -2639,37 +2639,117 @@ virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED,
>>  
>>  
>>  #if WITH_BLKID
>> +
>> +typedef enum {
>> +    VIR_STORAGE_BLKID_PROBE_ERROR = -1,
>> +    VIR_STORAGE_BLKID_PROBE_UNDEFINED, /* Nothing found */
>> +    VIR_STORAGE_BLKID_PROBE_UNKNOWN,   /* Don't know libvirt fs/part type */
>> +    VIR_STORAGE_BLKID_PROBE_MATCH,     /* Matches the on disk format */
>> +    VIR_STORAGE_BLKID_PROBE_DIFFERENT, /* Format doesn't match on disk format */
>> +} virStorageBackendBLKIDProbeResult;
>> +
>> +/*
>> + * Utility function to probe for a file system on the device using the
>> + * blkid "superblock" (e.g. default) APIs.
>> + *
>> + * NB: In general this helper will handle the virStoragePoolFormatFileSystem
>> + *     format types; however, if called from the Disk path, the initial fstype
>> + *     check will fail forcing the usage of the ProbePart helper.
>> + *
>> + * Returns virStorageBackendBLKIDProbeResult enum
>> + */
>> +static virStorageBackendBLKIDProbeResult
>> +virStorageBackendBLKIDProbeFS(blkid_probe probe,
>> +                              const char *device,
>> +                              const char *format)
>> +{
>> +    const char *fstype = NULL;
>> +
>> +    /* Make sure we're doing a superblock probe from the start */
>> +    blkid_probe_enable_superblocks(probe, true);
>> +    blkid_probe_reset_superblocks_filter(probe);
>> +
>> +    if (blkid_do_probe(probe) != 0) {
>> +        VIR_INFO("No filesystem found on device '%s'", device);
>> +        return VIR_STORAGE_BLKID_PROBE_UNDEFINED;
>> +    }
>> +
>> +    if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) {
>> +        if (STREQ(fstype, format))
>> +            return VIR_STORAGE_BLKID_PROBE_MATCH;
>> +
>> +        return VIR_STORAGE_BLKID_PROBE_DIFFERENT;
>> +    }
>> +
>> +    if (blkid_known_fstype(format) == 0)
>> +        return VIR_STORAGE_BLKID_PROBE_UNKNOWN;
>> +
>> +    return VIR_STORAGE_BLKID_PROBE_ERROR;
>> +}
> 
> I'm confused. So before this patch set this function was returning
> something among these lines. Then in patch #2 you make it return -1 or
> 0, and here we go again.

IIRC, the diffs were really ugly with the 1 step approach.

Maybe if I created a step between 1 and 2 which introduced and
implemented the VIR_STORAGE_BLKID_PROBE_* constants that may make it
easier going from 2 to 3.

John

> 
> The code after this patch looks sane though. And I don't really care
> that much about partial steps. The result looks good, therefore you have
> my ACK.
> 
> Michal
>