2011/4/14 Eric Blake <eblake@xxxxxxxxxx>:
> Rather than copying and pasting lots of code, factor it into a
> single helper function.
>
> * src/phyp/phyp_driver.c (phypExecInt): New function.
> (phypGetVIOSPartitionID, phypNumDomainsGeneric, phypGetLparID)
> (phypGetLparMem, phypGetLparCPUGeneric, phypGetRemoteSlot)
> (phypGetVIOSNextSlotNumber, phypAttachDevice)
> (phypGetStoragePoolSize, phypStoragePoolNumOfVolumes)
> (phypNumOfStoragePools, phypInterfaceDestroy)
> (phypInterfaceDefineXML, phypInterfaceLookupByName)
> (phypInterfaceIsActive, phypNumOfInterfaces): Use it.
> ---
> Âsrc/phyp/phyp_driver.c | Â316 ++++++++++--------------------------------------
> Â1 files changed, 67 insertions(+), 249 deletions(-)
Okay lets take a look at each instance if stricter parsing is safe or not.
> @@ -267,17 +284,7 @@ phypGetVIOSPartitionID(virConnectPtr conn)
> Â Â Â Â virBufferVSprintf(&buf, " -m %s", managed_system);
> Â Â virBufferAddLit(&buf, " -r lpar -F lpar_id,lpar_env"
> Â Â Â Â Â Â Â Â Â Â "|sed -n '/vioserver/ {\n s/,.*$//\n p\n}'");
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &id) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> -
> + Â ÂphypExecInt(session, &buf, conn, &id);
> Â Â return id;
> Â}
In this case I'm not sure that your change is safe. The executed
command looks like it requests a table with two columns
lpar_id,lpar_env and the uses some sed construct to pick the line that
contains "vioserver". Then the lpar_id is parsed from the beginning of
that line. I'm not sure that this sed construct just returns the
number from the beginning of the line. If that is the case that your
stricter parsing it safe. But it looks like that's not that case and
ignoring content after the number in the selected line is important.
> @@ -364,17 +368,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type)
> Â Â Â Â virBufferVSprintf(&buf, " -m %s", managed_system);
> Â Â virBufferVSprintf(&buf, " -F lpar_id,state %s |grep -c '^[0-9]*'",
> Â Â Â Â Â Â Â Â Â Â Â state);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> -
> + Â ÂphypExecInt(session, &buf, conn, &ndom);
> Â Â return ndom;
> Â}
Here the stricter parsing will be safe as the last grep returns a count.
But I wonder about the last grep. I thought it would be there to count
the number of lines that start with a number, but it doesn't work:
$ printf "aa\n22\n33\n" | grep -c '^[0-9]*'
3
I expected it to print 2 here.
$ printf "aa\n22\n33\n\n" | grep -c '^[0-9]*'
4
So the '^[0-9]*' pattern matches every line. But
$ printf "aa\n22\n33\n\n" | grep -c '^[0-9]+'
0
So the last grep here is just a wc -l.
> @@ -1298,27 +1292,14 @@ phypGetLparID(LIBSSH2_SESSION * session, const char *managed_system,
> Â{
> Â Â phyp_driverPtr phyp_driver = conn->privateData;
> Â Â int system_type = phyp_driver->system_type;
> - Â Âint exit_status = 0;
> Â Â int lpar_id = -1;
> - Â Âchar *char_ptr;
> - Â Âchar *ret = NULL;
> Â Â virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> Â Â virBufferAddLit(&buf, "lssyscfg -r lpar");
> Â Â if (system_type == HMC)
> Â Â Â Â virBufferVSprintf(&buf, " -m %s", managed_system);
> Â Â virBufferVSprintf(&buf, " --filter lpar_names=%s -F lpar_id", name);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> -
> + Â ÂphypExecInt(session, &buf, conn, &lpar_id);
> Â Â return lpar_id;
> Â}
This one is safe too, as the requested output is just the lpar_id.
> @@ -1397,17 +1375,7 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id,
> Â Â virBufferVSprintf(&buf,
> Â Â Â Â Â Â Â Â Â Â Â " -r mem --level lpar -F %s --filter lpar_ids=%d",
> Â Â Â Â Â Â Â Â Â Â Â type ? "curr_mem" : "curr_max_mem", lpar_id);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &memory) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> -
> + Â ÂphypExecInt(session, &buf, conn, &memory);
> Â Â return memory;
> Â}
This one is safe too, as the requested output is just the curr_mem or
curr_max_mem.
> @@ -1431,17 +1396,7 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system,
> Â Â virBufferVSprintf(&buf,
> Â Â Â Â Â Â Â Â Â Â Â " -r proc --level lpar -F %s --filter lpar_ids=%d",
> Â Â Â Â Â Â Â Â Â Â Â type ? "curr_max_procs" : "curr_procs", lpar_id);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &vcpus) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> -
> + Â ÂphypExecInt(session, &buf, conn, &vcpus);
> Â Â return vcpus;
> Â}
Safe too.
> @@ -1491,17 +1443,7 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system,
> Â Â Â Â virBufferVSprintf(&buf, " -m %s", managed_system);
> Â Â virBufferVSprintf(&buf, " -r virtualio --rsubtype scsi -F "
> Â Â Â Â Â Â Â Â Â Â Â "remote_slot_num --filter lpar_names=%s", lpar_name);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &remote_slot) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> -
> + Â ÂphypExecInt(session, &buf, conn, &remote_slot);
> Â Â return remote_slot;
> Â}
Safe too.
> @@ -1632,21 +1571,9 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn)
> Â Â Â Â Â Â Â Â Â Â Â "virtual_serial_adapters|sed -e 's/\"//g' -e "
> Â Â Â Â Â Â Â Â Â Â Â "'s/,/\\n/g'|sed -e 's/\\(^[0-9][0-9]\\*\\).*$/\\1/'"
> Â Â Â Â Â Â Â Â Â Â Â "|sort|tail -n 1", profile);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âslot += 1;
> -
> -cleanup:
> - Â ÂVIR_FREE(profile);
> - Â ÂVIR_FREE(ret);
> -
> - Â Âreturn slot;
> + Â Âif (phypExecInt(session, &buf, conn, &slot) < 0)
> + Â Â Â Âreturn -1;
> + Â Âreturn slot + 1;
> Â}
Chain of seds transform line of comma separated items into a list of
\n separated items, then strips the trailing non-number part of each
item. Finally sorts the list and takes the last number.
This makes the stricter parsing safe.
> @@ -1859,12 +1785,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
> Â Â virBufferVSprintf(&buf,
> Â Â Â Â Â Â Â Â Â Â Â " slot_num,backing_device|grep %s|cut -d, -f1",
> Â Â Â Â Â Â Â Â Â Â Â dev->data.disk->src);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
> + Â Âif (phypExecInt(session, &buf, conn, &slot) < 0)
> Â Â Â Â goto cleanup;
The cut -d, -f1 makes stricter parsing safe here.
> @@ -1893,10 +1814,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
> Â Â Â Â Â Â Â Â Â Â Â "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'",
> Â Â Â Â Â Â Â Â Â Â Â domain_name, domain->id, ret, slot,
> Â Â Â Â Â Â Â Â Â Â Â vios_id, vios_name);
> - Â ÂVIR_FREE(ret);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
> + Â Âif (phypExecInt(session, &buf, conn, &slot) < 0)
> Â Â Â Â goto cleanup;
There is no filtering of the output before parsing it here. Therefore,
I'm not sure that stricter parsing is safe here.
> @@ -2016,17 +1931,7 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name)
> Â Â Â Â virBufferAddChar(&buf, '\'');
>
> Â Â virBufferVSprintf(&buf, "|sed '1d; s/ //g'");
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &sp_size) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> -
> + Â ÂphypExecInt(session, &buf, conn, &sp_size);
> Â Â return sp_size;
> Â}
Here the stricter parsing might be unsafe.
> @@ -2530,21 +2432,11 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool)
> Â Â if (system_type == HMC)
> Â Â Â Â virBufferAddChar(&buf, '\'');
> Â Â virBufferVSprintf(&buf, "|grep -c '^.*$'");
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &nvolumes) == -1)
> - Â Â Â Âgoto cleanup;
> + Â Âif (phypExecInt(session, &buf, conn, &nvolumes) < 0)
> + Â Â Â Âreturn -1;
Here it's safe because of grep -c, that could be replaced by wc -l, as
it doesn't have a specific pattern.
> @@ -2650,17 +2539,7 @@ phypNumOfStoragePools(virConnectPtr conn)
> Â Â Â Â virBufferAddChar(&buf, '\'');
>
> Â Â virBufferVSprintf(&buf, "|grep -c '^.*$'");
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &nsp) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> -
> + Â ÂphypExecInt(session, &buf, conn, &nsp);
> Â Â return nsp;
> Â}
Safe because of grep -c.
> @@ -2898,17 +2776,10 @@ phypInterfaceDestroy(virInterfacePtr iface,
> Â Â Â Â Â Â Â Â Â Â Â " -r virtualio --rsubtype eth --level lpar "
> Â Â Â Â Â Â Â Â Â Â Â " -F mac_addr,slot_num|"
> Â Â Â Â Â Â Â Â Â Â Â " sed -n '/%s/ s/^.*,//p'", iface->mac);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &slot_num) == -1)
> + Â Âif (phypExecInt(session, &buf, iface->conn, &slot_num) < 0)
> Â Â Â Â goto cleanup;
The final sed makes stricter parsing safe here.
> @@ -2917,17 +2788,10 @@ phypInterfaceDestroy(virInterfacePtr iface,
> Â Â Â Â Â Â Â Â Â Â Â " -r virtualio --rsubtype eth --level lpar "
> Â Â Â Â Â Â Â Â Â Â Â " -F mac_addr,lpar_id|"
> Â Â Â Â Â Â Â Â Â Â Â " sed -n '/%s/ s/^.*,//p'", iface->mac);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1)
> + Â Âif (phypExecInt(session, &buf, iface->conn, &lpar_id) < 0)
> Â Â Â Â goto cleanup;
Safe too.
> @@ -2980,20 +2843,13 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml,
> Â Â Â Â Â Â Â Â Â Â Â " -r virtualio --rsubtype slot --level slot"
> Â Â Â Â Â Â Â Â Â Â Â " -Fslot_num --filter lpar_names=%s"
> Â Â Â Â Â Â Â Â Â Â Â " |sort|tail -n 1", def->name);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
> + Â Âif (phypExecInt(session, &buf, conn, &slot) < 0)
> Â Â Â Â goto cleanup;
Safe because of -Fslot_num.
> @@ -3094,17 +2949,10 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name)
> Â Â Â Â Â Â Â Â Â Â Â " -r virtualio --rsubtype slot --level slot "
> Â Â Â Â Â Â Â Â Â Â Â " -F drc_name,slot_num |"
> Â Â Â Â Â Â Â Â Â Â Â " sed -n '/%s/ s/^.*,//p'", name);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
> + Â Âif (phypExecInt(session, &buf, conn, &slot) < 0)
> Â Â Â Â goto cleanup;
Safe too because of the last sed.
> Â Â Â Â virBufferVSprintf(&buf, "-m %s ", managed_system);
> @@ -3113,12 +2961,7 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name)
> Â Â Â Â Â Â Â Â Â Â Â " -r virtualio --rsubtype slot --level slot "
> Â Â Â Â Â Â Â Â Â Â Â " -F drc_name,lpar_id |"
> Â Â Â Â Â Â Â Â Â Â Â " sed -n '/%s/ s/^.*,//p'", name);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1)
> + Â Âif (phypExecInt(session, &buf, conn, &lpar_id) < 0)
> Â Â Â Â goto cleanup;
Again, safe too because of the last sed.
> @@ -3167,16 +3006,7 @@ phypInterfaceIsActive(virInterfacePtr iface)
> Â Â Â Â Â Â Â Â Â Â Â " -r virtualio --rsubtype eth --level lpar "
> Â Â Â Â Â Â Â Â Â Â Â " -F mac_addr,state |"
> Â Â Â Â Â Â Â Â Â Â Â " sed -n '/%s/ s/^.*,//p'", iface->mac);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, iface->conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &state) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> + Â ÂphypExecInt(session, &buf, iface->conn, &state);
> Â Â return state;
> Â}
Again, safe too because of the last sed.
> @@ -3260,16 +3087,7 @@ phypNumOfInterfaces(virConnectPtr conn)
> Â Â virBufferVSprintf(&buf,
> Â Â Â Â Â Â Â Â Â Â Â "-r virtualio --rsubtype eth --level lpar|"
> Â Â Â Â Â Â Â Â Â Â Â "grep -v lpar_id=%d|grep -c lpar_name", vios_id);
> - Â Âret = phypExecBuffer(session, &buf, &exit_status, conn, false);
> -
> - Â Âif (exit_status < 0 || ret == NULL)
> - Â Â Â Âgoto cleanup;
> -
> - Â Âif (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1)
> - Â Â Â Âgoto cleanup;
> -
> -cleanup:
> - Â ÂVIR_FREE(ret);
> + Â ÂphypExecInt(session, &buf, conn, &nnets);
> Â Â return nnets;
> Â}
Safe because of grep -c.
In most cases we can see from the code that stricter parsing will be
safe, but in some places I'm not sure.
So, as long as you don't have a PHYP system at hand to really test it,
I'd suggest that we stick to the relaxed parsing.
Matthias
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list